Re: [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup

[Roger Pau Monne <roger.pau@citrix.com>]

Ian Jackson escribió:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index c7e057d..36d58cd 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -356,7 +356,6 @@ int libxl__device_disk_dev_number(const char *virtpath, int *pdisk,
>>       return -1;
>>   }
>>
>> -
>
> Unrelated whitespace change.

Done.

>> +/*
>> + * Perfrom recursive cleanup of xenstore path, from top to bottom
>> + * just like xenstore-rm -t
>> + */
>> +_hidden int libxl__xs_path_cleanup(libxl__gc *gc, char *path);
>
> I think, following my confusion, that this needs some better
> documentation comment.

Ok, I've tried to change to something more self-explaining.

>> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
>> index 3ea8d08..0b1b844 100644
>> --- a/tools/libxl/libxl_xshelp.c
>> +++ b/tools/libxl/libxl_xshelp.c
>> @@ -135,6 +135,28 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
>>       return s;
>>   }
>>
>> +int libxl__xs_path_cleanup(libxl__gc *gc, char *path)
>> +{
>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>> +    unsigned int nb = 0;
>> +    char *last;
>> +
>> +    if (!path)
>> +        return 0;
>> +
>> +    xs_rm(ctx->xsh, XBT_NULL, path);
>> +
>> +    for (last = strrchr(path, '/'); last != NULL; last = strrchr(path, '/')) {
>
> If the path is relative, this won't work correctly.

Are you sure? From what I've tested it seems to work ok.

>
> Also this whole thing needs to take place in a transaction, or it is
> racy.  Probably a transaction supplied by the caller, in which case
> you should assert it.

Ok, will add another lock.

>> +        *last = '\0';
>> +        if (!libxl__xs_directory(gc, XBT_NULL, path,&nb))
>> +            continue;
>
> If this fails, it should be a fatal error; we should not just blunder
> on.
>
> Ian.