Re: [Xen-devel] [PATCH 04/13 v2] libxl: don't leak p in libxl__wait_for_backend

[Matthew Daley <mattd@xxxxxxxxxxx>]

On Mon, Dec 2, 2013 at 1:42 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/12/2013 00:27, Matthew Daley wrote:
>> Use libxl__xs_read_checked instead of xs_read. While at it, tidy up the
>> function as well.
>>
>> Coverity-ID: 1055891
>> Signed-off-by: Matthew Daley <mattd@xxxxxxxxxxx>
>> ---
>> v2: Use libxl__xs_read_checked instead of xs_read. Tidy up the function.
>>
>>  tools/libxl/libxl_device.c   |   41 
>> +++++++++++++++++------------------------
>>  tools/libxl/libxl_internal.h |    3 ++-
>>  2 files changed, 19 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index d995c83..ba7d100 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -1199,37 +1199,30 @@ int 
>> libxl__wait_for_device_model_deprecated(libxl__gc *gc,
>>                                       check_callback, 
>> check_callback_userdata);
>>  }
>>
>> -int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
>> +int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
>> +                            const char *state)
>>  {
>> -    libxl_ctx *ctx = libxl__gc_owner(gc);
>>      int watchdog = 100;
>> -    unsigned int len;
>> -    char *p;
>> -    char *path = GCSPRINTF("%s/state", be_path);
>> -    int rc = -1;
>> +    const char *p, *path = GCSPRINTF("%s/state", be_path);
>> +    int rc;
>> +
>> +    while (watchdog-- > 0) {
>> +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &p);
>
> libxl__xs_read_checked() can return 0, with p set to NULL in the case of
> an ENOENT from xenstore.
>
> I think you need a NULL check before strcmp.
>
> ~Andrew
>
>> +        if (rc) return rc;
>>
>> -    while (watchdog > 0) {
>> -        p = xs_read(ctx->xsh, XBT_NULL, path, &len);
>>          if (p == NULL) {

^ That's checked here, isn't it? (The diff has sneakily left it here
inbetween a bunch of removals)

- Matthew

>> -            if (errno == ENOENT) {
>> -                LOG(ERROR, "Backend %s does not exist", be_path);
>> -            } else {
>> -                LOGE(ERROR, "Failed to access backend %s", be_path);
>> -            }
>> -            goto out;
>> -        } else {
>> -            if (!strcmp(p, state)) {
>> -                rc = 0;
>> -                goto out;
>> -            } else {
>> -                usleep(100000);
>> -                watchdog--;
>> -            }
>> +            LOG(ERROR, "Backend %s does not exist", be_path);
>> +            return ERROR_FAIL;
>>          }
>> +
>> +        if (!strcmp(p, state))
>> +            return 0;
>> +
>> +        usleep(100000);
>>      }
>> +
>>      LOG(ERROR, "Backend %s not ready", be_path);
>> -out:
>> -    return rc;
>> +    return ERROR_FAIL;
>>  }
>>
>>  /*
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index a2d8247..1bd23ff 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -944,7 +944,8 @@ _hidden char *libxl__device_frontend_path(libxl__gc *gc, 
>> libxl__device *device);
>>  _hidden int libxl__parse_backend_path(libxl__gc *gc, const char *path,
>>                                        libxl__device *dev);
>>  _hidden int libxl__device_destroy(libxl__gc *gc, libxl__device *dev);
>> -_hidden int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char 
>> *state);
>> +_hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
>> +                                    const char *state);
>>  _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
>>                              libxl_nic_type *nictype);
>>
>

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