[PATCH] fix rename: xenstore not fully updated

[PATCH] fix rename: xenstore not fully updated

[Chunyan Liu]

Currently libxl__domain_rename only update /local/domain/<domid>/name,
still some places in xenstore are not updated, including:
/vm/<uuid>/name and /local/domain/0/backend/<device>/<domid>/.../domain.

Signed-off-by: Chunyan Liu <cyliu@suse.com>
---
 tools/libxl/libxl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f7961f6..6671b08 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -359,6 +359,9 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
     uint32_t stub_dm_domid;
     const char *stub_dm_old_name = NULL, *stub_dm_new_name = NULL;
     int rc;
+    const char *vm_path, *vm_name_path;
+    char** be_dev = NULL;
+    unsigned int ndevs = 0;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path) goto x_nomem;
@@ -429,6 +432,58 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
         goto x_fail;
     }
 
+    /* update /vm/<uuid>/name */
+    vm_path = libxl__xs_read(gc, trans, libxl__sprintf(gc, "%s/vm", dom_path));
+    vm_name_path = libxl__sprintf(gc, "%s/name", vm_path);
+    if (libxl__xs_write_checked(gc, trans, vm_name_path, new_name)) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to write new name `%s'"
+                   " to %s", new_name, vm_name_path);
+        goto x_fail;
+    }
+
+
+    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
+    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
+    if (be_dev && ndevs) {
+        for (int i = 0; i < ndevs; i++, be_dev++) {
+            /* <device>: vbd, vif, vkbd, ... */
+            char** be_dom = NULL;
+            unsigned int ndoms = 0;
+            be_dom = libxl__xs_directory(gc, trans,
+                     GCSPRINTF("/local/domain/0/backend/%s", *be_dev), &ndoms);
+            if (be_dom && ndoms) {
+                for (int j = 0; j < ndoms; j++, be_dom++) {
+                    /* <device>/<domid>: vif/1, vif/2, ...*/
+                    char ** be_devid = NULL;
+                    unsigned int ndevids = 0;
+
+                    if (strcmp(*be_dom, GCSPRINTF("%d", domid)))
+                        continue;
+
+                    be_devid = libxl__xs_directory(gc, trans,
+                                     GCSPRINTF("/local/domain/0/backend/%s/%s",
+                                     *be_dev, *be_dom),
+                                     &ndevids);
+                    if (be_devid && ndevids) {
+                        for (int k = 0; k < ndevids; k++, be_devid++) {
+                            /* <device>/<domid>/<devid>: vif/1/0, vif/1/1, ... */
+                            char *tmp = GCSPRINTF("/local/domain/0/backend/%s/%s/%s/domain",
+                                                  *be_dev, *be_dom, *be_devid);
+                            if (!libxl__xs_read(gc, trans, tmp))
+                                continue;
+
+                            if (libxl__xs_write_checked(gc, trans, tmp, new_name)) {
+                                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to write new name `%s'"
+                                           " to %s", new_name, tmp);
+                                goto x_fail;
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     if (stub_dm_domid) {
         rc = libxl__domain_rename(gc, stub_dm_domid,
                                   stub_dm_old_name,
-- 
1.8.4.5

Re: [PATCH] fix rename: xenstore not fully updated

[Wei Liu]

Is this a regression? Can it wait until 4.6?

On Mon, Nov 17, 2014 at 05:19:47PM +0800, Chunyan Liu wrote:
> Currently libxl__domain_rename only update /local/domain/<domid>/name,
> still some places in xenstore are not updated, including:
> /vm/<uuid>/name and /local/domain/0/backend/<device>/<domid>/.../domain.
> 

I noticed this problem a few days ago and thanks for looking into this.

I have some comments, though.

> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  tools/libxl/libxl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index f7961f6..6671b08 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -359,6 +359,9 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>      uint32_t stub_dm_domid;
>      const char *stub_dm_old_name = NULL, *stub_dm_new_name = NULL;
>      int rc;
> +    const char *vm_path, *vm_name_path;
> +    char** be_dev = NULL;
> +    unsigned int ndevs = 0;
>  
>      dom_path = libxl__xs_get_dompath(gc, domid);
>      if (!dom_path) goto x_nomem;
> @@ -429,6 +432,58 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
>          goto x_fail;
>      }
>  
> +    /* update /vm/<uuid>/name */
> +    vm_path = libxl__xs_read(gc, trans, libxl__sprintf(gc, "%s/vm", dom_path));
> +    vm_name_path = libxl__sprintf(gc, "%s/name", vm_path);
> +    if (libxl__xs_write_checked(gc, trans, vm_name_path, new_name)) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to write new name `%s'"
> +                   " to %s", new_name, vm_name_path);
> +        goto x_fail;
> +    }
> +
> +
> +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);

The hard-coded 0 cannot work if you have driver domain.

At the very least it should be using LIBXL_TOOLSTACK_DOMID.

> +    if (be_dev && ndevs) {
> +        for (int i = 0; i < ndevs; i++, be_dev++) {
> +            /* <device>: vbd, vif, vkbd, ... */
> +            char** be_dom = NULL;
> +            unsigned int ndoms = 0;
> +            be_dom = libxl__xs_directory(gc, trans,
> +                     GCSPRINTF("/local/domain/0/backend/%s", *be_dev), &ndoms);
> +            if (be_dom && ndoms) {
> +                for (int j = 0; j < ndoms; j++, be_dom++) {
> +                    /* <device>/<domid>: vif/1, vif/2, ...*/
> +                    char ** be_devid = NULL;
> +                    unsigned int ndevids = 0;
> +
> +                    if (strcmp(*be_dom, GCSPRINTF("%d", domid)))
> +                        continue;
> +
> +                    be_devid = libxl__xs_directory(gc, trans,
> +                                     GCSPRINTF("/local/domain/0/backend/%s/%s",
> +                                     *be_dev, *be_dom),
> +                                     &ndevids);
> +                    if (be_devid && ndevids) {
> +                        for (int k = 0; k < ndevids; k++, be_devid++) {
> +                            /* <device>/<domid>/<devid>: vif/1/0, vif/1/1, ... */
> +                            char *tmp = GCSPRINTF("/local/domain/0/backend/%s/%s/%s/domain",
> +                                                  *be_dev, *be_dom, *be_devid);

Same here.

And I would like to request this hunk to be in a separate function if
possible. I got lost when counting the closing "}". ;-)

Wei.

> +                            if (!libxl__xs_read(gc, trans, tmp))
> +                                continue;
> +
> +                            if (libxl__xs_write_checked(gc, trans, tmp, new_name)) {
> +                                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "failed to write new name `%s'"
> +                                           " to %s", new_name, tmp);
> +                                goto x_fail;
> +                            }
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +    }
> +
>      if (stub_dm_domid) {
>          rc = libxl__domain_rename(gc, stub_dm_domid,
>                                    stub_dm_old_name,
> -- 
> 1.8.4.5

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Campbell]

On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> 
> The hard-coded 0 cannot work if you have driver domain.
> 
> At the very least it should be using LIBXL_TOOLSTACK_DOMID.

Does anyone know the purpose of this node, i.e. what consumes it?
docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
xen/include/public/io/*.h AFAICS.

The backend needing to know the name (as opposed to domid) of the
frontend domain seems rather unusual to me.

Perhaps we should just nuke this key?

Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Wei Liu]

On Mon, Nov 17, 2014 at 10:25:31AM +0000, Ian Campbell wrote:
> On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> > 
> > The hard-coded 0 cannot work if you have driver domain.
> > 
> > At the very least it should be using LIBXL_TOOLSTACK_DOMID.
> 
> Does anyone know the purpose of this node, i.e. what consumes it?
> docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
> xen/include/public/io/*.h AFAICS.
> 
> The backend needing to know the name (as opposed to domid) of the
> frontend domain seems rather unusual to me.
> 

I think it's for consistency but Chunyan might have different opinions.

Wei.

> Perhaps we should just nuke this key?
> 
> Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Campbell]

On Mon, 2014-11-17 at 10:41 +0000, Wei Liu wrote:
> On Mon, Nov 17, 2014 at 10:25:31AM +0000, Ian Campbell wrote:
> > On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > > > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> > > 
> > > The hard-coded 0 cannot work if you have driver domain.
> > > 
> > > At the very least it should be using LIBXL_TOOLSTACK_DOMID.
> > 
> > Does anyone know the purpose of this node, i.e. what consumes it?
> > docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
> > xen/include/public/io/*.h AFAICS.
> > 
> > The backend needing to know the name (as opposed to domid) of the
> > frontend domain seems rather unusual to me.
> > 
> 
> I think it's for consistency but Chunyan might have different opinions.

Consistency of what?

> 
> Wei.
> 
> > Perhaps we should just nuke this key?
> > 
> > Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Wei Liu]

On Mon, Nov 17, 2014 at 10:42:45AM +0000, Ian Campbell wrote:
> On Mon, 2014-11-17 at 10:41 +0000, Wei Liu wrote:
> > On Mon, Nov 17, 2014 at 10:25:31AM +0000, Ian Campbell wrote:
> > > On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > > > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > > > > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> > > > 
> > > > The hard-coded 0 cannot work if you have driver domain.
> > > > 
> > > > At the very least it should be using LIBXL_TOOLSTACK_DOMID.
> > > 
> > > Does anyone know the purpose of this node, i.e. what consumes it?
> > > docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
> > > xen/include/public/io/*.h AFAICS.
> > > 
> > > The backend needing to know the name (as opposed to domid) of the
> > > frontend domain seems rather unusual to me.
> > > 
> > 
> > I think it's for consistency but Chunyan might have different opinions.
> 
> Consistency of what?
> 

Say, when you look at backend path, it shows the frontend domain has
name of "blah--incoming" because it's migrated; however, in
/local/domain/$DOMID/ there's a different name "blah".

Wei.

> > 
> > Wei.
> > 
> > > Perhaps we should just nuke this key?
> > > 
> > > Ian.
> 

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Campbell]

On Mon, 2014-11-17 at 10:42 +0000, Ian Campbell wrote:
> On Mon, 2014-11-17 at 10:41 +0000, Wei Liu wrote:
> > On Mon, Nov 17, 2014 at 10:25:31AM +0000, Ian Campbell wrote:
> > > On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > > > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > > > > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> > > > 
> > > > The hard-coded 0 cannot work if you have driver domain.
> > > > 
> > > > At the very least it should be using LIBXL_TOOLSTACK_DOMID.
> > > 
> > > Does anyone know the purpose of this node, i.e. what consumes it?
> > > docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
> > > xen/include/public/io/*.h AFAICS.
> > > 
> > > The backend needing to know the name (as opposed to domid) of the
> > > frontend domain seems rather unusual to me.
> > > 
> > 
> > I think it's for consistency but Chunyan might have different opinions.
> 
> Consistency of what?

Dunno if this is relevant but I appear to have this node only for pci
and console backend directories, and not for vbd or vif.

I quick grep for domain\" in both linux/drivers/xen/xen-pciback and
xen/tools/console seems to suggest neither of them are reading it.

Ian

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Campbell]

On Mon, 2014-11-17 at 10:48 +0000, Wei Liu wrote:
> On Mon, Nov 17, 2014 at 10:42:45AM +0000, Ian Campbell wrote:
> > On Mon, 2014-11-17 at 10:41 +0000, Wei Liu wrote:
> > > On Mon, Nov 17, 2014 at 10:25:31AM +0000, Ian Campbell wrote:
> > > > On Mon, 2014-11-17 at 09:52 +0000, Wei Liu wrote:
> > > > > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> > > > > > +    be_dev = libxl__xs_directory(gc, trans, "/local/domain/0/backend", &ndevs);
> > > > > 
> > > > > The hard-coded 0 cannot work if you have driver domain.
> > > > > 
> > > > > At the very least it should be using LIBXL_TOOLSTACK_DOMID.
> > > > 
> > > > Does anyone know the purpose of this node, i.e. what consumes it?
> > > > docs/misc/xenstore-paths.markdown doesn't mention it, neither do any of
> > > > xen/include/public/io/*.h AFAICS.
> > > > 
> > > > The backend needing to know the name (as opposed to domid) of the
> > > > frontend domain seems rather unusual to me.
> > > > 
> > > 
> > > I think it's for consistency but Chunyan might have different opinions.
> > 
> > Consistency of what?
> > 
> 
> Say, when you look at backend path, it shows the frontend domain has
> name of "blah--incoming" because it's migrated; however, in
> /local/domain/$DOMID/ there's a different name "blah".

I'm suggesting we nuke this key entirely because it is meaningless, not
questioning the need for it to be correct if we keep it.

Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Jackson]

Chunyan Liu writes ("[PATCH] fix rename: xenstore not fully updated"):
> Currently libxl__domain_rename only update /local/domain/<domid>/name,
> still some places in xenstore are not updated, including:
> /vm/<uuid>/name and /local/domain/0/backend/<device>/<domid>/.../domain.

Thanks.

...
> +    /* update /vm/<uuid>/name */
> +    vm_path = libxl__xs_read(gc, trans, libxl__sprintf(gc, "%s/vm", dom_path));

This seems to be obtaining the uuid from xenstore.  Can't we get it
from the hypervisor ?  That avoids quite a bit of this path fiddling.

> +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */

Um, what on earth creates that ?

Worse, what reads it ?

I think that putting this information in the backend directory is
entirely wrong.

(Also, please use GCSPRINTF, libxl__xs_read_checked, etc., and keep
your lines to less than 75 characters or so.)

Thanks,
Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Ian Campbell]

