[PATCH 0/2 V3] fix rename: xenstore not fully updated

[PATCH 0/2 V3] 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.

This patch series updates /vm/<uuid>/name in xenstore, and removes the unusual
'domain' field under backend directory.

---
Changes to v2:
  * split one patch into two:
    1/2 remove 'domain' field in backend dir;
    2/2 add code to update /vm/<uuid>/name
  * remove all redundant logging.

Chunyan Liu (2):
  remove domain field in xenstore backend dir
  fix rename: xenstore not fully updated

 tools/libxl/libxl.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
1.8.4.5

[PATCH 1/2 V3] remove domain field in xenstore backend dir

[Chunyan Liu]

Remove the unusual 'domain' field under backend directory. The
affected are backend/console, backend/vfb, backend/vkbd.

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f7961f6..dbefaf3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3605,8 +3605,6 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(back, "1");
     flexarray_append(back, "state");
     flexarray_append(back, libxl__sprintf(gc, "%d", 1));
-    flexarray_append(back, "domain");
-    flexarray_append(back, libxl__domid_to_name(gc, domid));
     flexarray_append(back, "protocol");
     flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
 
@@ -3943,8 +3941,6 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(back, "1");
     flexarray_append(back, "state");
     flexarray_append(back, libxl__sprintf(gc, "%d", 1));
-    flexarray_append(back, "domain");
-    flexarray_append(back, libxl__domid_to_name(gc, domid));
 
     flexarray_append(front, "backend-id");
     flexarray_append(front, libxl__sprintf(gc, "%d", vkb->backend_domid));
@@ -4041,7 +4037,6 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb)
     flexarray_append_pair(back, "frontend-id", libxl__sprintf(gc, "%d", domid));
     flexarray_append_pair(back, "online", "1");
     flexarray_append_pair(back, "state", libxl__sprintf(gc, "%d", 1));
-    flexarray_append_pair(back, "domain", libxl__domid_to_name(gc, domid));
     flexarray_append_pair(back, "vnc",
                           libxl_defbool_val(vfb->vnc.enable) ? "1" : "0");
     flexarray_append_pair(back, "vnclisten", vfb->vnc.listen);
-- 
1.8.4.5

Re: [PATCH 1/2 V3] remove domain field in xenstore backend dir

[Ian Jackson]

Chunyan Liu writes ("[PATCH 1/2 V3] remove domain field in xenstore backend dir"):
> Remove the unusual 'domain' field under backend directory. The
> affected are backend/console, backend/vfb, backend/vkbd.

Thanks.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 2/2 V3] fix rename: xenstore not fully updated

[Chunyan Liu]

libxl__domain_rename only updates /local/domain/<domid>/name,
/vm/<uuid>/name in xenstore are not updated. Add code in
libxl__domain_rename to update /vm/<uuid>/name too.

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

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index dbefaf3..b403edc 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;
+    libxl_dominfo info;
+    char *uuid;
+    const char *vm_name_path;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path) goto x_nomem;
@@ -429,6 +432,16 @@ int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
         goto x_fail;
     }
 
+    /* update /vm/<uuid>/name */
+    rc = libxl_domain_info(ctx, &info, domid);
+    if (rc)
+        goto x_fail;
+
+    uuid = GCSPRINTF(LIBXL_UUID_FMT, LIBXL_UUID_BYTES(info.uuid));
+    vm_name_path = GCSPRINTF("/vm/%s/name", uuid);
+    if (libxl__xs_write_checked(gc, trans, vm_name_path, new_name))
+        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 2/2 V3] fix rename: xenstore not fully updated

[Ian Jackson]

Chunyan Liu writes ("[PATCH 2/2 V3] fix rename: xenstore not fully updated"):
> libxl__domain_rename only updates /local/domain/<domid>/name,
> /vm/<uuid>/name in xenstore are not updated. Add code in
> libxl__domain_rename to update /vm/<uuid>/name too.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Ian Jackson]

Hi Konrad, I have another release ack request:

Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> This patch series updates /vm/<uuid>/name in xenstore,

This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
bugfix which I think should go into Xen 4.5.

The risk WITHOUT this patch is that there are out-of-tree tools which
look here for the domain name and will get confused after it is
renamed.

The risk WITH this patch is that the implementation could be wrong
somehow, in which case the code would need to be updated again.  But
it's a very small patch and has been fully reviewed.


> and removes the unusual 'domain' field under backend directory.

