[PATCH] qemu-trad: xenstore: use relative path for device-model node

[PATCH] qemu-trad: xenstore: use relative path for device-model node

[Wei Liu]

For QEMU traditional stubdom, this is incompatible startup protocol
change.  This change needs to work with corresponding libxl changeset.
QEMU traditional is shipped with Xen so we are allowed to do such
change.

For QEMU traditional running in Dom0, there is no functional change
because it will still write to the same /local/domain/0 path.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xenstore.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index b0d6f77..09319c7 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 
 
     /* Set a watch for log-dirty commands from the migration tools */
-    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
+    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
                   domid) != -1) {
         xs_watch(xsh, buf, "logdirty");
         fprintf(logfile, "Watching %s\n", buf);
@@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 
     /* Set a watch for suspend requests from the migration tools */
     if (pasprintf(&buf, 
-                  "/local/domain/0/device-model/%u/command", domid) != -1) {
+                  "device-model/%u/command", domid) != -1) {
         xs_watch(xsh, buf, "dm-command");
         fprintf(logfile, "Watching %s\n", buf);
     }
@@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
     int disable_pf = 0;
     unsigned int len;
 
-    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
+    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
         goto out;
 
     params = xs_read(xsh, XBT_NULL, buf, &len);
@@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
 
     /* Remember the paths for the command and response entries */
     if (pasprintf(&ret_path,
-                "/local/domain/0/device-model/%u/logdirty/ret",
+                "device-model/%u/logdirty/ret",
                 domid) == -1) {
         fprintf(logfile, "Log-dirty: out of memory\n");
         exit(1);
     }
     if (pasprintf(&cmd_path,
-                "/local/domain/0/device-model/%u/logdirty/cmd",
+                "device-model/%u/logdirty/cmd",
                 domid) == -1) {
         fprintf(logfile, "Log-dirty: out of memory\n");
         exit(1);
@@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
     unsigned int len;
 
     if (pasprintf(&path, 
-                  "/local/domain/0/device-model/%u/command", domid) == -1) {
+                  "device-model/%u/command", domid) == -1) {
         fprintf(logfile, "out of memory reading dm command\n");
         goto out;
     }