On Mon, 2014-11-17 at 15:45 +0000, Ian Jackson wrote:
> > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */
> 
> Um, what on earth creates that ?
> 
> Worse, what reads it ?
> 
> I think that putting this information in the backend directory is
> entirely wrong.

I concluded the same thing.

Chunyan, I think it would be better to simply remove the code which adds
this domain field under the backend dir in the first place, instead of
adding all this complex code to try and keep it up to date.

Can you do that in the next iteration please?

Thanks,

Ian.

Re: [PATCH] fix rename: xenstore not fully updated

[Chun Yan Liu]

>>> On 11/17/2014 at 11:53 PM, in message <1416239594.5466.23.camel@citrix.com>,
Ian Campbell <Ian.Campbell@citrix.com> wrote: 
> On Mon, 2014-11-17 at 15:45 +0000, Ian Jackson wrote: 
> > > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */ 
> >  
> > Um, what on earth creates that ? 
> >  
> > Worse, what reads it ? 
> >  
> > I think that putting this information in the backend directory is 
> > entirely wrong. 
>  
> I concluded the same thing. 
>  
> Chunyan, I think it would be better to simply remove the code which adds 
> this domain field under the backend dir in the first place, instead of 
> adding all this complex code to try and keep it up to date. 
>  
> Can you do that in the next iteration please? 

