xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches
@ 2016-06-06 10:52 Wei Liu
  2016-06-06 10:52 ` [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary Wei Liu
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

A handful of patches I accumulated during freeze when I was debugging various
issues in xl / libxl.

Wei.

Wei Liu (6):
  xl: remus/colo: only initialise ha variable when necessary
  libxl: add emacs block to libxl_linux.c
  libxl: linux hotplug: clean up get_hotplug_env
  libxl: debug output for args and env when invoking hotplug script
  libxl: rename a field in libxl__domain_create_state
  libxl: log file name in failure in libxl__create_qemu_logfile

 tools/libxl/libxl_create.c   | 32 ++++++++++++++++----------------
 tools/libxl/libxl_device.c   | 25 +++++++++++++++++++++++++
 tools/libxl/libxl_dm.c       |  6 ++++--
 tools/libxl/libxl_internal.h |  2 +-
 tools/libxl/libxl_linux.c    | 23 ++++++++++++++++-------
 tools/libxl/xl_cmdimpl.c     |  6 ++++--
 6 files changed, 66 insertions(+), 28 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:18   ` Ian Jackson
  2016-06-06 10:52 ` [PATCH 2/6] libxl: add emacs block to libxl_linux.c Wei Liu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

The original code is bogus because the common case is no HA enabled.
Setting ha variable at the beginning is not very useful.

Move ha to the scope where it is used.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d8530f0..c33691c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4752,8 +4752,6 @@ static void migrate_receive(int debug, int daemonize, int monitor,
     char rc_buf;
     char *migration_domname;
     struct domain_create dom_info;
-    const char *ha = checkpointed == LIBXL_CHECKPOINTED_STREAM_COLO ?
-                     "COLO" : "Remus";
 
     signal(SIGPIPE, SIG_IGN);
     /* if we get SIGPIPE we'd rather just have it as an error */
@@ -4788,6 +4786,9 @@ static void migrate_receive(int debug, int daemonize, int monitor,
     switch (checkpointed) {
     case LIBXL_CHECKPOINTED_STREAM_REMUS:
     case LIBXL_CHECKPOINTED_STREAM_COLO:
+    {
+        const char *ha = checkpointed == LIBXL_CHECKPOINTED_STREAM_COLO ?
+                         "COLO" : "Remus";
         /* If we are here, it means that the sender (primary) has crashed.
          * TODO: Split-Brain Check.
          */
@@ -4824,6 +4825,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                     ha, common_domname, domid, rc);
 
         exit(rc ? EXIT_FAILURE : EXIT_SUCCESS);
+    }
     default:
         /* do nothing */
         break;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/6] libxl: add emacs block to libxl_linux.c
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
  2016-06-06 10:52 ` [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:19   ` Ian Jackson
  2016-06-06 10:52 ` [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env Wei Liu
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_linux.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 624f028..aed8009 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -300,3 +300,11 @@ int libxl__pci_topology_init(libxl__gc *gc,
 
     return err;
 }
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
  2016-06-06 10:52 ` [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary Wei Liu
  2016-06-06 10:52 ` [PATCH 2/6] libxl: add emacs block to libxl_linux.c Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:20   ` Ian Jackson
  2016-06-06 10:52 ` [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script Wei Liu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

That get_hotplug_env function is called for both block and nic. Move
some nic specific code out of common code to appropriate place.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_linux.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index aed8009..0033a0e 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -39,12 +39,7 @@ static char **get_hotplug_env(libxl__gc *gc,
     const char *type = libxl__device_kind_to_string(dev->backend_kind);
     char *be_path = libxl__device_backend_path(gc, dev);
     char **env;
-    char *gatewaydev;
     int nr = 0;
-    libxl_nic_type nictype;
-
-    gatewaydev = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/%s", be_path,
-                                                        "gatewaydev"));
 
     const int arraysize = 15;
     GCNEW_ARRAY(env, arraysize);
@@ -56,9 +51,15 @@ static char **get_hotplug_env(libxl__gc *gc,
     env[nr++] = GCSPRINTF("backend/%s/%u/%d", type, dev->domid, dev->devid);
     env[nr++] = "XENBUS_BASE_PATH";
     env[nr++] = "backend";
-    env[nr++] = "netdev";
-    env[nr++] = gatewaydev ? : "";
     if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) {
+        libxl_nic_type nictype;
+        char *gatewaydev;
+
+        gatewaydev = libxl__xs_read(gc, XBT_NULL,
+                                    GCSPRINTF("%s/%s", be_path, "gatewaydev"));
+        env[nr++] = "netdev";
+        env[nr++] = gatewaydev ? : "";
+
         if (libxl__nic_type(gc, dev, &nictype)) {
             LOG(ERROR, "unable to get nictype");
             return NULL;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
                   ` (2 preceding siblings ...)
  2016-06-06 10:52 ` [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:26   ` Ian Jackson
  2016-06-06 10:52 ` [PATCH 5/6] libxl: rename a field in libxl__domain_create_state Wei Liu
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_device.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 4717027..b922a94 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1167,6 +1167,31 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
     }
 
     LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
+    LOG(DEBUG, "extra args:");
+    {
+        const char *arg;
+        unsigned int x = 2;
+
+        arg = args[x];
+        while (arg) {
+            LOG(DEBUG, "\t%s", arg);
+            x++;
+            arg = args[x];
+        }
+    }
+    LOG(DEBUG, "env:");
+    {
+        const char *k, *v;
+        unsigned int x = 0;
+
+        k = env[x];
+        while (k) {
+            v = env[x+1];
+            LOG(DEBUG, "\t%s: %s", k, v);
+            x += 2;
+            k = env[x];
+        }
+    }
 
     nullfd = open("/dev/null", O_RDONLY);
     if (nullfd < 0) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/6] libxl: rename a field in libxl__domain_create_state
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
                   ` (3 preceding siblings ...)
  2016-06-06 10:52 ` [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:27   ` Ian Jackson
  2016-06-06 10:52 ` [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile Wei Liu
  2016-06-14 13:10 ` [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

The libxl__stub_dm_spawn_state field in libxl__domain_create_state was
named dmss. That was inconsistent with how things were named (usually
acronym) and there was already libxl__dm_spawn_state named dmss in other
places.

Change dmss to sdss and fix up all sites that reference this field.  No
functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_create.c   | 32 ++++++++++++++++----------------
 tools/libxl/libxl_internal.h |  2 +-
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ca5b167..1b99472 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -903,7 +903,7 @@ static void initiate_domain_create(libxl__egc *egc,
     }
 
     dcs->guest_domid = domid;
-    dcs->dmss.dm.guest_domid = 0; /* means we haven't spawned */
+    dcs->sdss.dm.guest_domid = 0; /* means we haven't spawned */
 
     ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
     if (ret) {
@@ -1050,11 +1050,11 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     /* We might be going to call libxl__spawn_local_dm, or _spawn_stub_dm.
      * Fill in any field required by either, including both relevant
      * callbacks (_spawn_stub_dm will overwrite our trespass if needed). */
-    dcs->dmss.dm.spawn.ao = ao;
-    dcs->dmss.dm.guest_config = dcs->guest_config;
-    dcs->dmss.dm.build_state = &dcs->build_state;
-    dcs->dmss.dm.callback = domcreate_devmodel_started;
-    dcs->dmss.callback = domcreate_devmodel_started;
+    dcs->sdss.dm.spawn.ao = ao;
+    dcs->sdss.dm.guest_config = dcs->guest_config;
+    dcs->sdss.dm.build_state = &dcs->build_state;
+    dcs->sdss.dm.callback = domcreate_devmodel_started;
+    dcs->sdss.callback = domcreate_devmodel_started;
 
     if (restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) {
         rc = libxl__domain_build(gc, d_config, domid, state);
@@ -1344,7 +1344,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
 
         if (d_config->b_info.device_model_version ==
             LIBXL_DEVICE_MODEL_VERSION_NONE) {
-            domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
+            domcreate_devmodel_started(egc, &dcs->sdss.dm, 0);
             return;
         }
 
@@ -1352,11 +1352,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_vkb_add(gc, domid, &vkb);
         libxl_device_vkb_dispose(&vkb);
 
-        dcs->dmss.dm.guest_domid = domid;
+        dcs->sdss.dm.guest_domid = domid;
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
-            libxl__spawn_stub_dm(egc, &dcs->dmss);
+            libxl__spawn_stub_dm(egc, &dcs->sdss);
         else
-            libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+            libxl__spawn_local_dm(egc, &dcs->sdss.dm);
 
         /*
          * Handle the domain's (and the related stubdomain's) access to
@@ -1387,12 +1387,12 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         if (ret < 0)
             goto error_out;
         if (ret) {
-            dcs->dmss.dm.guest_domid = domid;
-            libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+            dcs->sdss.dm.guest_domid = domid;
+            libxl__spawn_local_dm(egc, &dcs->sdss.dm);
             return;
         } else {
-            assert(!dcs->dmss.dm.guest_domid);
-            domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
+            assert(!dcs->sdss.dm.guest_domid);
+            domcreate_devmodel_started(egc, &dcs->sdss.dm, 0);
             return;
         }
     }
@@ -1411,7 +1411,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int ret)
 {
-    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
+    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, sdss.dm);
     STATE_AO_GC(dmss->spawn.ao);
     int domid = dcs->guest_domid;
 
@@ -1423,7 +1423,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
         goto error_out;
     }
 
-    if (dcs->dmss.dm.guest_domid) {
+    if (dcs->sdss.dm.guest_domid) {
         if (d_config->b_info.device_model_version
             == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
             libxl__qmp_initializations(gc, domid, d_config);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ae16c25..6fd7366 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3570,7 +3570,7 @@ struct libxl__domain_create_state {
     libxl__colo_restore_state crs;
     libxl__checkpoint_devices_state cds;
     libxl__bootloader_state bl;
-    libxl__stub_dm_spawn_state dmss;
+    libxl__stub_dm_spawn_state sdss;
         /* If we're not doing stubdom, we use only dmss.dm,
          * for the non-stubdom device model. */
     libxl__stream_read_state srs;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
                   ` (4 preceding siblings ...)
  2016-06-06 10:52 ` [PATCH 5/6] libxl: rename a field in libxl__domain_create_state Wei Liu
@ 2016-06-06 10:52 ` Wei Liu
  2016-06-14 10:28   ` Ian Jackson
  2016-06-14 13:10 ` [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
  6 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-06 10:52 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 155a653..69d2242 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -52,13 +52,15 @@ static int libxl__create_qemu_logfile(libxl__gc *gc, char *name)
     if (rc) return rc;
 
     logfile_w = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
-    free(logfile);
 
     if (logfile_w < 0) {
-        LOGE(ERROR, "unable to open Qemu logfile");
+        LOGE(ERROR, "unable to open Qemu logfile: %s", logfile);
+        free(logfile);
         return ERROR_FAIL;
     }
 
+    free(logfile);
+
     return logfile_w;
 }
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary
  2016-06-06 10:52 ` [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary Wei Liu
@ 2016-06-14 10:18   ` Ian Jackson
  2016-06-14 10:23     ` Wei Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:18 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> The original code is bogus because the common case is no HA enabled.
> Setting ha variable at the beginning is not very useful.

This seems like a plausible cleanup but I don't understand why you say
the original code is `bogus'.  Is it wrong ?  If so you don't say how.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] libxl: add emacs block to libxl_linux.c
  2016-06-06 10:52 ` [PATCH 2/6] libxl: add emacs block to libxl_linux.c Wei Liu
@ 2016-06-14 10:19   ` Ian Jackson
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:19 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("[PATCH 2/6] libxl: add emacs block to libxl_linux.c"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env
  2016-06-06 10:52 ` [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env Wei Liu
@ 2016-06-14 10:20   ` Ian Jackson
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:20 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("[PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env"):
> That get_hotplug_env function is called for both block and nic. Move
> some nic specific code out of common code to appropriate place.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary
  2016-06-14 10:18   ` Ian Jackson
@ 2016-06-14 10:23     ` Wei Liu
  2016-06-14 14:15       ` Ian Jackson
  0 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-14 10:23 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Wei Liu

On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> > The original code is bogus because the common case is no HA enabled.
> > Setting ha variable at the beginning is not very useful.
> 
> This seems like a plausible cleanup but I don't understand why you say
> the original code is `bogus'.  Is it wrong ?  If so you don't say how.
> 

What I meant is "the default state should be no HA enabled", hence if we
don't move the ha variable into a narrower scope it should have been
initialised to NULL first.  It is not a bug though.

Wei.

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-06-06 10:52 ` [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script Wei Liu
@ 2016-06-14 10:26   ` Ian Jackson
  2016-06-14 12:46     ` Wei Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:26 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("[PATCH 4/6] libxl: debug output for args and env when invoking hotplug script"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
...
> +        const char *arg;
> +        unsigned int x = 2;
> +
> +        arg = args[x];
> +        while (arg) {
> +            LOG(DEBUG, "\t%s", arg);
> +            x++;
> +            arg = args[x];
> +        }

What a strange way to write

   for (x=2; (arg = args[x]); x++) {

or

   for (x=2; (arg = args[x++]); ) {

or

   x = 2;
   while ((arg = args[x++])) {

If you really insist on not doing assignment in the conditional (which
IMO is a very usual C idiom) then you should avoid the repeated code
with

   x = 2;
   for (;;) {
      arg = args[x++];
      if (!arg) break;

or some such.

> +        const char *k, *v;
> +        unsigned int x = 0;
> +
> +        k = env[x];
> +        while (k) {
> +            v = env[x+1];
> +            LOG(DEBUG, "\t%s: %s", k, v);
> +            x += 2;
> +            k = env[x];
> +        }

How about one of

   for (x=0; (k = env[x]); x += 2) {
       v = env[x+1];

   for (x=0; (k = env[x]) && (v = env[x+1]); x += 2) {

   for (x=0; (k = env[x++]) && (v = env[x++]); ) {

   x = 0;
   while ((k = env[x++])) {
       v = env[x++];
       assert(v);

?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/6] libxl: rename a field in libxl__domain_create_state
  2016-06-06 10:52 ` [PATCH 5/6] libxl: rename a field in libxl__domain_create_state Wei Liu
@ 2016-06-14 10:27   ` Ian Jackson
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:27 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("[PATCH 5/6] libxl: rename a field in libxl__domain_create_state"):
> The libxl__stub_dm_spawn_state field in libxl__domain_create_state was
> named dmss. That was inconsistent with how things were named (usually
> acronym) and there was already libxl__dm_spawn_state named dmss in other
> places.
> 
> Change dmss to sdss and fix up all sites that reference this field.  No
> functional change.

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

(without really reading the patch....)

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile
  2016-06-06 10:52 ` [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile Wei Liu
@ 2016-06-14 10:28   ` Ian Jackson
  0 siblings, 0 replies; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 10:28 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel, Ian Jackson

Wei Liu writes ("[PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

It would be nice to fix the error handling pattern to `goto out'
style, but this patch is an improvement.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-06-14 10:26   ` Ian Jackson
@ 2016-06-14 12:46     ` Wei Liu
  2016-07-02 10:21       ` Wei Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-06-14 12:46 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Wei Liu

On Tue, Jun 14, 2016 at 11:26:56AM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 4/6] libxl: debug output for args and env when invoking hotplug script"):
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ...
> > +        const char *arg;
> > +        unsigned int x = 2;
> > +
> > +        arg = args[x];
> > +        while (arg) {
> > +            LOG(DEBUG, "\t%s", arg);
> > +            x++;
> > +            arg = args[x];
> > +        }

I will use

> 
> What a strange way to write
> 
>    for (x=2; (arg = args[x]); x++) {
> 

This style.

> or
> 
>    for (x=2; (arg = args[x++]); ) {
> 
> or
> 
>    x = 2;
>    while ((arg = args[x++])) {
> 
> If you really insist on not doing assignment in the conditional (which
> IMO is a very usual C idiom) then you should avoid the repeated code
> with
> 
>    x = 2;
>    for (;;) {
>       arg = args[x++];
>       if (!arg) break;
> 
> or some such.
> 
> > +        const char *k, *v;
> > +        unsigned int x = 0;
> > +
> > +        k = env[x];
> > +        while (k) {
> > +            v = env[x+1];
> > +            LOG(DEBUG, "\t%s: %s", k, v);
> > +            x += 2;
> > +            k = env[x];
> > +        }
> 
> How about one of
> 
>    for (x=0; (k = env[x]); x += 2) {
>        v = env[x+1];
> 

And this style.

---8<---
From 49714976c5fde3d08baa6f34295b3f7db6a81444 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Fri, 15 Apr 2016 12:56:03 +0100
Subject: [PATCH] libxl: debug output for args and env when invoking hotplug
 script

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v2: write the loops differently.
---
 tools/libxl/libxl_device.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 4717027..b3213be 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1167,6 +1167,24 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
     }
 
     LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
+    LOG(DEBUG, "extra args:");
+    {
+        const char *arg;
+        unsigned int x;
+
+        for (x = 2; (arg = args[x]); x++)
+            LOG(DEBUG, "\t%s", arg);
+    }
+    LOG(DEBUG, "env:");
+    {
+        const char *k, *v;
+        unsigned int x;
+
+        for (x = 0; (k = env[x]); x += 2) {
+            v = env[x+1];
+            LOG(DEBUG, "\t%s: %s", k, v);
+        }
+    }
 
     nullfd = open("/dev/null", O_RDONLY);
     if (nullfd < 0) {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches
  2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
                   ` (5 preceding siblings ...)
  2016-06-06 10:52 ` [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile Wei Liu
@ 2016-06-14 13:10 ` Wei Liu
  6 siblings, 0 replies; 21+ messages in thread
From: Wei Liu @ 2016-06-14 13:10 UTC (permalink / raw)
  To: Xen-devel; +Cc: Wei Liu, Ian Jackson

On Mon, Jun 06, 2016 at 11:52:06AM +0100, Wei Liu wrote:
> A handful of patches I accumulated during freeze when I was debugging various
> issues in xl / libxl.
> 
> Wei.
> 
> Wei Liu (6):
>   xl: remus/colo: only initialise ha variable when necessary
>   libxl: add emacs block to libxl_linux.c
>   libxl: linux hotplug: clean up get_hotplug_env
>   libxl: debug output for args and env when invoking hotplug script
>   libxl: rename a field in libxl__domain_create_state
>   libxl: log file name in failure in libxl__create_qemu_logfile
> 

I've queued up all the acked patches (2, 3, 5 and 6) for committing.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary
  2016-06-14 10:23     ` Wei Liu
@ 2016-06-14 14:15       ` Ian Jackson
  2016-06-14 14:18         ` Wei Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2016-06-14 14:15 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> > > The original code is bogus because the common case is no HA enabled.
> > > Setting ha variable at the beginning is not very useful.
> > 
> > This seems like a plausible cleanup but I don't understand why you say
> > the original code is `bogus'.  Is it wrong ?  If so you don't say how.
> 
> What I meant is "the default state should be no HA enabled", hence if we
> don't move the ha variable into a narrower scope it should have been
> initialised to NULL first.  It is not a bug though.

IC.  Perhaps you'd like to clarify the commit message ?
Whether or not you do,

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary
  2016-06-14 14:15       ` Ian Jackson
@ 2016-06-14 14:18         ` Wei Liu
  0 siblings, 0 replies; 21+ messages in thread
From: Wei Liu @ 2016-06-14 14:18 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Wei Liu

On Tue, Jun 14, 2016 at 03:15:19PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> > On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote:
> > > Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> > > > The original code is bogus because the common case is no HA enabled.
> > > > Setting ha variable at the beginning is not very useful.
> > > 
> > > This seems like a plausible cleanup but I don't understand why you say
> > > the original code is `bogus'.  Is it wrong ?  If so you don't say how.
> > 
> > What I meant is "the default state should be no HA enabled", hence if we
> > don't move the ha variable into a narrower scope it should have been
> > initialised to NULL first.  It is not a bug though.
> 
> IC.  Perhaps you'd like to clarify the commit message ?

Sure, I will incorporate my reply above to the commit message.

> Whether or not you do,
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-06-14 12:46     ` Wei Liu
@ 2016-07-02 10:21       ` Wei Liu
  2016-07-04 17:06         ` Ian Jackson
  0 siblings, 1 reply; 21+ messages in thread
From: Wei Liu @ 2016-07-02 10:21 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Wei Liu

> From 49714976c5fde3d08baa6f34295b3f7db6a81444 Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Fri, 15 Apr 2016 12:56:03 +0100
> Subject: [PATCH] libxl: debug output for args and env when invoking hotplug
>  script
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Ian, Ping?

> ---
> v2: write the loops differently.
> ---
>  tools/libxl/libxl_device.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 4717027..b3213be 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1167,6 +1167,24 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>      }
>  
>      LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
> +    LOG(DEBUG, "extra args:");
> +    {
> +        const char *arg;
> +        unsigned int x;
> +
> +        for (x = 2; (arg = args[x]); x++)
> +            LOG(DEBUG, "\t%s", arg);
> +    }
> +    LOG(DEBUG, "env:");
> +    {
> +        const char *k, *v;
> +        unsigned int x;
> +
> +        for (x = 0; (k = env[x]); x += 2) {
> +            v = env[x+1];
> +            LOG(DEBUG, "\t%s: %s", k, v);
> +        }
> +    }
>  
>      nullfd = open("/dev/null", O_RDONLY);
>      if (nullfd < 0) {
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-07-02 10:21       ` Wei Liu
@ 2016-07-04 17:06         ` Ian Jackson
  2016-07-06 17:23           ` Wei Liu
  0 siblings, 1 reply; 21+ messages in thread
From: Ian Jackson @ 2016-07-04 17:06 UTC (permalink / raw)
  To: Wei Liu; +Cc: Xen-devel

Wei Liu writes ("Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script"):
> > From 49714976c5fde3d08baa6f34295b3f7db6a81444 Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Fri, 15 Apr 2016 12:56:03 +0100
> > Subject: [PATCH] libxl: debug output for args and env when invoking hotplug
> >  script
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Ian, Ping?

Sorry, yes.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script
  2016-07-04 17:06         ` Ian Jackson
@ 2016-07-06 17:23           ` Wei Liu
  0 siblings, 0 replies; 21+ messages in thread
From: Wei Liu @ 2016-07-06 17:23 UTC (permalink / raw)
  To: Ian Jackson; +Cc: Xen-devel, Wei Liu

On Mon, Jul 04, 2016 at 06:06:27PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script"):
> > > From 49714976c5fde3d08baa6f34295b3f7db6a81444 Mon Sep 17 00:00:00 2001
> > > From: Wei Liu <wei.liu2@citrix.com>
> > > Date: Fri, 15 Apr 2016 12:56:03 +0100
> > > Subject: [PATCH] libxl: debug output for args and env when invoking hotplug
> > >  script
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Ian, Ping?
> 
> Sorry, yes.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Queued. Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2016-07-06 17:23 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-06 10:52 [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu
2016-06-06 10:52 ` [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary Wei Liu
2016-06-14 10:18   ` Ian Jackson
2016-06-14 10:23     ` Wei Liu
2016-06-14 14:15       ` Ian Jackson
2016-06-14 14:18         ` Wei Liu
2016-06-06 10:52 ` [PATCH 2/6] libxl: add emacs block to libxl_linux.c Wei Liu
2016-06-14 10:19   ` Ian Jackson
2016-06-06 10:52 ` [PATCH 3/6] libxl: linux hotplug: clean up get_hotplug_env Wei Liu
2016-06-14 10:20   ` Ian Jackson
2016-06-06 10:52 ` [PATCH 4/6] libxl: debug output for args and env when invoking hotplug script Wei Liu
2016-06-14 10:26   ` Ian Jackson
2016-06-14 12:46     ` Wei Liu
2016-07-02 10:21       ` Wei Liu
2016-07-04 17:06         ` Ian Jackson
2016-07-06 17:23           ` Wei Liu
2016-06-06 10:52 ` [PATCH 5/6] libxl: rename a field in libxl__domain_create_state Wei Liu
2016-06-14 10:27   ` Ian Jackson
2016-06-06 10:52 ` [PATCH 6/6] libxl: log file name in failure in libxl__create_qemu_logfile Wei Liu
2016-06-14 10:28   ` Ian Jackson
2016-06-14 13:10 ` [PATCH 0/6] xl/libxl: some cleanup / debugging aid patches Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).