This is a reference to "[PATCH 1/2 V3] remove domain field in xenstore
backend dir".  The change to libxl is that it no longer writes
  /local/domain/0/backend/vfb/3/0/domain = "name of frontend domain"

It seems hardly conceivable that anyone could be using this field.
Existing users will not work after the domain is renamed, anyway.

The risk on both sides of the decision lies entirely with out-of-tree
software which looks here for the domain name for some reason.  We
don't think any such tools exist.

Note that the domain name cannot be used directly by a non-dom0
programs because the mapping between domids and domain names is in a
part of xenstore which is not accessible to guests.  (It is possible
that a guest would read this value merely to display it.)


If such out-of-tree software exists:

The risk WITHOUT this patch is that it might report, or (worse)
operate on, the wrong domain entirely.

The risk WITH this patch is that it (or some subset of its
functionality) would stop working right away.


An alternative would be to update all of these entries on rename.
That's a large and somewhat fiddly patch which we don't think is
appropriate given that the presence of this key is a mistake.


Thanks,
ian.

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Konrad Rzeszutek Wilk]

On Wed, Nov 19, 2014 at 11:26:32AM +0000, Ian Jackson wrote:
> Hi Konrad, I have another release ack request:
> 
> Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> > This patch series updates /vm/<uuid>/name in xenstore,
> 
> This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
> bugfix which I think should go into Xen 4.5.
> 
> The risk WITHOUT this patch is that there are out-of-tree tools which
> look here for the domain name and will get confused after it is
> renamed.

When was this introduced? Did it exist with Xend?

> 
> The risk WITH this patch is that the implementation could be wrong
> somehow, in which case the code would need to be updated again.  But
> it's a very small patch and has been fully reviewed.

I checked QEMU and didn't find anything in there.

> 
> 
> > and removes the unusual 'domain' field under backend directory.
> 
> This is a reference to "[PATCH 1/2 V3] remove domain field in xenstore
> backend dir".  The change to libxl is that it no longer writes
>   /local/domain/0/backend/vfb/3/0/domain = "name of frontend domain"
> 
> It seems hardly conceivable that anyone could be using this field.
> Existing users will not work after the domain is renamed, anyway.
> 
> The risk on both sides of the decision lies entirely with out-of-tree
> software which looks here for the domain name for some reason.  We
> don't think any such tools exist.
> 
> Note that the domain name cannot be used directly by a non-dom0
> programs because the mapping between domids and domain names is in a
> part of xenstore which is not accessible to guests.  (It is possible
> that a guest would read this value merely to display it.)
> 
> 
> If such out-of-tree software exists:
> 
> The risk WITHOUT this patch is that it might report, or (worse)
> operate on, the wrong domain entirely.
> 
> The risk WITH this patch is that it (or some subset of its
> functionality) would stop working right away.
> 
> 
> An alternative would be to update all of these entries on rename.
> That's a large and somewhat fiddly patch which we don't think is
> appropriate given that the presence of this key is a mistake.
> 
> 
> Thanks,
> ian.

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Ian Campbell]

On Wed, 2014-11-19 at 16:25 -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 19, 2014 at 11:26:32AM +0000, Ian Jackson wrote:
> > Hi Konrad, I have another release ack request:
> > 
> > Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> > > This patch series updates /vm/<uuid>/name in xenstore,
> > 
> > This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
> > bugfix which I think should go into Xen 4.5.
> > 
> > The risk WITHOUT this patch is that there are out-of-tree tools which
> > look here for the domain name and will get confused after it is
> > renamed.
> 
> When was this introduced? Did it exist with Xend?

Based on:
 git grep domain\" RELEASE-4.4.0  tools/python/
 git grep domain\' RELEASE-4.4.0 tools/python/
it doesn't appear so, but someone with a xend install would be needed to
confirm for sure.

Given that this has always been wrong for a libxl domain after migration
it seems likely to me that noone is looking at this field.
> 
> > 
> > The risk WITH this patch is that the implementation could be wrong
> > somehow, in which case the code would need to be updated again.  But
> > it's a very small patch and has been fully reviewed.
> 
> I checked QEMU and didn't find anything in there.

Great.

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Konrad Rzeszutek Wilk]