Sure. I also doubt why the domain field is added to the backend dir, and only
under some device types, like vkbd, console, etc. under vif and vbd, there is
no such filed. If no one uses that field, I'll remove that from backend dir.

- Chunyan

>  
> Thanks, 
>  
> Ian. 
>  
>  
>  

Re: [PATCH] fix rename: xenstore not fully updated

[Chun Yan Liu]

>>> On 11/17/2014 at 11:45 PM, in message
<21610.6141.943750.141913@mariner.uk.xensource.com>, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote: 
> Chunyan Liu writes ("[PATCH] fix rename: xenstore not fully updated"): 
> > Currently libxl__domain_rename only update /local/domain/<domid>/name, 
> > still some places in xenstore are not updated, including: 
> > /vm/<uuid>/name and /local/domain/0/backend/<device>/<domid>/.../domain. 
>  
> Thanks. 
>  
> ... 
> > +    /* update /vm/<uuid>/name */ 
> > +    vm_path = libxl__xs_read(gc, trans, libxl__sprintf(gc, "%s/vm",  
> dom_path)); 
>  
> This seems to be obtaining the uuid from xenstore.  Can't we get it 
> from the hypervisor ?  That avoids quite a bit of this path fiddling. 

Will update. Thanks.

>  
> > +    /* update backend /local/domain/0/backend/<device>/<domid>/.../domain */ 
>  
> Um, what on earth creates that ? 
>  
> Worse, what reads it ? 
>  
> I think that putting this information in the backend directory is 
> entirely wrong. 
>  
> (Also, please use GCSPRINTF, libxl__xs_read_checked, etc., and keep 
> your lines to less than 75 characters or so.) 
>  
> Thanks, 
> Ian. 
>  
>