Mailing List Archive: Xen: Devel

[PATCH v3 2/5] libxl: add libxl__xs_path_cleanup

Index | Next | Previous | View Threaded

roger.pau at citrix

Apr 20, 2012, 6:23 AM

Post #1 of 7 (87 views)
Permalink

[PATCH v3 2/5] libxl: add libxl__xs_path_cleanup

Add a function which behaves like "xenstore-rm -t", and which will be used to
clean xenstore after unplug since we will be no longer executing
xen-hotplug-cleanup script, that used to do that for us.

Changes since v2:

* Moved the function to libxl_xshelp.c and added the prototype to
libxl_internal.h.

Signed-off-by: Roger Pau Monne <roger.pau [at] citrix>
---
tools/libxl/libxl_device.c | 1 -
tools/libxl/libxl_internal.h | 6 ++++++
tools/libxl/libxl_xshelp.c | 22 ++++++++++++++++++++++
3 files changed, 28 insertions(+), 1 deletions(-)

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

-
typedef struct {
libxl__ao *ao;
libxl__ev_devstate ds;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9e15f95..020810a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -458,6 +458,12 @@ _hidden bool libxl__xs_mkdir(libxl__gc *gc, xs_transaction_t t,

_hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);

+/*
+ * 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);
+

/*
* Event generation functions provided by the libxl event core to the
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, '/')) {
+ *last = '\0';
+ if (!libxl__xs_directory(gc, XBT_NULL, path, &nb))
+ continue;
+ if (nb == 0) {
+ xs_rm(ctx->xsh, XBT_NULL, path);
+ }
+ }
+ return 0;
+}
+
/*
* Local variables:
* mode: C
--
1.7.7.5 (Apple Git-26)

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Ian.Jackson at eu

Apr 23, 2012, 8:33 AM

Post #2 of 7 (90 views)
Permalink

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

Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
> Add a function which behaves like "xenstore-rm -t", and which will be used to
> clean xenstore after unplug since we will be no longer executing
> xen-hotplug-cleanup script, that used to do that for us.

This is all rather odd. I hadn't previously noticed the existence of
xenstore-rm -t.

With the C xenstored, the RM command will delete a whole directory
tree, regardless of its contents. This is documented in
docs/misc/xenstore.txt.

The comment in xs.c near xs_rm, which says that directories must be
empty, seems to be wrong.

What does oxenstored do ? I'm tempted to say that it should follow
the C xenstored behaviour.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

roger.pau at citrix

Apr 23, 2012, 8:42 AM

Post #3 of 7 (85 views)
Permalink

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

Ian Jackson escribió:
> Roger Pau Monne writes ("[Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
>> Add a function which behaves like "xenstore-rm -t", and which will be used to
>> clean xenstore after unplug since we will be no longer executing
>> xen-hotplug-cleanup script, that used to do that for us.
>
> This is all rather odd. I hadn't previously noticed the existence of
> xenstore-rm -t.
>
> With the C xenstored, the RM command will delete a whole directory
> tree, regardless of its contents. This is documented in
> docs/misc/xenstore.txt.

This is a recursive delete, from top to bottom, let me put an example
which will make this clear, since probably the title is wrong. Imagine
you have the following xenstore entry:

/foo/bar/baz = 123

If you do a:

xenstore-rm /foo/bar/baz

the following will remain in xenstore:

/foo/bar

What this function does is clean empty folders that contained the
deleted entry, so using this function on /foo/bar/baz would have cleaned
the whole directory.

> The comment in xs.c near xs_rm, which says that directories must be
> empty, seems to be wrong.
>
> What does oxenstored do ? I'm tempted to say that it should follow
> the C xenstored behaviour.
>
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Ian.Jackson at eu

Apr 23, 2012, 9:49 AM

Post #4 of 7 (89 views)
Permalink

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

Roger Pau Monne writes ("Re: [Xen-devel] [PATCH v3 2/5] libxl: add libxl__xs_path_cleanup"):
> Ian Jackson escribió:
> > With the C xenstored, the RM command will delete a whole directory
> > tree, regardless of its contents. This is documented in
> > docs/misc/xenstore.txt.
>
> This is a recursive delete, from top to bottom, let me put an example
> which will make this clear, since probably the title is wrong. Imagine
> you have the following xenstore entry:
>
> /foo/bar/baz = 123
>
> If you do a:
>
> xenstore-rm /foo/bar/baz
>
> the following will remain in xenstore:
>
> /foo/bar
>
> What this function does is clean empty folders that contained the
> deleted entry, so using this function on /foo/bar/baz would have cleaned
> the whole directory.

Oh! This was quite unclear to me and I didn't read your code closely
enough to spot this. I'll go back and read your patch again.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Ian.Jackson at eu

Apr 23, 2012, 9:52 AM

Post #5 of 7 (90 views)
Permalink

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

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.

> +/*
> + * 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.

> 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.

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.

> + *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.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

roger.pau at citrix

Apr 25, 2012, 6:19 AM

Post #6 of 7 (80 views)
Permalink

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

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.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

roger.pau at citrix

May 2, 2012, 2:44 AM

Post #7 of 7 (74 views)
Permalink

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

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.
>
>> +/*
>> + * 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.
>
>> 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.
>
> 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.

This cannot be done inside of a transaction, because we cannot check
that the directory is empty if the remove has not actually taken place,
and checking that there are zero or one elements (the one we 'had'
removed) can lead to unexpected results, as someone might be deleting
elements on our back and we might actually delete a directory that still
has valid entries.

>
>> + *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.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads