[PATCH 0 of 6 RESENT] A bunch of fixes for libxl

[PATCH 0 of 6 RESENT] A bunch of fixes for libxl

[Marek Marczykowski]

A bunch of simple fixes for libxl. Most of them fixes SEGV or other annoying
bugs.

This time for xen-unstable tree - only part of previous patches are applicable.
All of them should be also backported to xen-4.1-testing tree (bugfixes).

[PATCH 1 of 6 RESENT] libxl: Remove frontend and backend devices from xenstore after destroy

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307143993 -7200
# Node ID c32797243a6ba61dd2942a0307151e42fb7bf157
# Parent  37c77bacb52aa7795978b994f9d371b979b2cb07
libxl: Remove frontend and backend devices from xenstore after destroy

Cleanup frontend and backend devices from xenstore for all dev types - not only
disks. Because backend cleanup moved to libxl__device_destroy,
libxl__devices_destroy is somehow simpler.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r 37c77bacb52a -r c32797243a6b tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Mon May 23 17:38:28 2011 +0100
+++ b/tools/libxl/libxl.c	Sat Jun 04 01:33:13 2011 +0200
@@ -1105,8 +1105,6 @@ int libxl_device_disk_del(libxl_ctx *ctx
     device.devid            = devid;
     device.kind             = DEVICE_VBD;
     rc = libxl__device_del(&gc, &device, wait);
-    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device));
-    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device));
 out_free:
     libxl__free_all(&gc);
     return rc;
diff -r 37c77bacb52a -r c32797243a6b tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Mon May 23 17:38:28 2011 +0100
+++ b/tools/libxl/libxl_device.c	Sat Jun 04 01:33:13 2011 +0200
@@ -272,6 +272,8 @@ retry_transaction:
     if (!force) {
         xs_watch(ctx->xsh, state_path, be_path);
         rc = 1;
+    } else {
+        xs_rm(ctx->xsh, XBT_NULL, be_path);
     }
 out:
     return rc;
@@ -311,10 +313,8 @@ int libxl__devices_destroy(libxl__gc *gc
     char *path, *be_path, *fe_path;
     unsigned int num1, num2;
     char **l1 = NULL, **l2 = NULL;
-    int i, j, n = 0, n_watches = 0;
-    flexarray_t *toremove;
+    int i, j, n_watches = 0;
 
-    toremove = flexarray_make(16, 1);
     path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
     l1 = libxl__xs_directory(gc, XBT_NULL, path, &num1);
     if (!l1) {
@@ -338,7 +338,6 @@ int libxl__devices_destroy(libxl__gc *gc
             if (be_path != NULL) {
                 if (libxl__device_destroy(gc, be_path, force) > 0)
                     n_watches++;
-                flexarray_set(toremove, n++, libxl__dirname(gc, be_path));
             } else {
                 xs_rm(ctx->xsh, XBT_NULL, path);
             }
@@ -351,7 +350,6 @@ int libxl__devices_destroy(libxl__gc *gc
     if (be_path && strcmp(be_path, "")) {
         if (libxl__device_destroy(gc, be_path, force) > 0)
             n_watches++;
-        flexarray_set(toremove, n++, libxl__dirname(gc, be_path));
     }
 
     if (!force) {
@@ -371,17 +369,13 @@ int libxl__devices_destroy(libxl__gc *gc
             }
         }
     }
-    for (i = 0; i < n; i++) {
-        flexarray_get(toremove, i, (void**) &path);
-        xs_rm(ctx->xsh, XBT_NULL, path);
-    }
 out:
-    flexarray_free(toremove);
     return 0;
 }
 
 int libxl__device_del(libxl__gc *gc, libxl__device *dev, int wait)
 {
+    libxl_ctx *ctx = libxl__gc_owner(gc);
     char *backend_path;
     int rc;
 
@@ -400,6 +394,7 @@ int libxl__device_del(libxl__gc *gc, lib
         (void)wait_for_dev_destroy(gc, &tv);
     }
 
+    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(gc, dev));
     rc = 0;
 
 out:

Re: [PATCH 1 of 6 RESENT] libxl: Remove frontend and backend devices from xenstore after destroy

[Ian Campbell]

On Sun, 2011-06-05 at 17:50 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1307143993 -7200
> # Node ID c32797243a6ba61dd2942a0307151e42fb7bf157
> # Parent  37c77bacb52aa7795978b994f9d371b979b2cb07
> libxl: Remove frontend and backend devices from xenstore after destroy
> 
> Cleanup frontend and backend devices from xenstore for all dev types - not only
> disks. Because backend cleanup moved to libxl__device_destroy,
> libxl__devices_destroy is somehow simpler.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r 37c77bacb52a -r c32797243a6b tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Mon May 23 17:38:28 2011 +0100
> +++ b/tools/libxl/libxl.c	Sat Jun 04 01:33:13 2011 +0200
> @@ -1105,8 +1105,6 @@ int libxl_device_disk_del(libxl_ctx *ctx
>      device.devid            = devid;
>      device.kind             = DEVICE_VBD;
>      rc = libxl__device_del(&gc, &device, wait);
> -    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, &device));
> -    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, &device));
>  out_free:
>      libxl__free_all(&gc);
>      return rc;
> diff -r 37c77bacb52a -r c32797243a6b tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Mon May 23 17:38:28 2011 +0100
> +++ b/tools/libxl/libxl_device.c	Sat Jun 04 01:33:13 2011 +0200
> @@ -272,6 +272,8 @@ retry_transaction:
>      if (!force) {
>          xs_watch(ctx->xsh, state_path, be_path);
>          rc = 1;
> +    } else {
> +        xs_rm(ctx->xsh, XBT_NULL, be_path);
>      }
>  out:
>      return rc;
> @@ -311,10 +313,8 @@ int libxl__devices_destroy(libxl__gc *gc
>      char *path, *be_path, *fe_path;
>      unsigned int num1, num2;
>      char **l1 = NULL, **l2 = NULL;
> -    int i, j, n = 0, n_watches = 0;
> -    flexarray_t *toremove;
> +    int i, j, n_watches = 0;
>  
> -    toremove = flexarray_make(16, 1);
>      path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
>      l1 = libxl__xs_directory(gc, XBT_NULL, path, &num1);
>      if (!l1) {
> @@ -338,7 +338,6 @@ int libxl__devices_destroy(libxl__gc *gc
>              if (be_path != NULL) {
>                  if (libxl__device_destroy(gc, be_path, force) > 0)
>                      n_watches++;
> -                flexarray_set(toremove, n++, libxl__dirname(gc, be_path));
>              } else {
>                  xs_rm(ctx->xsh, XBT_NULL, path);
>              }
> @@ -351,7 +350,6 @@ int libxl__devices_destroy(libxl__gc *gc
>      if (be_path && strcmp(be_path, "")) {
>          if (libxl__device_destroy(gc, be_path, force) > 0)
>              n_watches++;
> -        flexarray_set(toremove, n++, libxl__dirname(gc, be_path));
>      }
>  
>      if (!force) {
> @@ -371,17 +369,13 @@ int libxl__devices_destroy(libxl__gc *gc
>              }
>          }
>      }
> -    for (i = 0; i < n; i++) {
> -        flexarray_get(toremove, i, (void**) &path);
> -        xs_rm(ctx->xsh, XBT_NULL, path);
> -    }
>  out:
> -    flexarray_free(toremove);
>      return 0;
>  }
>  
>  int libxl__device_del(libxl__gc *gc, libxl__device *dev, int wait)
>  {
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>      char *backend_path;
>      int rc;
>  
> @@ -400,6 +394,7 @@ int libxl__device_del(libxl__gc *gc, lib
>          (void)wait_for_dev_destroy(gc, &tv);
>      }
>  
> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(gc, dev));
>      rc = 0;
>  
>  out:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 1 of 6 RESENT] libxl: Remove frontend and backend devices from xenstore after destroy

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH 1 of 6 RESENT] libxl: Remove frontend and backend devices from xenstore after destroy"):
> On Sun, 2011-06-05 at 17:50 +0100, Marek Marczykowski wrote:
> > # HG changeset patch
> > # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> > # Date 1307143993 -7200
> > # Node ID c32797243a6ba61dd2942a0307151e42fb7bf157
> > # Parent  37c77bacb52aa7795978b994f9d371b979b2cb07
> > libxl: Remove frontend and backend devices from xenstore after destroy
> > 
> > Cleanup frontend and backend devices from xenstore for all dev types - not only
> > disks. Because backend cleanup moved to libxl__device_destroy,
> > libxl__devices_destroy is somehow simpler.
> > 
> > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied, thanks.