On Thu, Nov 20, 2014 at 03:28:30PM +0000, Ian Campbell wrote:
> On Wed, 2014-11-19 at 16:25 -0500, Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 19, 2014 at 11:26:32AM +0000, Ian Jackson wrote:
> > > Hi Konrad, I have another release ack request:
> > > 
> > > Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> > > > This patch series updates /vm/<uuid>/name in xenstore,
> > > 
> > > This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
> > > bugfix which I think should go into Xen 4.5.
> > > 
> > > The risk WITHOUT this patch is that there are out-of-tree tools which
> > > look here for the domain name and will get confused after it is
> > > renamed.
> > 
> > When was this introduced? Did it exist with Xend?
> 
> Based on:
>  git grep domain\" RELEASE-4.4.0  tools/python/
>  git grep domain\' RELEASE-4.4.0 tools/python/
> it doesn't appear so, but someone with a xend install would be needed to
> confirm for sure.
> 
> Given that this has always been wrong for a libxl domain after migration
> it seems likely to me that noone is looking at this field.

And this is not a regression though.

What I am understanding is that we advertise to out-side toolstack
users for a couple of releases something which is misleading and wrong.

My fear is that there such toolstack users out there who will
get their pitchforks out when this shows up.

But perhaps there is a way to mitigate this. Is there another way
(and can it be in the commit description) to get the proper
domain name? I presume it is just looking at /local/domain/%d/name ?

As such could this be added:

"The proper way to get the domain name is to get it from
/local/domain/%d/name instead from this field."

Or such?
 
> > 
> > > 
> > > The risk WITH this patch is that the implementation could be wrong
> > > somehow, in which case the code would need to be updated again.  But
> > > it's a very small patch and has been fully reviewed.
> > 
> > I checked QEMU and didn't find anything in there.
> 
> Great.
> 

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Ian Campbell]

On Fri, 2014-11-21 at 12:01 -0500, Konrad Rzeszutek Wilk wrote:
> On Thu, Nov 20, 2014 at 03:28:30PM +0000, Ian Campbell wrote:
> > On Wed, 2014-11-19 at 16:25 -0500, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Nov 19, 2014 at 11:26:32AM +0000, Ian Jackson wrote:
> > > > Hi Konrad, I have another release ack request:
> > > > 
> > > > Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> > > > > This patch series updates /vm/<uuid>/name in xenstore,
> > > > 
> > > > This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
> > > > bugfix which I think should go into Xen 4.5.
> > > > 
> > > > The risk WITHOUT this patch is that there are out-of-tree tools which
> > > > look here for the domain name and will get confused after it is
> > > > renamed.
> > > 
> > > When was this introduced? Did it exist with Xend?
> > 
> > Based on:
> >  git grep domain\" RELEASE-4.4.0  tools/python/
> >  git grep domain\' RELEASE-4.4.0 tools/python/
> > it doesn't appear so, but someone with a xend install would be needed to
> > confirm for sure.
> > 
> > Given that this has always been wrong for a libxl domain after migration
> > it seems likely to me that noone is looking at this field.
> 
> And this is not a regression though.
> 
> What I am understanding is that we advertise to out-side toolstack
> users for a couple of releases something which is misleading and wrong.

...and also basically useless, there is really no reason to be looking
for the domain's name under a subset of the backend nodes (only vkb, vfb
and console have this key at all). None of the helper function which we
provide do this.

