[PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[Doug Goldstein]

The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
uninitialized in main_vdisplattach().

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
CC: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/xl/xl_vdispl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xl/xl_vdispl.c b/tools/xl/xl_vdispl.c
index 3cc99b6aed..8fbad5be68 100644
--- a/tools/xl/xl_vdispl.c
+++ b/tools/xl/xl_vdispl.c
@@ -25,7 +25,7 @@
 int main_vdisplattach(int argc, char **argv)
 {
     int opt;
-    int rc;
+    int rc = ERROR_FAIL;
     uint32_t domid;
     libxl_device_vdispl vdispl;
 
-- 
2.16.1


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

Re: [PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[Jan Beulich]

>>> On 13.03.18 at 05:43, <cardoe@cardoe.com> wrote:
> The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
> uninitialized in main_vdisplattach().
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> ---
> CC: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> ---
>  tools/xl/xl_vdispl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please trim your Cc list - I've removed all individuals here, and I
don't see why you've copied all REST maintainers when this is
clearly a pure tool stack change.

Jan


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

Re: [PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[Oleksandr Grytsov]

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

On Tue, Mar 13, 2018 at 10:03 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 13.03.18 at 05:43, <cardoe@cardoe.com> wrote:
> > The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
> > uninitialized in main_vdisplattach().
> >
> > Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> > ---
> > CC: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > ---
> >  tools/xl/xl_vdispl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Please trim your Cc list - I've removed all individuals here, and I
> don't see why you've copied all REST maintainers when this is
> clearly a pure tool stack change.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
>


Hi, Doug,

Thanks for pointing it out.

The implementation is done according to CODING_STYLE document (see ERROR
HANDLING)
which requests to define return value rc uninitialized. The only path where
rc is uninitialized it is
when dryrun_only set to true. So, proper fix should be:

    if (dryrun_only) {
        char *json = libxl_device_vdispl_to_json(ctx, &vdispl);
        printf("vdispl: %s\n", json);
        free(json);
+      rc = 0;
        goto out;
    }


-- 
Best Regards,
Oleksandr Grytsov.

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

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

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

Re: [PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[George Dunlap]

On 03/13/2018 04:43 AM, Doug Goldstein wrote:
> The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
> uninitialized in main_vdisplattach().
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>

It looks like that was designed on purpose to use the uninitialized
warnings to catch paths where the rc wasn't specifically set.

The only path where it's not set is if dryrun_only is true; and in that
case, we probably actually want it to succeed, not fail.

No matter what, rc = 0 should be added to the dryrun_only path.  If we
want to make ERROR_FAIL the default, then the various places rc is set
to ERROR_FAIL should be removed.

 -George

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

Re: [PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[Wei Liu]

On Tue, Mar 13, 2018 at 10:56:35AM +0000, George Dunlap wrote:
> On 03/13/2018 04:43 AM, Doug Goldstein wrote:
> > The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
> > uninitialized in main_vdisplattach().
> > 
> > Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> 
> It looks like that was designed on purpose to use the uninitialized
> warnings to catch paths where the rc wasn't specifically set.
> 
> The only path where it's not set is if dryrun_only is true; and in that
> case, we probably actually want it to succeed, not fail.
> 
> No matter what, rc = 0 should be added to the dryrun_only path.  If we
> want to make ERROR_FAIL the default, then the various places rc is set
> to ERROR_FAIL should be removed.

Yeah, CODING_STYLE dictates rc to not be initialised.

Setting rc = 0 in the appropriate place is the right solution.

Wei.

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

Re: [PATCH] tools/xl: fix uninitialized variable in xl_vdispl

[Doug Goldstein]

On 3/13/18 3:03 AM, Jan Beulich wrote:
>>>> On 13.03.18 at 05:43, <cardoe@cardoe.com> wrote:
>> The code added in 7a48622a78a0b452e8afa55b8442c958abd226a7 could use rc
>> uninitialized in main_vdisplattach().
>>
>> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
>> ---
>> CC: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>> ---
>>  tools/xl/xl_vdispl.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Please trim your Cc list - I've removed all individuals here, and I
> don't see why you've copied all REST maintainers when this is
> clearly a pure tool stack change.
> 
> Jan
> 

Yea it was a mistake on my part. Submitting patches late at night caused
me to add a -f to get_maintainer.pl

-- 
Doug Goldstein

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