Ian.

[PATCH 2 of 6 RESENT] libxl: Accept disk name in libxl_devid_to_device_disk

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962929 -7200
# Node ID 5231fa19f3e39091ff29e2a6dca057ca54403092
# Parent  c32797243a6ba61dd2942a0307151e42fb7bf157
libxl: Accept disk name in libxl_devid_to_device_disk

Accept disk name in xl block-detach.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r c32797243a6b -r 5231fa19f3e3 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Sat Jun 04 01:33:13 2011 +0200
+++ b/tools/libxl/libxl_device.c	Wed Jun 01 23:15:29 2011 +0200
@@ -198,7 +198,7 @@ static int device_virtdisk_matches(const
     return 1;
 }
 
-int libxl__device_disk_dev_number(char *virtpath, int *pdisk, int *ppartition)
+int libxl__device_disk_dev_number(const char *virtpath, int *pdisk, int *ppartition)
 {
     int disk, partition;
     char *ep;
diff -r c32797243a6b -r 5231fa19f3e3 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Sat Jun 04 01:33:13 2011 +0200
+++ b/tools/libxl/libxl_internal.h	Wed Jun 01 23:15:29 2011 +0200
@@ -205,7 +205,7 @@ _hidden char *libxl__device_disk_string_
 _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
 
 _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
-_hidden int libxl__device_disk_dev_number(char *virtpath,
+_hidden int libxl__device_disk_dev_number(const char *virtpath,
                                           int *pdisk, int *ppartition);
 
 _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
diff -r c32797243a6b -r 5231fa19f3e3 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Sat Jun 04 01:33:13 2011 +0200
+++ b/tools/libxl/libxl_utils.c	Wed Jun 01 23:15:29 2011 +0200
@@ -530,18 +530,18 @@ int libxl_devid_to_device_disk(libxl_ctx
                                const char *devid, libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    char *endptr, *val;
+    char *val;
     char *dompath, *diskpath, *be_path;
     unsigned int devid_n;
     int rc = ERROR_INVAL;
 
-    devid_n = strtoul(devid, &endptr, 10);
-    if (devid == endptr) {
+    devid_n = libxl__device_disk_dev_number(devid, NULL, NULL);
+    if (devid_n < 0) {
         goto out;
     }
     rc = ERROR_FAIL;
     dompath = libxl__xs_get_dompath(&gc, domid);
-    diskpath = libxl__sprintf(&gc, "%s/device/vbd/%s", dompath, devid);
+    diskpath = libxl__sprintf(&gc, "%s/device/vbd/%d", dompath, devid_n);
     if (!diskpath) {
         goto out;
     }

Re: [PATCH 2 of 6 RESENT] libxl: Accept disk name in libxl_devid_to_device_disk

[Ian Campbell]

On Sun, 2011-06-05 at 17:50 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962929 -7200
> # Node ID 5231fa19f3e39091ff29e2a6dca057ca54403092
> # Parent  c32797243a6ba61dd2942a0307151e42fb7bf157
> libxl: Accept disk name in libxl_devid_to_device_disk
> 
> Accept disk name in xl block-detach.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

While reviewing I noticed a path out of libxl__device_disk_dev_number
which would succeed without setting pdisk or ppartition, which would
surprise a caller which provided them. 

I wasn't immediately sure from docs/misc/vbd-interface.txt how to parse
a vdev which is an arbitrary number. Many cases are covered in the
document but there are others (e.g. NetBSD uses small integers iirc)
which I was unsure of. (I expect that document needs expanding to cover
these cases, but I'm not sure what to put...)

For now I just returned an error to prevent a cascading failure.

8<-------------------------------------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1307359172 -3600
# Node ID b5dfb12aa231c98a4dcf3372d3bf1cf9a85df2a4
# Parent  86009542df09c70ca8b4a95e4dd3de63cf95c9d6
libxl: fail to parse disk vpath if a disk+part number is required but unavailable

libxl__device_disk_dev_number() can parse a virtpath which is an encoded
unsigned long but does not set *pdisk or *ppartition in that case.

Ideally we would parse the number but for now simply fail to prevent cascading
failures.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 86009542df09 -r b5dfb12aa231 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Mon Jun 06 12:11:25 2011 +0100
+++ b/tools/libxl/libxl_device.c	Mon Jun 06 12:19:32 2011 +0100
@@ -222,8 +222,12 @@ int libxl__device_disk_dev_number(char *
 
     errno = 0;
     ul = strtoul(virtpath, &ep, 0);
-    if (!errno && !*ep && ul <= INT_MAX)
+    if (!errno && !*ep && ul <= INT_MAX) {
+        /* FIXME: should parse ul to determine these. */
+        if (pdisk || ppartition)
+            return -1;
         return ul;
+    }
 
     if (device_virtdisk_matches(virtpath, "hd",
                                 &disk, 3,

Re: [PATCH 2 of 6 RESENT] libxl: Accept disk name in libxl_devid_to_device_disk

[Ian Campbell]

On Mon, 2011-06-06 at 12:24 +0100, Ian Campbell wrote:
> On Sun, 2011-06-05 at 17:50 +0100, Marek Marczykowski wrote:
> > # HG changeset patch
> > # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> > # Date 1306962929 -7200
> > # Node ID 5231fa19f3e39091ff29e2a6dca057ca54403092
> > # Parent  c32797243a6ba61dd2942a0307151e42fb7bf157
> > libxl: Accept disk name in libxl_devid_to_device_disk
> > 
> > Accept disk name in xl block-detach.
> > 
> > Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> While reviewing I noticed a path out of libxl__device_disk_dev_number
> which would succeed without setting pdisk or ppartition, which would
> surprise a caller which provided them. 
> 
> I wasn't immediately sure from docs/misc/vbd-interface.txt how to parse
> a vdev which is an arbitrary number. Many cases are covered in the
> document but there are others (e.g. NetBSD uses small integers iirc)
> which I was unsure of. (I expect that document needs expanding to cover
> these cases, but I'm not sure what to put...)

Actually the doc does say:
        To cope with guests which predate this specification we preserve the
        existing facility to specify the xenstore numerical value directly by
        putting a single number (hex, decimal or octal) in the domain config
        file instead of the disk identifier; this number is written directly
        to xenstore (after conversion to the canonical decimal format).

so I guess it is explicit that these values cannot be reliably parsed.

Ian.

Re: [PATCH 2 of 6 RESENT] libxl: Accept disk name in libxl_devid_to_device_disk [and 1 more messages]

[Ian Jackson]

Marek Marczykowski writes ("[Xen-devel] [PATCH 2 of 6 RESENT] libxl: Accept disk name in libxl_devid_to_device_disk"):
> libxl: Accept disk name in libxl_devid_to_device_disk

Applied, thanks (long lines fixed up).

Ian.

[PATCH 3 of 6 RESENT] libxl: Allocate memory for strings in libxl_device_disk

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962954 -7200
# Node ID 9fe949c7ab9601bb5500a53c538f7a23b61e1bcb
# Parent  5231fa19f3e39091ff29e2a6dca057ca54403092
libxl: Allocate memory for strings in libxl_device_disk

Memory for strings in libxl_device_disk must be allocated from outside of
libxl__gc to not be freed at the end of function (by libxl__free_all).

Fixes xl block-detach

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r 5231fa19f3e3 -r 9fe949c7ab96 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Wed Jun 01 23:15:29 2011 +0200
+++ b/tools/libxl/libxl_utils.c	Wed Jun 01 23:15:54 2011 +0200
@@ -551,10 +551,10 @@ int libxl_devid_to_device_disk(libxl_ctx
         goto out;
     disk->backend_domid = strtoul(val, NULL, 10);
     be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
-    disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path));
+    disk->pdev_path = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path), NULL);
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
     libxl_string_to_backend(ctx, val, &(disk->backend));
-    disk->vdev = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path));
+    disk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path), NULL);
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
     disk->unpluggable = !strcmp(val, "1");
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path));