Also these nodes are not advertised as supported either via
docs/misc/xenstore-paths.markdown or xen/include/public/io/*.h.

> My fear is that there such toolstack users out there who will
> get their pitchforks out when this shows up.
> 
> But perhaps there is a way to mitigate this. Is there another way
> (and can it be in the commit description) to get the proper
> domain name? I presume it is just looking at /local/domain/%d/name ?
> 
> As such could this be added:
> 
> "The proper way to get the domain name is to get it from
> /local/domain/%d/name instead from this field."

The proper way is to use libxl_domid_to_name, not to go scrobbling
around in xenstore. We could say this (or both) in the commit message
though if it would help to reassure you.

Either way I think it really would be best to take this fix rather than
worrying overly about the infinitesimal possibility that someone might
be using this key.

Ian.

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Konrad Rzeszutek Wilk]

On Tue, Nov 25, 2014 at 02:13:01PM +0000, Ian Campbell wrote:
> On Fri, 2014-11-21 at 12:01 -0500, Konrad Rzeszutek Wilk wrote:
> > On Thu, Nov 20, 2014 at 03:28:30PM +0000, Ian Campbell wrote:
> > > On Wed, 2014-11-19 at 16:25 -0500, Konrad Rzeszutek Wilk wrote:
> > > > On Wed, Nov 19, 2014 at 11:26:32AM +0000, Ian Jackson wrote:
> > > > > Hi Konrad, I have another release ack request:
> > > > > 
> > > > > Chunyan Liu writes ("[PATCH 0/2 V3] 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.
> > > > > > This patch series updates /vm/<uuid>/name in xenstore,
> > > > > 
> > > > > This ("[PATCH 2/2 V3] fix rename: xenstore not fully updated") is a
> > > > > bugfix which I think should go into Xen 4.5.
> > > > > 
> > > > > The risk WITHOUT this patch is that there are out-of-tree tools which
> > > > > look here for the domain name and will get confused after it is
> > > > > renamed.
> > > > 
> > > > When was this introduced? Did it exist with Xend?
> > > 
> > > Based on:
> > >  git grep domain\" RELEASE-4.4.0  tools/python/
> > >  git grep domain\' RELEASE-4.4.0 tools/python/
> > > it doesn't appear so, but someone with a xend install would be needed to
> > > confirm for sure.
> > > 
> > > Given that this has always been wrong for a libxl domain after migration
> > > it seems likely to me that noone is looking at this field.
> > 
> > And this is not a regression though.
> > 
> > What I am understanding is that we advertise to out-side toolstack
> > users for a couple of releases something which is misleading and wrong.
> 
> ...and also basically useless, there is really no reason to be looking
> for the domain's name under a subset of the backend nodes (only vkb, vfb
> and console have this key at all). None of the helper function which we
> provide do this.
> 
> Also these nodes are not advertised as supported either via
> docs/misc/xenstore-paths.markdown or xen/include/public/io/*.h.
> 
> > My fear is that there such toolstack users out there who will
> > get their pitchforks out when this shows up.
> > 
> > But perhaps there is a way to mitigate this. Is there another way
> > (and can it be in the commit description) to get the proper
> > domain name? I presume it is just looking at /local/domain/%d/name ?
> > 
> > As such could this be added:
> > 
> > "The proper way to get the domain name is to get it from
> > /local/domain/%d/name instead from this field."
> 
> The proper way is to use libxl_domid_to_name, not to go scrobbling
> around in xenstore. We could say this (or both) in the commit message
> though if it would help to reassure you.

That (both) would most certainly reassure me. Thank you!

> 
> Either way I think it really would be best to take this fix rather than
> worrying overly about the infinitesimal possibility that someone might
> be using this key.

Right, so with the reassurance text added on:
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Ian.
> 

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Ian Campbell]

On Tue, 2014-11-25 at 10:51 -0500, Konrad Rzeszutek Wilk wrote:
> Right, so with the reassurance text added on:
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks.

Chunyan, I propose to commit adding the following to the commit text of
"[PATCH 1/2 V3] remove domain field in xenstore backend dir":

        The correct way to obtain a domain's name is via
        libxl_domid_to_name(), or by reading
        from /local/domain/$DOMID/name for toolstacks not using libxl.
        
Does that sound OK to you?

Ian.

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Konrad Rzeszutek Wilk]

On Tue, Nov 25, 2014 at 03:58:33PM +0000, Ian Campbell wrote:
> On Tue, 2014-11-25 at 10:51 -0500, Konrad Rzeszutek Wilk wrote:
> > Right, so with the reassurance text added on:
> > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Thanks.
> 
> Chunyan, I propose to commit adding the following to the commit text of
> "[PATCH 1/2 V3] remove domain field in xenstore backend dir":
> 
>         The correct way to obtain a domain's name is via
>         libxl_domid_to_name(), or by reading
>         from /local/domain/$DOMID/name for toolstacks not using libxl.
>         
> Does that sound OK to you?

Yes.
> 
> Ian.
> 

Re: [PATCH 0/2 V3] fix rename: xenstore not fully updated

[Ian Campbell]

On Tue, 2014-11-25 at 11:03 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 25, 2014 at 03:58:33PM +0000, Ian Campbell wrote:
> > On Tue, 2014-11-25 at 10:51 -0500, Konrad Rzeszutek Wilk wrote:
> > > Right, so with the reassurance text added on:
> > > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > Thanks.
> > 
> > Chunyan, I propose to commit adding the following to the commit text of
> > "[PATCH 1/2 V3] remove domain field in xenstore backend dir":
> > 
> >         The correct way to obtain a domain's name is via
> >         libxl_domid_to_name(), or by reading
> >         from /local/domain/$DOMID/name for toolstacks not using libxl.
> >         
> > Does that sound OK to you?
> 
> Yes.

Thanks, that was really a question for Chunyan, but rather than wait any
longer I've applied with that change.

Ian.