Re: [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/11/13 11:32, Ian Jackson wrote:
> Andrew Cooper writes ("[Xen-devel] [Patch v3 3/4] tools/libxl: Fix libxl__device_nic_from_xs_be()"):
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> -    tmp = xs_read(ctx->xsh, XBT_NULL,
>> -                  libxl__sprintf(gc, "%s/handle", be_path), &len);
>> -    if ( tmp )
>> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                libxl__sprintf(gc, "%s/handle", be_path),
>> +                                &tmp);
>> +
>> +    if ((rc == 0) && strlen(tmp))
> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> (for the benefit of Ian C.)
>
> This is not correct.  See the doc comment for libxl__xs_read_checked:
>
>     /* On success, *result_out came from the gc.
>      * On error, *result_out is undefined.
>      * ENOENT counts as success but sets *result_out=0
>      */
>     int libxl__xs_read_checked(libxl__gc *gc, xs_transaction_t t,
>                                const char *path, const char **result_out);
>
> So the correct pattern is:
>
>     rc = libxl__xs_read_checked(gc, XBT_NULL, blah blah blah, &tmp);
>     if (rc) goto out;
>
>     if (tmp) {
>         use tmp;
>     } else {
>         the path doesn't exist, do the other thing;
>     }
>
> I don't think there should be any need to check for empty strings
> written to xenstore here ?  The old code doesn't.  Please someone tell
> me there isn't.
>
> Thanks,
> Ian.

Ah - I think I have gotten the wrong indirection on tmp when attempting
to apply the documented ENOENT behaviour.

As this function cant fail, I was trying to force all error paths to
apply safe defaults to the libxl_device_nic structure.

I believe substituting the strlen(tmp) check for NULL checks will
produce the intended behaviour?

~Andrew