Re: [PATCH 3 of 6 RESENT] libxl: Allocate memory for strings in libxl_device_disk

[Ian Campbell]

On Sun, 2011-06-05 at 17:50 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962954 -7200
> # Node ID 9fe949c7ab9601bb5500a53c538f7a23b61e1bcb
> # Parent  5231fa19f3e39091ff29e2a6dca057ca54403092
> libxl: Allocate memory for strings in libxl_device_disk
> 
> Memory for strings in libxl_device_disk must be allocated from outside of
> libxl__gc to not be freed at the end of function (by libxl__free_all).
> 
> Fixes xl block-detach
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r 5231fa19f3e3 -r 9fe949c7ab96 tools/libxl/libxl_utils.c
> --- a/tools/libxl/libxl_utils.c	Wed Jun 01 23:15:29 2011 +0200
> +++ b/tools/libxl/libxl_utils.c	Wed Jun 01 23:15:54 2011 +0200
> @@ -551,10 +551,10 @@ int libxl_devid_to_device_disk(libxl_ctx
>          goto out;
>      disk->backend_domid = strtoul(val, NULL, 10);
>      be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
> -    disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path));
> +    disk->pdev_path = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path), NULL);
>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
>      libxl_string_to_backend(ctx, val, &(disk->backend));
> -    disk->vdev = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path));
> +    disk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path), NULL);
>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
>      disk->unpluggable = !strcmp(val, "1");
>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path));
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 3 of 6 RESENT] libxl: Allocate memory for strings in libxl_device_disk

[Ian Jackson]

Marek Marczykowski writes ("[Xen-devel] [PATCH 3 of 6 RESENT] libxl: Allocate memory for strings in libxl_device_disk"):
> libxl: Allocate memory for strings in libxl_device_disk
> 
> Memory for strings in libxl_device_disk must be allocated from outside of
> libxl__gc to not be freed at the end of function (by libxl__free_all).
> 
> Fixes xl block-detach

Applied, thanks (long lines fixed up).

Ian.

[PATCH 4 of 6 RESENT] xl: Use macros for param parsing in network-attach, to prevent use explicit sizeof("param")

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307145042 -7200
# Node ID 0863dfd23b3a10bee6d3eda30415dff0eef2bee1
# Parent  9fe949c7ab9601bb5500a53c538f7a23b61e1bcb
xl: Use macros for param parsing in network-attach, to prevent use explicit sizeof("param")

'script=' length was wrong... Replaced calls to strncmp("param", *argv,
explicit sizeof("param")) with macro and helper function to extract parameter
value. Also introduce replace_string function to simplify code.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r 9fe949c7ab96 -r 0863dfd23b3a tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Wed Jun 01 23:15:54 2011 +0200
+++ b/tools/libxl/xl_cmdimpl.c	Sat Jun 04 01:50:42 2011 +0200
@@ -1229,6 +1229,24 @@ static int handle_domain_death(libxl_ctx
     return restart;
 }
 