@@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
     } else if (!strncmp(command, "usb-add", len)) {
         fprintf(logfile, "dm-command: usb-add a usb device\n");
         if (pasprintf(&path,
-                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
     } else if (!strncmp(command, "usb-del", len)) {
         fprintf(logfile, "dm-command: usb-del a usb device\n");
         if (pasprintf(&path,
-                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
         fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
 
         if (pasprintf(&path, 
-                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                      "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
         fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
 
         if (pasprintf(&path, 
-                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                      "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
     char *path = NULL;
 
     if (pasprintf(&path, 
-                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
+                  "device-model/%u/%s", domid, subpath) == -1) {
         fprintf(logfile, "out of memory recording dm \n");
         goto out;
     }
@@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
 {
     char *path = NULL, *value = NULL;
 
-    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
+    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
         return NULL;
 
     value = xs_read(xsh, XBT_NULL, path, len);
-- 
1.9.1

Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node

[Andrew Cooper]

On 30/03/15 13:37, Wei Liu wrote:
> For QEMU traditional stubdom, this is incompatible startup protocol
> change.  This change needs to work with corresponding libxl changeset.
> QEMU traditional is shipped with Xen so we are allowed to do such
> change.
>
> For QEMU traditional running in Dom0, there is no functional change
> because it will still write to the same /local/domain/0 path.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

For the change, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

However, it would be nice if you also unsplit the pasprintf() calls.  It
looks as if all of them should fit on a single line now.

~Andrew

> ---
>  xenstore.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/xenstore.c b/xenstore.c
> index b0d6f77..09319c7 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>  
>      /* Set a watch for log-dirty commands from the migration tools */
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
> +    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
>                    domid) != -1) {
>          xs_watch(xsh, buf, "logdirty");
>          fprintf(logfile, "Watching %s\n", buf);
> @@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>      /* Set a watch for suspend requests from the migration tools */
>      if (pasprintf(&buf, 
> -                  "/local/domain/0/device-model/%u/command", domid) != -1) {
> +                  "device-model/%u/command", domid) != -1) {
>          xs_watch(xsh, buf, "dm-command");
>          fprintf(logfile, "Watching %s\n", buf);
>      }
> @@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
>      int disable_pf = 0;
>      unsigned int len;
>  
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
> +    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
>          goto out;
>  
>      params = xs_read(xsh, XBT_NULL, buf, &len);
> @@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
>  
>      /* Remember the paths for the command and response entries */
>      if (pasprintf(&ret_path,
> -                "/local/domain/0/device-model/%u/logdirty/ret",
> +                "device-model/%u/logdirty/ret",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
>      }
>      if (pasprintf(&cmd_path,
> -                "/local/domain/0/device-model/%u/logdirty/cmd",
> +                "device-model/%u/logdirty/cmd",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
> @@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
>      unsigned int len;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/command", domid) == -1) {
> +                  "device-model/%u/command", domid) == -1) {
>          fprintf(logfile, "out of memory reading dm command\n");
>          goto out;
>      }
> @@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-add", len)) {
>          fprintf(logfile, "dm-command: usb-add a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-del", len)) {
>          fprintf(logfile, "dm-command: usb-del a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
>      char *path = NULL;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
> +                  "device-model/%u/%s", domid, subpath) == -1) {
>          fprintf(logfile, "out of memory recording dm \n");
>          goto out;
>      }
> @@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
>  {
>      char *path = NULL, *value = NULL;
>  
> -    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
> +    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
>          return NULL;
>  
>      value = xs_read(xsh, XBT_NULL, path, len);

[PATCH] qemu-trad: xenstore: use relative path for device-model node

[Ian Jackson]

Wei Liu writes ("[PATCH] qemu-trad: xenstore: use relative path for device-model node"):
> For QEMU traditional stubdom, this is incompatible startup protocol
> change.  This change needs to work with corresponding libxl changeset.
> QEMU traditional is shipped with Xen so we are allowed to do such
> change.
> 
> For QEMU traditional running in Dom0, there is no functional change
> because it will still write to the same /local/domain/0 path.

I think you mean:

  For QEMU traditional running in Dom0, there is no functional change
  because it will still write to the same /local/domain/0 path.

  For QEMU traditional stubdom, this is an incompatible startup
  protocol change.  There is a corresponding libxl changeset "libxl:
  use new QEMU xenstore protocol", which has not yet been committed
  (54c2e621 was reverted in 84066dd4).

  QEMU traditional stubdom was broken by #### and is still broken in
  -unstable, so this incompatible change is not a regression.

where #### is some commit I haven't been able to easily identify.

Is that right ?

Thanks,
Ian.

Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node

[Wei Liu]

On Thu, Apr 09, 2015 at 05:47:08PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] qemu-trad: xenstore: use relative path for device-model node"):
> > For QEMU traditional stubdom, this is incompatible startup protocol
> > change.  This change needs to work with corresponding libxl changeset.
> > QEMU traditional is shipped with Xen so we are allowed to do such
> > change.
> > 
> > For QEMU traditional running in Dom0, there is no functional change
> > because it will still write to the same /local/domain/0 path.
> 
> I think you mean:
> 
>   For QEMU traditional running in Dom0, there is no functional change
>   because it will still write to the same /local/domain/0 path.
> 
>   For QEMU traditional stubdom, this is an incompatible startup
>   protocol change.  There is a corresponding libxl changeset "libxl:
>   use new QEMU xenstore protocol", which has not yet been committed
>   (54c2e621 was reverted in 84066dd4).
> 

So far so good.

>   QEMU traditional stubdom was broken by #### and is still broken in
>   -unstable, so this incompatible change is not a regression.
> 

It's complicated.

QEMU traditional stubdom was broken by a0731cca "ioreq-server: on-demand
creation of ioreq server" in 4.5. Currently there is a workaround in
-unstable dd748d12 "x86/hvm: wait for at least one ioreq server to be
enabled" (which should be backported to 4.5). QEMU traditional stubdom
works with that workaround in -unstable but it's not ideal situation.

This incompatible change is not a regression because we don't change the
protocol 4.5 uses. We will only use the new protocol for -unstable.

Wei.

> where #### is some commit I haven't been able to easily identify.
> 
> Is that right ?
> 
> Thanks,
> Ian.

Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node"):
> On Thu, Apr 09, 2015 at 05:47:08PM +0100, Ian Jackson wrote:
> > 
> > I think you mean:
> > ...
> 
> So far so good.
> 
> >   QEMU traditional stubdom was broken by #### and is still broken in
> >   -unstable, so this incompatible change is not a regression.
> > 
> 
> It's complicated.
> 
> QEMU traditional stubdom was broken by a0731cca "ioreq-server: on-demand
> creation of ioreq server" in 4.5. Currently there is a workaround in
> -unstable dd748d12 "x86/hvm: wait for at least one ioreq server to be
> enabled" (which should be backported to 4.5). QEMU traditional stubdom
> works with that workaround in -unstable but it's not ideal situation.

Right.  So that means that this patch needs to go in at the same time
as the corresponding libxl change.

> This incompatible change is not a regression because we don't change the
> protocol 4.5 uses. We will only use the new protocol for -unstable.

I meant, is it a regression in -unstable from earlier -unstable ?

And the answer is that unless both libxl and qemu change at the same
time, it would be a regression in -unstable ?

Ian.

Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node

[Wei Liu]

On Thu, Apr 09, 2015 at 06:46:31PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node"):
> > On Thu, Apr 09, 2015 at 05:47:08PM +0100, Ian Jackson wrote:
> > > 
> > > I think you mean:
> > > ...
> > 
> > So far so good.
> > 
> > >   QEMU traditional stubdom was broken by #### and is still broken in
> > >   -unstable, so this incompatible change is not a regression.
> > > 
> > 
> > It's complicated.
> > 
> > QEMU traditional stubdom was broken by a0731cca "ioreq-server: on-demand
> > creation of ioreq server" in 4.5. Currently there is a workaround in
> > -unstable dd748d12 "x86/hvm: wait for at least one ioreq server to be
> > enabled" (which should be backported to 4.5). QEMU traditional stubdom
> > works with that workaround in -unstable but it's not ideal situation.
> 
> Right.  So that means that this patch needs to go in at the same time
> as the corresponding libxl change.
> 

I don't follow "go in at the same time". They are in two different
trees, don't they?

> > This incompatible change is not a regression because we don't change the
> > protocol 4.5 uses. We will only use the new protocol for -unstable.
> 
> I meant, is it a regression in -unstable from earlier -unstable ?
> 
> And the answer is that unless both libxl and qemu change at the same
> time, it would be a regression in -unstable ?
> 

It would be a regression because stubdom in -unstable is working now
with Paul's workaround. So yes, both changes need to go in at the same
time -- though I don't know how you would do that.

Wei.

> Ian.

Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] qemu-trad: xenstore: use relative path for device-model node"):
> On Thu, Apr 09, 2015 at 06:46:31PM +0100, Ian Jackson wrote:
> > Right.  So that means that this patch needs to go in at the same time
> > as the corresponding libxl change.
> 
> I don't follow "go in at the same time". They are in two different
> trees, don't they?

The commit id of the qemu-trad tree is in Config.mk in xen.git.  So it
is possible to update them "simultaneously".  (Of course not every way
of building and deploying Xen will honour this, but if you don't
honour it you deserve what you get.)

> > And the answer is that unless both libxl and qemu change at the same
> > time, it would be a regression in -unstable ?
> 
> It would be a regression because stubdom in -unstable is working now
> with Paul's workaround. So yes, both changes need to go in at the same
> time -- though I don't know how you would do that.

Right.  That's what the Config.mk update is for.

So if the libxl patch is otherwise ready, we can commit both at once.
I will commit and push to qemu-trad, update the libxl patch to contain
the Config.mk update as well, and push the result to xen.git.

We normally explain the need to do this in the commit message for the
patches, and cross reference the two commits.

Ian.