[v2,1/2] compositor: Propagate errors from module_init

Submitted by Ondřej Majerech on Dec. 6, 2014, 1:49 a.m.

Details

Message ID ea98e026700a98ad23a058ea5d9917461d2d7ef8.1417830473.git.majerech.o@gmail.com
State Accepted
Commit 01e98b65c6d526edf6cfbd22d254bd7804bf8790
Headers show

Not browsing as part of any series.

Commit Message

Ondřej Majerech Dec. 6, 2014, 1:49 a.m.
load_modules currently ignores errors signalled by both
weston_load_module and module_init, and instead always returns 0. Its
return value appears to be checked in callers, so we most likely want to
propagate any errors.

Signed-off-by: Ondřej Majerech <majerech.o@gmail.com>
---
 src/compositor.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compositor.c b/src/compositor.c
index 53f6220..8dddd67 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -4423,8 +4423,10 @@  load_modules(struct weston_compositor *ec, const char *modules,
 		end = strchrnul(p, ',');
 		snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
 		module_init = weston_load_module(buffer, "module_init");
-		if (module_init)
-			module_init(ec, argc, argv);
+		if (!module_init)
+			return -1;
+		if (module_init(ec, argc, argv) < 0)
+			return -1;
 		p = end;
 		while (*p == ',')
 			p++;

Comments

On 05/12/14 07:49 PM, Ondřej Majerech wrote:
> load_modules currently ignores errors signalled by both
> weston_load_module and module_init, and instead always returns 0. Its
> return value appears to be checked in callers, so we most likely want to
> propagate any errors.
> 
> Signed-off-by: Ondřej Majerech <majerech.o@gmail.com>
> ---
>  src/compositor.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 53f6220..8dddd67 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -4423,8 +4423,10 @@ load_modules(struct weston_compositor *ec, const char *modules,
>  		end = strchrnul(p, ',');
>  		snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
>  		module_init = weston_load_module(buffer, "module_init");
> -		if (module_init)
> -			module_init(ec, argc, argv);
> +		if (!module_init)
> +			return -1;
> +		if (module_init(ec, argc, argv) < 0)
> +			return -1;

This looks like a good idea to me - wouldn't mind seeing a weston_log()
here to let us know what failed and why, but even so:

Reviewed-by: Derek Foreman <derekf@osg.samsung.com>

>  		p = end;
>  		while (*p == ',')
>  			p++;
>
On Tue, 09 Dec 2014 10:19:22 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 05/12/14 07:49 PM, Ondřej Majerech wrote:
> > load_modules currently ignores errors signalled by both
> > weston_load_module and module_init, and instead always returns 0.
> > Its return value appears to be checked in callers, so we most
> > likely want to propagate any errors.
> > 
> > Signed-off-by: Ondřej Majerech <majerech.o@gmail.com>
> > ---
> >  src/compositor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 53f6220..8dddd67 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -4423,8 +4423,10 @@ load_modules(struct weston_compositor *ec,
> > const char *modules, end = strchrnul(p, ',');
> >  		snprintf(buffer, sizeof buffer, "%.*s", (int) (end
> > - p), p); module_init = weston_load_module(buffer, "module_init");
> > -		if (module_init)
> > -			module_init(ec, argc, argv);
> > +		if (!module_init)
> > +			return -1;
> > +		if (module_init(ec, argc, argv) < 0)
> > +			return -1;
> 
> This looks like a good idea to me - wouldn't mind seeing a
> weston_log() here to let us know what failed and why, but even so:

weston_load_module already says what failed, so no need to report that
again. And for an error coming from module_init, you don't really know
what the problem might be (other than that the initialisation has
failed), so I think it should be the job of module_init to report
what's wrong. 

> 
> Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
> 
> >  		p = end;
> >  		while (*p == ',')
> >  			p++;
> > 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On 09/12/14 12:29 PM, Ondřej Majerech wrote:
> On Tue, 09 Dec 2014 10:19:22 -0600
> Derek Foreman <derekf@osg.samsung.com> wrote:
> 
>> On 05/12/14 07:49 PM, Ondřej Majerech wrote:
>>> load_modules currently ignores errors signalled by both
>>> weston_load_module and module_init, and instead always returns 0.
>>> Its return value appears to be checked in callers, so we most
>>> likely want to propagate any errors.
>>>
>>> Signed-off-by: Ondřej Majerech <majerech.o@gmail.com>
>>> ---
>>>  src/compositor.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/compositor.c b/src/compositor.c
>>> index 53f6220..8dddd67 100644
>>> --- a/src/compositor.c
>>> +++ b/src/compositor.c
>>> @@ -4423,8 +4423,10 @@ load_modules(struct weston_compositor *ec,
>>> const char *modules, end = strchrnul(p, ',');
>>>  		snprintf(buffer, sizeof buffer, "%.*s", (int) (end
>>> - p), p); module_init = weston_load_module(buffer, "module_init");
>>> -		if (module_init)
>>> -			module_init(ec, argc, argv);
>>> +		if (!module_init)
>>> +			return -1;
>>> +		if (module_init(ec, argc, argv) < 0)
>>> +			return -1;
>>
>> This looks like a good idea to me - wouldn't mind seeing a
>> weston_log() here to let us know what failed and why, but even so:
> 
> weston_load_module already says what failed, so no need to report that
> again. And for an error coming from module_init, you don't really know
> what the problem might be (other than that the initialisation has
> failed), so I think it should be the job of module_init to report
> what's wrong. 

A-ha, you are correct.  :)
On Tue, 09 Dec 2014 10:19:22 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 05/12/14 07:49 PM, Ondřej Majerech wrote:
> > load_modules currently ignores errors signalled by both
> > weston_load_module and module_init, and instead always returns 0. Its
> > return value appears to be checked in callers, so we most likely want to
> > propagate any errors.
> > 
> > Signed-off-by: Ondřej Majerech <majerech.o@gmail.com>
> > ---
> >  src/compositor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index 53f6220..8dddd67 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -4423,8 +4423,10 @@ load_modules(struct weston_compositor *ec, const char *modules,
> >  		end = strchrnul(p, ',');
> >  		snprintf(buffer, sizeof buffer, "%.*s", (int) (end - p), p);
> >  		module_init = weston_load_module(buffer, "module_init");
> > -		if (module_init)
> > -			module_init(ec, argc, argv);
> > +		if (!module_init)
> > +			return -1;
> > +		if (module_init(ec, argc, argv) < 0)
> > +			return -1;
> 
> This looks like a good idea to me - wouldn't mind seeing a weston_log()
> here to let us know what failed and why, but even so:
> 
> Reviewed-by: Derek Foreman <derekf@osg.samsung.com>

Pushed, thanks,
pq