+/* for now used only by main_networkattach, but can be reused elsewhere */
+static int match_option_size(const char *prefix, size_t len,
+        char *arg, char **argopt)
+{
+    int rc = strncmp(prefix, arg, len);
+    if (!rc) *argopt = arg+len;
+    return !rc;
+}
+#define match_option(_prefix, _arg, _oparg) \
+    match_option_size((_prefix "="), sizeof((_prefix)) + 1, (_arg), &(_oparg))
+
+static void replace_string(char **str, const char *val)
+{
+    free(*str);
+    *str = strdup(val);
+}
+
+
 static int preserve_domain(libxl_ctx *ctx, uint32_t domid, libxl_event *event,
                            libxl_domain_config *d_config, libxl_dominfo *info)
 {
@@ -3925,7 +3943,7 @@ int main_networkattach(int argc, char **
 {
     int opt;
     libxl_device_nic nic;
-    char *endptr;
+    char *endptr, *oparg;
     const char *tok;
     int i;
     unsigned int val;
@@ -3944,17 +3962,17 @@ int main_networkattach(int argc, char **
     }
     libxl_device_nic_init(&nic, -1);
     for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
-        if (!strncmp("type=", *argv, 5)) {
-            if (!strncmp("vif", (*argv) + 5, 4)) {
+        if (match_option("type", *argv, oparg)) {
+            if (!strcmp("vif", oparg)) {
                 nic.nictype = LIBXL_NIC_TYPE_VIF;
-            } else if (!strncmp("ioemu", (*argv) + 5, 5)) {
+            } else if (!strcmp("ioemu", oparg)) {
                 nic.nictype = LIBXL_NIC_TYPE_IOEMU;
             } else {
                 fprintf(stderr, "Invalid parameter `type'.\n");
                 return 1;
             }
-        } else if (!strncmp("mac=", *argv, 4)) {
-            tok = strtok((*argv) + 4, ":");
+        } else if (match_option("mac", *argv, oparg)) {
+            tok = strtok(oparg, ":");
             for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
                 val = strtoul(tok, &endptr, 16);
                 if ((tok == endptr) || (val > 255)) {
@@ -3963,29 +3981,24 @@ int main_networkattach(int argc, char **
                 }
                 nic.mac[i] = val;
             }
-        } else if (!strncmp("bridge=", *argv, 7)) {
-            free(nic.bridge);
-            nic.bridge = strdup((*argv) + 7);
-        } else if (!strncmp("ip=", *argv, 3)) {
-            free(nic.ip);
-            nic.ip = strdup((*argv) + 3);
-        } else if (!strncmp("script=", *argv, 6)) {
-            free(nic.script);
-            nic.script = strdup((*argv) + 6);
-        } else if (!strncmp("backend=", *argv, 8)) {
-            if(libxl_name_to_domid(ctx, ((*argv) + 8), &val)) {
+        } else if (match_option("bridge", *argv, oparg)) {
+            replace_string(&nic.bridge, oparg);
+        } else if (match_option("ip", *argv, oparg)) {
+            replace_string(&nic.ip, oparg);
+        } else if (match_option("script", *argv, oparg)) {
+            replace_string(&nic.script, oparg);
+        } else if (match_option("backend", *argv, oparg)) {
+            if(libxl_name_to_domid(ctx, oparg, &val)) {
                 fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
                 val = 0;
             }
             nic.backend_domid = val;
-        } else if (!strncmp("vifname=", *argv, 8)) {
-            free(nic.ifname);
-            nic.ifname = strdup((*argv) + 8);
-        } else if (!strncmp("model=", *argv, 6)) {
-            free(nic.model);
-            nic.model = strdup((*argv) + 6);
-        } else if (!strncmp("rate=", *argv, 5)) {
-        } else if (!strncmp("accel=", *argv, 6)) {
+        } else if (match_option("vifname", *argv, oparg)) {
+            replace_string(&nic.ifname, oparg);
+        } else if (match_option("model", *argv, oparg)) {
+            replace_string(&nic.model, oparg);
+        } else if (match_option("rate", *argv, oparg)) {
+        } else if (match_option("accel", *argv, oparg)) {
         } else {
             fprintf(stderr, "unrecognized argument `%s'\n", *argv);
             return 1;

Re: [PATCH 4 of 6 RESENT] xl: Use macros for param parsing in network-attach, to prevent use explicit sizeof("param")

[Stefano Stabellini]

On Sun, 5 Jun 2011, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1307145042 -7200
> # Node ID 0863dfd23b3a10bee6d3eda30415dff0eef2bee1
> # Parent  9fe949c7ab9601bb5500a53c538f7a23b61e1bcb
> xl: Use macros for param parsing in network-attach, to prevent use explicit sizeof("param")
> 
> 'script=' length was wrong... Replaced calls to strncmp("param", *argv,
> explicit sizeof("param")) with macro and helper function to extract parameter
> value. Also introduce replace_string function to simplify code.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

It seems correct to me and makes the code easier to read, so

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> diff -r 9fe949c7ab96 -r 0863dfd23b3a tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed Jun 01 23:15:54 2011 +0200
> +++ b/tools/libxl/xl_cmdimpl.c	Sat Jun 04 01:50:42 2011 +0200
> @@ -1229,6 +1229,24 @@ static int handle_domain_death(libxl_ctx
>      return restart;
>  }
>  
> +/* for now used only by main_networkattach, but can be reused elsewhere */
> +static int match_option_size(const char *prefix, size_t len,
> +        char *arg, char **argopt)
> +{
> +    int rc = strncmp(prefix, arg, len);
> +    if (!rc) *argopt = arg+len;
> +    return !rc;
> +}
> +#define match_option(_prefix, _arg, _oparg) \
> +    match_option_size((_prefix "="), sizeof((_prefix)) + 1, (_arg), &(_oparg))
> +
> +static void replace_string(char **str, const char *val)
> +{
> +    free(*str);
> +    *str = strdup(val);
> +}
> +
> +
>  static int preserve_domain(libxl_ctx *ctx, uint32_t domid, libxl_event *event,
>                             libxl_domain_config *d_config, libxl_dominfo *info)
>  {
> @@ -3925,7 +3943,7 @@ int main_networkattach(int argc, char **
>  {
>      int opt;
>      libxl_device_nic nic;
> -    char *endptr;
> +    char *endptr, *oparg;
>      const char *tok;
>      int i;
>      unsigned int val;
> @@ -3944,17 +3962,17 @@ int main_networkattach(int argc, char **
>      }
>      libxl_device_nic_init(&nic, -1);
>      for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
> -        if (!strncmp("type=", *argv, 5)) {
> -            if (!strncmp("vif", (*argv) + 5, 4)) {
> +        if (match_option("type", *argv, oparg)) {
> +            if (!strcmp("vif", oparg)) {
>                  nic.nictype = LIBXL_NIC_TYPE_VIF;
> -            } else if (!strncmp("ioemu", (*argv) + 5, 5)) {
> +            } else if (!strcmp("ioemu", oparg)) {
>                  nic.nictype = LIBXL_NIC_TYPE_IOEMU;
>              } else {
>                  fprintf(stderr, "Invalid parameter `type'.\n");
>                  return 1;
>              }
> -        } else if (!strncmp("mac=", *argv, 4)) {
> -            tok = strtok((*argv) + 4, ":");
> +        } else if (match_option("mac", *argv, oparg)) {
> +            tok = strtok(oparg, ":");
>              for (i = 0; tok && i < 6; tok = strtok(NULL, ":"), ++i) {
>                  val = strtoul(tok, &endptr, 16);
>                  if ((tok == endptr) || (val > 255)) {
> @@ -3963,29 +3981,24 @@ int main_networkattach(int argc, char **
>                  }
>                  nic.mac[i] = val;
>              }
> -        } else if (!strncmp("bridge=", *argv, 7)) {
> -            free(nic.bridge);
> -            nic.bridge = strdup((*argv) + 7);
> -        } else if (!strncmp("ip=", *argv, 3)) {
> -            free(nic.ip);
> -            nic.ip = strdup((*argv) + 3);
> -        } else if (!strncmp("script=", *argv, 6)) {
> -            free(nic.script);
> -            nic.script = strdup((*argv) + 6);
> -        } else if (!strncmp("backend=", *argv, 8)) {
> -            if(libxl_name_to_domid(ctx, ((*argv) + 8), &val)) {
> +        } else if (match_option("bridge", *argv, oparg)) {
> +            replace_string(&nic.bridge, oparg);
> +        } else if (match_option("ip", *argv, oparg)) {
> +            replace_string(&nic.ip, oparg);
> +        } else if (match_option("script", *argv, oparg)) {
> +            replace_string(&nic.script, oparg);
> +        } else if (match_option("backend", *argv, oparg)) {
> +            if(libxl_name_to_domid(ctx, oparg, &val)) {
>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
>                  val = 0;
>              }
>              nic.backend_domid = val;
> -        } else if (!strncmp("vifname=", *argv, 8)) {
> -            free(nic.ifname);
> -            nic.ifname = strdup((*argv) + 8);
> -        } else if (!strncmp("model=", *argv, 6)) {
> -            free(nic.model);
> -            nic.model = strdup((*argv) + 6);
> -        } else if (!strncmp("rate=", *argv, 5)) {
> -        } else if (!strncmp("accel=", *argv, 6)) {
> +        } else if (match_option("vifname", *argv, oparg)) {
> +            replace_string(&nic.ifname, oparg);
> +        } else if (match_option("model", *argv, oparg)) {
> +            replace_string(&nic.model, oparg);
> +        } else if (match_option("rate", *argv, oparg)) {
> +        } else if (match_option("accel", *argv, oparg)) {
>          } else {
>              fprintf(stderr, "unrecognized argument `%s'\n", *argv);
>              return 1;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

[PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307145131 -7200
# Node ID 0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
# Parent  0863dfd23b3a10bee6d3eda30415dff0eef2bee1
xen.lowlevel.xl: Return None on empty domain name

Previously PyString_FromString(NULL) was called, which caused assertion
failure.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r 0863dfd23b3a -r 0c0f9e1bd140 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Sat Jun 04 01:50:42 2011 +0200
+++ b/tools/python/xen/lowlevel/xl/xl.c	Sat Jun 04 01:52:11 2011 +0200
@@ -409,14 +409,16 @@ static PyObject *pyxl_domid_to_name(XlOb
 {
     char *domname;
     int domid;
-    PyObject *ret;
+    PyObject *ret = Py_None;
 
     if ( !PyArg_ParseTuple(args, "i", &domid) )
         return NULL;
 
     domname = libxl_domid_to_name(self->ctx, domid);
-    ret = PyString_FromString(domname);
-    free(domname);
+    if (domname) {
+        ret = PyString_FromString(domname);
+        free(domname);
+    }
 
     return ret;
 }

Re: [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name

[Ian Jackson]

Marek Marczykowski writes ("[Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name"):
> xen.lowlevel.xl: Return None on empty domain name
> 
> Previously PyString_FromString(NULL) was called, which caused assertion
> failure.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

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

I modified it slightly to make the diff and complexity slightly
smaller (since "free(NULL)" is legal).

Ian.

Backport libxl bugfixes to 4.1 (was: Re: [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 967 bytes --]

On 27.06.2011 18:40, Ian Jackson wrote:
> Right, I think I've dealt with all your outstanding patches now.

Looks good, thanks :)
Keir, can you apply them to 4.1 tree? This require slight modifications,
but all of them are fixing bugs present also in 4.1.

I can also send patches ready for 4.1 tree if it helps.

This is all about:
23607:2f63562df1c4	libxl: Do not SEGV when no 'removable' disk parameter
in xenstoredefault tip
23606:cc2f376d0cd9	xen.lowlevel.xl: Return None on empty domain name
23605:ff8d170852b3	libxl: Remove frontend and backend devices from
xenstore after destroy
23604:5d7998be2252	libxl: Accept disk name in libxl_devid_to_device_disk
23603:6656d80b4de4	libxl: Allocate memory for strings in libxl_device_disk
23602:5c353f53c8e2	xl: Use macros for param parsing in network-attach


-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Backport libxl bugfixes to 4.1 (was: Re: [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)

[Keir Fraser]

On 28/06/2011 22:37, "Marek Marczykowski" <marmarek@mimuw.edu.pl> wrote:

> On 27.06.2011 18:40, Ian Jackson wrote:
>> Right, I think I've dealt with all your outstanding patches now.
> 
> Looks good, thanks :)
> Keir, can you apply them to 4.1 tree? This require slight modifications,
> but all of them are fixing bugs present also in 4.1.

Ian Jackson deals with libxl backports to 4.1.

 -- Keir

> I can also send patches ready for 4.1 tree if it helps.
> 
> This is all about:
> 23607:2f63562df1c4 libxl: Do not SEGV when no 'removable' disk parameter
> in xenstoredefault tip
> 23606:cc2f376d0cd9 xen.lowlevel.xl: Return None on empty domain name
> 23605:ff8d170852b3 libxl: Remove frontend and backend devices from
> xenstore after destroy
> 23604:5d7998be2252 libxl: Accept disk name in libxl_devid_to_device_disk
> 23603:6656d80b4de4 libxl: Allocate memory for strings in libxl_device_disk
> 23602:5c353f53c8e2 xl: Use macros for param parsing in network-attach
> 

Backport libxl bugfixes to 4.1 (was: Re: [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)

[Ian Jackson]

Marek Marczykowski writes ("Backport libxl bugfixes to 4.1 (was: Re: [Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)"):
> On 27.06.2011 18:40, Ian Jackson wrote:
> > Right, I think I've dealt with all your outstanding patches now.
> 
> Looks good, thanks :)
> Keir, can you apply them to 4.1 tree? This require slight modifications,
> but all of them are fixing bugs present also in 4.1.

Can we let it all sit in -unstable for a few weeks ?

> I can also send patches ready for 4.1 tree if it helps.

That would certainly be useful.

> This is all about:
> 23607:2f63562df1c4	libxl: Do not SEGV when no 'removable' disk parameter
> 23606:cc2f376d0cd9	xen.lowlevel.xl: Return None on empty domain name
> 23605:ff8d170852b3	libxl: Remove frontend and backend devices from
> 23604:5d7998be2252	libxl: Accept disk name in libxl_devid_to_device_disk
> 23603:6656d80b4de4	libxl: Allocate memory for strings in libxl_device_disk

Without checking each one, I think these are suitable for backport.

> 23602:5c353f53c8e2	xl: Use macros for param parsing in network-attach

This one isn't, I think ?

Ian.

Backport libxl bugfixes to 4.1 (was: Re: [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)

[Ian Jackson]

Ian Jackson writes ("Backport libxl bugfixes to 4.1 (was: Re: [Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)"):
> Marek Marczykowski writes ("Backport libxl bugfixes to 4.1 (was: Re: [Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)"):
> > I can also send patches ready for 4.1 tree if it helps.
> 
> That would certainly be useful.
> 
> > This is all about:
> > 23607:2f63562df1c4	libxl: Do not SEGV when no 'removable' disk parameter
> > 23606:cc2f376d0cd9	xen.lowlevel.xl: Return None on empty domain name
> > 23605:ff8d170852b3	libxl: Remove frontend and backend devices from
> > 23604:5d7998be2252	libxl: Accept disk name in libxl_devid_to_device_disk
> > 23603:6656d80b4de4	libxl: Allocate memory for strings in libxl_device_disk
> 
> Without checking each one, I think these are suitable for backport.

Did you send backports for these ?  I think we may have missed them.

Ian.

Re: Backport libxl bugfixes to 4.1

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1214 bytes --]

On 25.08.2011 13:05, Ian Jackson wrote:
> Ian Jackson writes ("Backport libxl bugfixes to 4.1 (was: Re: [Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)"):
>> Marek Marczykowski writes ("Backport libxl bugfixes to 4.1 (was: Re: [Xen-devel] [PATCH 5 of 6 RESENT] xen.lowlevel.xl: Return None on empty domain name)"):
>>> I can also send patches ready for 4.1 tree if it helps.
>>
>> That would certainly be useful.
>>
>>> This is all about:
>>> 23607:2f63562df1c4	libxl: Do not SEGV when no 'removable' disk parameter
>>> 23606:cc2f376d0cd9	xen.lowlevel.xl: Return None on empty domain name
>>> 23605:ff8d170852b3	libxl: Remove frontend and backend devices from
>>> 23604:5d7998be2252	libxl: Accept disk name in libxl_devid_to_device_disk
>>> 23603:6656d80b4de4	libxl: Allocate memory for strings in libxl_device_disk
>>
>> Without checking each one, I think these are suitable for backport.
> 
> Did you send backports for these ?  I think we may have missed them.

Not yet. Will send in near future (today/tomorrow).

-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

[PATCH 0 of 5] A bunch of fixes for libxl

[Marek Marczykowski]

Backorted version of patches.

[PATCH 1 of 5] libxl: Remove frontend and backend devices from xenstore after destroy

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307285159 -7200
# Node ID 969051f07ee813d2a556f50d37cf59d4e509bf67
# Parent  43acc031eb24945973dffda2b7caf976993bbd5f
libxl: Remove frontend and backend devices from xenstore after destroy

Cleanup frontend and backend devices from xenstore for all dev types - not only
disks. Because backend cleanup moved to libxl__device_destroy,
libxl__devices_destroy is somehow simpler.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -269,7 +269,9 @@ retry_transaction:
     if (!force) {
         xs_watch(ctx->xsh, state_path, be_path);
         rc = 1;
-    }
+    } else {
+		xs_rm(ctx->xsh, XBT_NULL, be_path);
+	}
 out:
     libxl__free_all(&gc);
     return rc;
@@ -310,10 +312,8 @@ int libxl__devices_destroy(libxl_ctx *ct
     char *path, *be_path, *fe_path;
     unsigned int num1, num2;
     char **l1 = NULL, **l2 = NULL;
-    int i, j, n = 0, n_watches = 0;
-    flexarray_t *toremove;
+    int i, j, n_watches = 0;
 
-    toremove = flexarray_make(16, 1);
     path = libxl__sprintf(&gc, "/local/domain/%d/device", domid);
     l1 = libxl__xs_directory(&gc, XBT_NULL, path, &num1);
     if (!l1) {
@@ -337,7 +337,6 @@ int libxl__devices_destroy(libxl_ctx *ct
             if (be_path != NULL) {
                 if (libxl__device_destroy(ctx, be_path, force) > 0)
                     n_watches++;
-                flexarray_set(toremove, n++, libxl__dirname(&gc, be_path));
             } else {
                 xs_rm(ctx->xsh, XBT_NULL, path);
             }
@@ -350,7 +349,6 @@ int libxl__devices_destroy(libxl_ctx *ct
     if (be_path && strcmp(be_path, "")) {
         if (libxl__device_destroy(ctx, be_path, force) > 0)
             n_watches++;
-        flexarray_set(toremove, n++, libxl__dirname(&gc, be_path));
     }
 
     if (!force) {
@@ -370,12 +368,7 @@ int libxl__devices_destroy(libxl_ctx *ct
             }
         }
     }
-    for (i = 0; i < n; i++) {
-        flexarray_get(toremove, i, (void**) &path);
-        xs_rm(ctx->xsh, XBT_NULL, path);
-    }
 out:
-    flexarray_free(toremove);
     libxl__free_all(&gc);
     return 0;
 }
@@ -401,6 +394,7 @@ int libxl__device_del(libxl_ctx *ctx, li
         (void)wait_for_dev_destroy(ctx, &tv);
     }
 
+    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));
     rc = 0;
 
 out:

[PATCH 2 of 5] libxl: Accept disk name in libxl_devid_to_device_disk

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307285297 -7200
# Node ID 8080606fb87a751f7cde4c6a08e3945a21d397b0
# Parent  969051f07ee813d2a556f50d37cf59d4e509bf67
libxl: Accept disk name in libxl_devid_to_device_disk

Accept disk name in xl block-detach.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -201,7 +201,7 @@ static int device_virtdisk_matches(const
     return 1;
 }
 
-int libxl__device_disk_dev_number(char *virtpath)
+int libxl__device_disk_dev_number(const char *virtpath)
 {
     int disk, partition;
     char *ep;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -182,7 +182,7 @@ _hidden char *libxl__device_disk_string_
 _hidden char *libxl__device_disk_string_of_format(libxl_disk_format format);
 
 _hidden int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *minor);
-_hidden int libxl__device_disk_dev_number(char *virtpath);
+_hidden int libxl__device_disk_dev_number(const char *virtpath);
 
 _hidden int libxl__device_generic_add(libxl_ctx *ctx, libxl__device *device,
                              char **bents, char **fents);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -529,18 +529,18 @@ int libxl_devid_to_device_disk(libxl_ctx
                                const char *devid, libxl_device_disk *disk)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    char *endptr, *val;
+    char *val;
     char *dompath, *diskpath, *be_path;
     unsigned int devid_n;
     int rc = ERROR_INVAL;
 
-    devid_n = strtoul(devid, &endptr, 10);
-    if (devid == endptr) {
+    devid_n = libxl__device_disk_dev_number(devid);
+    if (devid_n < 0) {
         goto out;
     }
     rc = ERROR_FAIL;
     dompath = libxl__xs_get_dompath(&gc, domid);
-    diskpath = libxl__sprintf(&gc, "%s/device/vbd/%s", dompath, devid);
+    diskpath = libxl__sprintf(&gc, "%s/device/vbd/%d", dompath, devid_n);
     if (!diskpath) {
         goto out;
     }

[PATCH 3 of 5] libxl: Allocate memory for strings in libxl_device_disk

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962954 -7200
# Node ID aa6e0521bb3853b0e1d90e870220568020929cf5
# Parent  8080606fb87a751f7cde4c6a08e3945a21d397b0
libxl: Allocate memory for strings in libxl_device_disk

Memory for strings in libxl_device_disk must be allocated from outside of
libxl__gc to not be freed at the end of function (by libxl__free_all).

Fixes xl block-detach

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -551,10 +551,10 @@ int libxl_devid_to_device_disk(libxl_ctx
     disk->backend_domid = strtoul(val, NULL, 10);
     disk->domid = domid;
     be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
-    disk->pdev_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path));
+    disk->pdev_path = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path), NULL);
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
     libxl_string_to_backend(ctx, val, &(disk->backend));
-    disk->vdev = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path));
+    disk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path), NULL);
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
     disk->unpluggable = !strcmp(val, "1");
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mode", be_path));

[PATCH 4 of 5] xen.lowlevel.xl: Return None on empty domain name

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307285583 -7200
# Node ID b77de60c85431593d439f2c4ac46023c6f8e5ee2
# Parent  f33981cd7d3df1718123cd364ee91a7bad064283
xen.lowlevel.xl: Return None on empty domain name

Previously PyString_FromString(NULL) was called, which caused assertion
failure.

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -412,14 +412,16 @@ static PyObject *pyxl_domid_to_name(XlOb
 {
     char *domname;
     int domid;
-    PyObject *ret;
+    PyObject *ret = Py_None;
 
     if ( !PyArg_ParseTuple(args, "i", &domid) )
         return NULL;
 
     domname = libxl_domid_to_name(&self->ctx, domid);
-    ret = PyString_FromString(domname);
-    free(domname);
+    if (domname) {
+        ret = PyString_FromString(domname);
+        free(domname);
+    }
 
     return ret;
 }

[PATCH 5 of 5] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307285721 -7200
# Node ID fed2015fe1745e5e712380960cc50a71629698fc
# Parent  b77de60c85431593d439f2c4ac46023c6f8e5ee2
libxl: Do not SEGV when no 'removable' disk parameter in xenstore

Just assume disk as not removable when no 'removable' paremeter

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1724,6 +1724,7 @@ static unsigned int libxl_append_disk_li
                              libxl__xs_get_dompath(&gc, 0), type, domid);
     dir = libxl__xs_directory(&gc, XBT_NULL, be_path, &n);
     if (dir) {
+        char *removable;
         *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         pdisk = *disks + *ndisks;
         *ndisks += n;
@@ -1742,7 +1743,11 @@ static unsigned int libxl_append_disk_li
                 libxl__sprintf(&gc, "%s/%s/type", be_path, *dir)), 
                 &(pdisk->backend));
             pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/dev", be_path, *dir), &len);
-            pdisk->unpluggable = atoi(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path, *dir)));
+            removable = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path, *dir));
+            if (removable)
+                pdisk->unpluggable = atoi(removable);
+            else
+                pdisk->unpluggable = 0;
             if (!strcmp(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/mode", be_path, *dir)), "w"))
                 pdisk->readwrite = 1;
             else

Re: [PATCH 0 of 5] A bunch of fixes for libxl

[Ian Jackson]

Marek Marczykowski writes ("[Xen-devel] [PATCH 0 of 5] A bunch of fixes for libxl"):
> Backorted version of patches.

Applied all 5, thanks.

Ian.

[PATCH 6 of 6 RESENT] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307145395 -7200
# Node ID 70fc2a0f0a6003f1bf591cd941a841a9e6b69c01
# Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
libxl: Do not SEGV when no 'removable' disk parameter in xenstore

Just assume disk as not removable when no 'removable' paremeter

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff -r 0c0f9e1bd140 -r 70fc2a0f0a60 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Sat Jun 04 01:52:11 2011 +0200
+++ b/tools/libxl/libxl.c	Sat Jun 04 01:56:35 2011 +0200
@@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
                              libxl__xs_get_dompath(gc, 0), type, domid);
     dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
     if (dir) {
+        char *removable;
         *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         pdisk = *disks + *ndisks;
         *ndisks += n;
@@ -1581,6 +1582,11 @@ static unsigned int libxl__append_disk_l
                 &(pdisk->backend));
             pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
             pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));
+            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
+            if (removable)
+                pdisk->unpluggable = atoi(removable);
+            else
+                pdisk->unpluggable = 0;
             if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
                 pdisk->readwrite = 1;
             else

Re: [PATCH 6 of 6 RESENT] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Stefano Stabellini]

On Sun, 5 Jun 2011, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1307145395 -7200
> # Node ID 70fc2a0f0a6003f1bf591cd941a841a9e6b69c01
> # Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
> libxl: Do not SEGV when no 'removable' disk parameter in xenstore
> 
> Just assume disk as not removable when no 'removable' paremeter
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> diff -r 0c0f9e1bd140 -r 70fc2a0f0a60 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Sat Jun 04 01:52:11 2011 +0200
> +++ b/tools/libxl/libxl.c	Sat Jun 04 01:56:35 2011 +0200
> @@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
>                               libxl__xs_get_dompath(gc, 0), type, domid);
>      dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
>      if (dir) {
> +        char *removable;
>          *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
>          pdisk = *disks + *ndisks;
>          *ndisks += n;
> @@ -1581,6 +1582,11 @@ static unsigned int libxl__append_disk_l
>                  &(pdisk->backend));
>              pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
>              pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));

shouldn't you be removing this ^ line? 

> +            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
> +            if (removable)
> +                pdisk->unpluggable = atoi(removable);
> +            else
> +                pdisk->unpluggable = 0;
>              if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
>                  pdisk->readwrite = 1;
>              else
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH 6 of 6 RESENT] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2304 bytes --]

On 07.06.2011 13:57, Stefano Stabellini wrote:
> On Sun, 5 Jun 2011, Marek Marczykowski wrote:
>> # HG changeset patch
>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>> # Date 1307145395 -7200
>> # Node ID 70fc2a0f0a6003f1bf591cd941a841a9e6b69c01
>> # Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
>> libxl: Do not SEGV when no 'removable' disk parameter in xenstore
>>
>> Just assume disk as not removable when no 'removable' paremeter
>>
>> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
>>
>> diff -r 0c0f9e1bd140 -r 70fc2a0f0a60 tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c	Sat Jun 04 01:52:11 2011 +0200
>> +++ b/tools/libxl/libxl.c	Sat Jun 04 01:56:35 2011 +0200
>> @@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
>>                               libxl__xs_get_dompath(gc, 0), type, domid);
>>      dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
>>      if (dir) {
>> +        char *removable;
>>          *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
>>          pdisk = *disks + *ndisks;
>>          *ndisks += n;
>> @@ -1581,6 +1582,11 @@ static unsigned int libxl__append_disk_l
>>                  &(pdisk->backend));
>>              pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
>>              pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));
> 
> shouldn't you be removing this ^ line? 

Yes, should be...

> 
>> +            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
>> +            if (removable)
>> +                pdisk->unpluggable = atoi(removable);
>> +            else
>> +                pdisk->unpluggable = 0;
>>              if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
>>                  pdisk->readwrite = 1;
>>              else
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>>


-- 
Pozdrawiam / Best Regards,
Marek Marczykowski         | RLU #390519
marmarek at mimuw edu pl   | xmpp:marmarek at staszic waw pl


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5842 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

[PATCH] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307145395 -7200
# Node ID b2b8fef3732c10f012fc209d2850e80d95471582
# Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
libxl: Do not SEGV when no 'removable' disk parameter in xenstore

Just assume disk as not removable when no 'removable' paremeter

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
                              libxl__xs_get_dompath(gc, 0), type, domid);
     dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
     if (dir) {
+        char *removable;
         *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         pdisk = *disks + *ndisks;
         *ndisks += n;
@@ -1580,7 +1581,11 @@ static unsigned int libxl__append_disk_l
                 libxl__sprintf(gc, "%s/%s/type", be_path, *dir)), 
                 &(pdisk->backend));
             pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
-            pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));
+            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
+            if (removable)
+                pdisk->unpluggable = atoi(removable);
+            else
+                pdisk->unpluggable = 0;
             if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
                 pdisk->readwrite = 1;
             else

Re: [PATCH] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Stefano Stabellini]

On Wed, 8 Jun 2011, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1307145395 -7200
> # Node ID b2b8fef3732c10f012fc209d2850e80d95471582
> # Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
> libxl: Do not SEGV when no 'removable' disk parameter in xenstore
> 
> Just assume disk as not removable when no 'removable' paremeter
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
>                               libxl__xs_get_dompath(gc, 0), type, domid);
>      dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
>      if (dir) {
> +        char *removable;
>          *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
>          pdisk = *disks + *ndisks;
>          *ndisks += n;
> @@ -1580,7 +1581,11 @@ static unsigned int libxl__append_disk_l
>                  libxl__sprintf(gc, "%s/%s/type", be_path, *dir)), 
>                  &(pdisk->backend));
>              pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
> -            pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));
> +            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
> +            if (removable)
> +                pdisk->unpluggable = atoi(removable);
> +            else
> +                pdisk->unpluggable = 0;
>              if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
>                  pdisk->readwrite = 1;
>              else
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: [PATCH] libxl: Do not SEGV when no 'removable' disk parameter in xenstore [and 1 more messages]

[Ian Jackson]

Marek Marczykowski writes ("[Xen-devel] [PATCH] libxl: Do not SEGV when no 'removable' disk parameter in xenstore"):
> libxl: Do not SEGV when no 'removable' disk parameter in xenstore
> 
> Just assume disk as not removable when no 'removable' paremeter
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

Thanks, applied.

(Fixed up conflict due to "unpluggable" rename, fixed up long lines).

Ian.

[PATCH RESENTv2] libxl: Do not SEGV when no 'removable' disk parameter in xenstore

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1307145395 -7200
# Node ID b2b8fef3732c10f012fc209d2850e80d95471582
# Parent  0c0f9e1bd14073b5cb1d4f58b6950d16128003fa
libxl: Do not SEGV when no 'removable' disk parameter in xenstore

Just assume disk as not removable when no 'removable' paremeter

Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1563,6 +1563,7 @@ static unsigned int libxl__append_disk_l
                              libxl__xs_get_dompath(gc, 0), type, domid);
     dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
     if (dir) {
+        char *removable;
         *disks = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         pdisk = *disks + *ndisks;
         *ndisks += n;
@@ -1580,7 +1581,11 @@ static unsigned int libxl__append_disk_l
                 libxl__sprintf(gc, "%s/%s/type", be_path, *dir)), 
                 &(pdisk->backend));
             pdisk->vdev = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(gc, "%s/%s/dev", be_path, *dir), &len);
-            pdisk->unpluggable = atoi(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir)));
+            removable = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/removable", be_path, *dir));
+            if (removable)
+                pdisk->unpluggable = atoi(removable);
+            else
+                pdisk->unpluggable = 0;
             if (!strcmp(libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s/mode", be_path, *dir)), "w"))
                 pdisk->readwrite = 1;
             else