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

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

On 02/12/2013 00:46, Matthew Daley wrote:
> On Mon, Dec 2, 2013 at 1:42 PM, Andrew Cooper <andrew.cooper3@citrix.com> 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@bugfuzz.com>
>>> ---
>>> 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

So it is.  That was sneaky.

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

>
>>> -            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);
>>>