[PATCH] libxl: init_acpi_config should return rc in exit path

[PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

... otherwise it returns 0 even if the function fails.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

This should be backported to 4.8.
---
 tools/libxl/libxl_x86_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
index 686ac8e..6cf0c30 100644
--- a/tools/libxl/libxl_x86_acpi.c
+++ b/tools/libxl/libxl_x86_acpi.c
@@ -154,7 +154,7 @@ static int init_acpi_config(libxl__gc *gc,
     config->acpi_revision = 5;
 
 out:
-    return 0;
+    return rc;
 }
 
 int libxl__dom_load_acpi(libxl__gc *gc,
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 784 bytes --]

On 14/12/16 11:44, Wei Liu wrote:
> ... otherwise it returns 0 even if the function fails.

Coverity-ID: 1397121

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>
> This should be backported to 4.8.
> ---
>  tools/libxl/libxl_x86_acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> index 686ac8e..6cf0c30 100644
> --- a/tools/libxl/libxl_x86_acpi.c
> +++ b/tools/libxl/libxl_x86_acpi.c
> @@ -154,7 +154,7 @@ static int init_acpi_config(libxl__gc *gc,
>      config->acpi_revision = 5;
>  
>  out:
> -    return 0;
> +    return rc;
>  }
>  
>  int libxl__dom_load_acpi(libxl__gc *gc,


[-- Attachment #1.2: Type: text/html, Size: 1732 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

On Wed, Dec 14, 2016 at 11:49:34AM +0000, Andrew Cooper wrote:
> On 14/12/16 11:44, Wei Liu wrote:
> > ... otherwise it returns 0 even if the function fails.
> 
> Coverity-ID: 1397121
> 

Does it work this way? Coverity does lead to the discovery of this bug,
but this CID doesn't reveal the bug directly.
 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> > This should be backported to 4.8.
> > ---
> >  tools/libxl/libxl_x86_acpi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> > index 686ac8e..6cf0c30 100644
> > --- a/tools/libxl/libxl_x86_acpi.c
> > +++ b/tools/libxl/libxl_x86_acpi.c
> > @@ -154,7 +154,7 @@ static int init_acpi_config(libxl__gc *gc,
> >      config->acpi_revision = 5;
> >  
> >  out:
> > -    return 0;
> > +    return rc;
> >  }
> >  
> >  int libxl__dom_load_acpi(libxl__gc *gc,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Ian Jackson]

Wei Liu writes ("[PATCH] libxl: init_acpi_config should return rc in exit path"):
> ... otherwise it returns 0 even if the function fails.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

FTR, I'm happy for obvious patches from you, like this one, to go in
without waiting for my ack!

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

On Fri, Dec 16, 2016 at 11:05:03AM +0000, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] libxl: init_acpi_config should return rc in exit path"):
> > ... otherwise it returns 0 even if the function fails.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 

Thanks.

> FTR, I'm happy for obvious patches from you, like this one, to go in
> without waiting for my ack!
> 

Noted.

> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Boris Ostrovsky]

On 12/14/2016 06:49 AM, Andrew Cooper wrote:
> On 14/12/16 11:44, Wei Liu wrote:
>> ... otherwise it returns 0 even if the function fails.
>
> Coverity-ID: 1397121
>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>>
>> This should be backported to 4.8.
>> ---
>>  tools/libxl/libxl_x86_acpi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
>> index 686ac8e..6cf0c30 100644
>> --- a/tools/libxl/libxl_x86_acpi.c
>> +++ b/tools/libxl/libxl_x86_acpi.c
>> @@ -154,7 +154,7 @@ static int init_acpi_config(libxl__gc *gc,
>>      config->acpi_revision = 5;
>>  
>>  out:
>> -    return 0;
>> +    return rc;

This breaks init_acpi_config(): xc_domain_getinfo() at the top of
returns a positive number (number of domains).

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

On Fri, Dec 16, 2016 at 10:10:43AM -0500, Boris Ostrovsky wrote:
> On 12/14/2016 06:49 AM, Andrew Cooper wrote:
> > On 14/12/16 11:44, Wei Liu wrote:
> >> ... otherwise it returns 0 even if the function fails.
> >
> > Coverity-ID: 1397121
> >
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> >> ---
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >>
> >> This should be backported to 4.8.
> >> ---
> >>  tools/libxl/libxl_x86_acpi.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> >> index 686ac8e..6cf0c30 100644
> >> --- a/tools/libxl/libxl_x86_acpi.c
> >> +++ b/tools/libxl/libxl_x86_acpi.c
> >> @@ -154,7 +154,7 @@ static int init_acpi_config(libxl__gc *gc,
> >>      config->acpi_revision = 5;
> >>  
> >>  out:
> >> -    return 0;
> >> +    return rc;
> 
> This breaks init_acpi_config(): xc_domain_getinfo() at the top of
> returns a positive number (number of domains).
> 

I see, I think we also need to set rc=0 in the success path.

Wei.

> -boris
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

On Fri, Dec 16, 2016 at 10:21:07AM -0500, Boris Ostrovsky wrote:
> On 12/16/2016 10:16 AM, Wei Liu wrote:
> >
> > I see, I think we also need to set rc=0 in the success path.
> 
> Or 'return rc < 0 ? rc : 0;'
> 

I don't think so, because there could be functions that only return >0
values to indicate error.

Wei.

> -boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Boris Ostrovsky]

On 12/16/2016 10:16 AM, Wei Liu wrote:
>
> I see, I think we also need to set rc=0 in the success path.

Or 'return rc < 0 ? rc : 0;'

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: init_acpi_config should return rc in exit path"):
> I don't think so, because there could be functions that only return >0
> values to indicate error.

The problem here is the violation of the CODING_STYLE which forbids
the use of `rc' to contain anything other than a libxl error value.

`r' should be used for the values from libxc.  And each failure path
should set rc.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Wei Liu]

On Wed, Dec 14, 2016 at 11:44:36AM +0000, Wei Liu wrote:
> ... otherwise it returns 0 even if the function fails.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> This should be backported to 4.8.

And please don't just apply this one patch because it is buggy.

You would also need "libxl: set rc to 0 in init_acpi_config in success
path".

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] libxl: init_acpi_config should return rc in exit path

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] libxl: init_acpi_config should return rc in exit path"):
> 
> And please don't just apply this one patch because it is buggy.
> 
> You would also need "libxl: set rc to 0 in init_acpi_config in success
> path".

Noted, thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel