[PATCH 00 of 10] A bunch of fixes (and one feature) for libxl

[PATCH 00 of 10] A bunch of fixes (and one feature) for libxl

[Marek Marczykowski]

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

The last patch implements simple way to reuse block* scripts in
/etc/xen/scripts (called by udev). This most likely will be ignored
because of more elegant way pending in paches on the list. But maybe
someone will be interested in it.

[PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962865 -7200
# Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
# 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.

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
@@ -1065,8 +1065,6 @@
     device.devid            = devid;
     device.kind             = DEVICE_VBD;
     rc = libxl__device_del(ctx, &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));
     libxl__free_all(&gc);
     return rc;
 }
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
@@ -401,6 +401,8 @@
         (void)wait_for_dev_destroy(ctx, &tv);
     }
 
+    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev));
+    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));
     rc = 0;
 
 out:

Re: [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy

[Ian Campbell]

Hi Marek,

Please can you add:
[diff]
showfunc = True
to your ~/.hgrc. It makes patches much easier to read.

 On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962865 -7200
> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
> # 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.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

Thanks!

The whole device destroy/teardown path is a bit of a twisty maze of
confusingly named functions and parameters (e.g. force == !wait, destroy
vs delete vs remove etc). Any attempt to try and untangle things is most
welcome and I think this is a step in the right direction.

> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1065,8 +1065,6 @@
>      device.devid            = devid;
>      device.kind             = DEVICE_VBD;
>      rc = libxl__device_del(ctx, &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));
>      libxl__free_all(&gc);
>      return rc;
>  }
> 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
> @@ -401,6 +401,8 @@
>          (void)wait_for_dev_destroy(ctx, &tv);
>      }
>  
> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev));
> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));

In the case where wait == true libxl__device_destroy will add a watch on
the be path and wait_for_dev_destroy will subsequently remove the path.

I think it would be better to add the xs_rm for be path to
libxl__device_destroy as the else clause to the !force which adds the
watch? I think this allow the other caller (libxl__devices_destroy) to
be simplified too since all that mucking around with the toremove
flexarray could be dropped.

Ian.

>      rc = 0;
>  
>  out:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy

[Marek Marczykowski]

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

On 03.06.2011 10:03, Ian Campbell wrote:
> Hi Marek,
> 
> Please can you add:
> [diff]
> showfunc = True
> to your ~/.hgrc. It makes patches much easier to read.
> 
>  On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
>> # HG changeset patch
>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>> # Date 1306962865 -7200
>> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
>> # 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.
>>
>> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> Thanks!
> 
> The whole device destroy/teardown path is a bit of a twisty maze of
> confusingly named functions and parameters (e.g. force == !wait, destroy
> vs delete vs remove etc). Any attempt to try and untangle things is most
> welcome and I think this is a step in the right direction.
> 
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1065,8 +1065,6 @@
>>      device.devid            = devid;
>>      device.kind             = DEVICE_VBD;
>>      rc = libxl__device_del(ctx, &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));
>>      libxl__free_all(&gc);
>>      return rc;
>>  }
>> 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
>> @@ -401,6 +401,8 @@
>>          (void)wait_for_dev_destroy(ctx, &tv);
>>      }
>>  
>> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev));
>> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));
> 
> In the case where wait == true libxl__device_destroy will add a watch on
> the be path and wait_for_dev_destroy will subsequently remove the path.

Only backend one...

> I think it would be better to add the xs_rm for be path to
> libxl__device_destroy as the else clause to the !force which adds the
> watch? 

Also - only backend one (libxl__device_destroy gets only backend path as
parameter).

> I think this allow the other caller (libxl__devices_destroy) to
> be simplified too since all that mucking around with the toremove
> flexarray could be dropped.

What about frontend device in xenstore? In most cases only backend is
removed. In wait_for_dev_destroy we don't know frontend patch (wheel,
not exactly true, but not always easy). Perhaps there should be two
watches - on backend AND on frontend (with writing "5" to frontend state
also?)? Or backend should be removed as you suggested, but frontend as
in my patch (after wait_for_dev_destroy if was called)?

-- 
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: [PATCH 01 of 10] libxl: Remove frontend and backend devices from xenstore after destroy

[Ian Campbell]

On Sat, 2011-06-04 at 00:30 +0100, Marek Marczykowski wrote:
> On 03.06.2011 10:03, Ian Campbell wrote:
> > Hi Marek,
> > 
> > Please can you add:
> > [diff]
> > showfunc = True
> > to your ~/.hgrc. It makes patches much easier to read.
> > 
> >  On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1306962865 -7200
> >> # Node ID e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
> >> # 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.
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> > 
> > Thanks!
> > 
> > The whole device destroy/teardown path is a bit of a twisty maze of
> > confusingly named functions and parameters (e.g. force == !wait, destroy
> > vs delete vs remove etc). Any attempt to try and untangle things is most
> > welcome and I think this is a step in the right direction.
> > 
> >>
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -1065,8 +1065,6 @@
> >>      device.devid            = devid;
> >>      device.kind             = DEVICE_VBD;
> >>      rc = libxl__device_del(ctx, &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));
> >>      libxl__free_all(&gc);
> >>      return rc;
> >>  }
> >> 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
> >> @@ -401,6 +401,8 @@
> >>          (void)wait_for_dev_destroy(ctx, &tv);
> >>      }
> >>  
> >> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_backend_path(&gc, dev));
> >> +    xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(&gc, dev));
> > 
> > In the case where wait == true libxl__device_destroy will add a watch on
> > the be path and wait_for_dev_destroy will subsequently remove the path.
> 
> Only backend one...
> 
> > I think it would be better to add the xs_rm for be path to
> > libxl__device_destroy as the else clause to the !force which adds the
> > watch? 
> 
> Also - only backend one (libxl__device_destroy gets only backend path as
> parameter).
> 
> > I think this allow the other caller (libxl__devices_destroy) to
> > be simplified too since all that mucking around with the toremove
> > flexarray could be dropped.
> 
> What about frontend device in xenstore? In most cases only backend is
> removed. In wait_for_dev_destroy we don't know frontend patch (wheel,
> not exactly true, but not always easy). Perhaps there should be two
> watches - on backend AND on frontend (with writing "5" to frontend state
> also?)? Or backend should be removed as you suggested, but frontend as
> in my patch (after wait_for_dev_destroy if was called)?

My comments were only about the location of the rm of the backend path.

I think your original patch moved the rm of the frontend path to the
right place.

an.

[PATCH 02 of 10] libxl: Do not start stubdom when not needed

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962900 -7200
# Node ID 59cff6b471f3f0c1e91349ed6653a47a40d4fd55
# Parent  e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
libxl: Do not start stubdom when not needed

Do not start stubdom when there is any disk - only when there are disks
requiring it.

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

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -828,8 +828,14 @@
         goto out;
     }
 
-    if (nr_disks > 0 && !libxl__blktap_enabled(&gc))
-        ret = 1;
+    if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) {
+        for (i = 0; i < nr_disks; i++) {
+            if (disks[i].backend == DISK_BACKEND_TAP || disks[i].backend == DISK_BACKEND_QDISK) {
+                ret = 1;
+                goto out;
+            }
+        }
+    }
 
 out:
     libxl__free_all(&gc);

Re: [PATCH 02 of 10] libxl: Do not start stubdom when not needed

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962900 -7200
> # Node ID 59cff6b471f3f0c1e91349ed6653a47a40d4fd55
> # Parent  e3a3f5cc95349e92b7cb8b1448e999ffc16bd060
> libxl: Do not start stubdom when not needed
> 
> Do not start stubdom when there is any disk - only when there are disks
> requiring it.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

What tree is your series against? This same issue was fixed by
23044:d4ca456c0c25 in mid-March.

In general you need to post patches against a reasonably recent
xen-unstable. You can also recommend patches (either new ones or
existing commits in xen-unstable) for backporting to the x.y-testing
trees if you think that is necessary.

I though 23044:d4ca456c0c25 had already been backported to 4.1-testing
but it seems not, Keir can we do so please?

Ian.

> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -828,8 +828,14 @@
>          goto out;
>      }
>  
> -    if (nr_disks > 0 && !libxl__blktap_enabled(&gc))
> -        ret = 1;
> +    if (nr_disks > 0 && !libxl__blktap_enabled(&gc)) {
> +        for (i = 0; i < nr_disks; i++) {
> +            if (disks[i].backend == DISK_BACKEND_TAP || disks[i].backend == DISK_BACKEND_QDISK) {
> +                ret = 1;
> +                goto out;
> +            }
> +        }
> +    }
>  
>  out:
>      libxl__free_all(&gc);
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 03 of 10] 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 81df382694fd6e208255197d8da19bfe435a2cbd
# Parent  59cff6b471f3f0c1e91349ed6653a47a40d4fd55
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_utils.c b/tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -529,18 +529,18 @@
                                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((char *)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;
     }

Re: [PATCH 03 of 10] libxl: Accept disk name in libxl_devid_to_device_disk

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962929 -7200
> # Node ID 81df382694fd6e208255197d8da19bfe435a2cbd
> # Parent  59cff6b471f3f0c1e91349ed6653a47a40d4fd55
> 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_utils.c b/tools/libxl/libxl_utils.c
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -529,18 +529,18 @@
>                                 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((char *)devid);

This function takes 3 parameters in xen-unstable.hg so this won't work.
Please can you rebase.

Also casting away the const like that is probably unsafe (it's likely a
const for a reason) and should have raised alarm bells.

libxl__device_disk_dev_number should take a const, it looks like that
would be a trivial change since the function doesn't appear to actually
modify the string, although I could be mistaken, in which case
libxl__device_disk_dev_number needs fixing to not modify the string.

Cheers,
Ian.


> +    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;
>      }
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 04 of 10] 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 df639d3eef683460b6d5ab38296cbd90b26f60f0
# Parent  81df382694fd6e208255197d8da19bfe435a2cbd
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 @@
     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 = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)));
     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 = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)));
     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 04 of 10] libxl: Allocate memory for strings in libxl_device_disk

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962954 -7200
> # Node ID df639d3eef683460b6d5ab38296cbd90b26f60f0
> # Parent  81df382694fd6e208255197d8da19bfe435a2cbd
> 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 @@
>      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 = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)));

>      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 = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)));

I think it is acceptable to use xs_read() directly in both these cases
and avoid the need to strdup.

Ian.

>      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

[PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306962980 -7200
# Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
# Parent  df639d3eef683460b6d5ab38296cbd90b26f60f0
libxl: Set libxl_device_nic->domid when looking up by devid

Fixes xl network-detach with device specified by id, not MAC

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
@@ -512,6 +512,7 @@
     }
     nic->backend_domid = strtoul(val, NULL, 10);
     nic->devid = strtoul(devid, NULL, 10);
+    nic->domid = domid;
 
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe));
     for (i = 0, tok = strtok(val, ":"); tok && (i < 6);

Re: [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306962980 -7200
> # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
> # Parent  df639d3eef683460b6d5ab38296cbd90b26f60f0
> libxl: Set libxl_device_nic->domid when looking up by devid
> 
> Fixes xl network-detach with device specified by id, not MAC

Please rebase onto xen-unstable.hg, the libxl_device_* structures no
longer contain a domid in that tree, it is passed as a parameter to the
relevant functions now.

Ian.

> 
> 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
> @@ -512,6 +512,7 @@
>      }
>      nic->backend_domid = strtoul(val, NULL, 10);
>      nic->devid = strtoul(devid, NULL, 10);
> +    nic->domid = domid;
>  
>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe));
>      for (i = 0, tok = strtok(val, ":"); tok && (i < 6);
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid

[Marek Marczykowski]

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

On 03.06.2011 10:11, Ian Campbell wrote:
> On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
>> # HG changeset patch
>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>> # Date 1306962980 -7200
>> # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
>> # Parent  df639d3eef683460b6d5ab38296cbd90b26f60f0
>> libxl: Set libxl_device_nic->domid when looking up by devid
>>
>> Fixes xl network-detach with device specified by id, not MAC
> 
> Please rebase onto xen-unstable.hg, the libxl_device_* structures no
> longer contain a domid in that tree, it is passed as a parameter to the
> relevant functions now.

Ok, but in 4.1 tree still domid field exists and is used. So this should
be fixed somehow (by applying this patch, or by backporting some other
patches from unstable)...

> 
> Ian.
> 
>>
>> 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
>> @@ -512,6 +512,7 @@
>>      }
>>      nic->backend_domid = strtoul(val, NULL, 10);
>>      nic->devid = strtoul(devid, NULL, 10);
>> +    nic->domid = domid;
>>  
>>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe));
>>      for (i = 0, tok = strtok(val, ":"); tok && (i < 6);
>>
>>
>>
>> _______________________________________________
>> 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

Re: [PATCH 05 of 10] libxl: Set libxl_device_nic->domid when looking up by devid

[Ian Campbell]

On Sat, 2011-06-04 at 00:24 +0100, Marek Marczykowski wrote:
> On 03.06.2011 10:11, Ian Campbell wrote:
> > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1306962980 -7200
> >> # Node ID 3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
> >> # Parent  df639d3eef683460b6d5ab38296cbd90b26f60f0
> >> libxl: Set libxl_device_nic->domid when looking up by devid
> >>
> >> Fixes xl network-detach with device specified by id, not MAC
> > 
> > Please rebase onto xen-unstable.hg, the libxl_device_* structures no
> > longer contain a domid in that tree, it is passed as a parameter to the
> > relevant functions now.
> 
> Ok, but in 4.1 tree still domid field exists and is used. So this should
> be fixed somehow (by applying this patch, or by backporting some other
> patches from unstable)...

IMHO the patch which removes domid is not suitable for backporting
(since it changes libxl API). So this patch seems suitable for 4.1
directly, I think.

Thanks,Ian.

> 
> > 
> > Ian.
> > 
> >>
> >> 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
> >> @@ -512,6 +512,7 @@
> >>      }
> >>      nic->backend_domid = strtoul(val, NULL, 10);
> >>      nic->devid = strtoul(devid, NULL, 10);
> >> +    nic->domid = domid;
> >>  
> >>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/mac", nic_path_fe));
> >>      for (i = 0, tok = strtok(val, ":"); tok && (i < 6);
> >>
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xensource.com
> >> http://lists.xensource.com/xen-devel
> > 
> > 
> 
> 

[PATCH 06 of 10] xl: Allocate memory for libxl_device_nic string members

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306963069 -7200
# Node ID eb7216a75b7d7a5de93c717401b447545022b582
# Parent  3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
xl: Allocate memory for libxl_device_nic string members

Do not use argv directly - libxl_device_nic_destroy will try to free() it.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4280,12 +4280,14 @@
                 nic.mac[i] = val;
             }
         } else if (!strncmp("bridge=", *argv, 7)) {
-            nic.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)) {
-            nic.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)) {
                 fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
@@ -4293,9 +4295,11 @@
             }
             nic.backend_domid = val;
         } else if (!strncmp("vifname=", *argv, 8)) {
-            nic.ifname = (*argv) + 8;
+			free(nic.ifname);
+            nic.ifname = strdup((*argv) + 8);
         } else if (!strncmp("model=", *argv, 6)) {
-            nic.model = (*argv) + 6;
+            free(nic.model);
+            nic.model = strdup((*argv) + 6);
         } else if (!strncmp("rate=", *argv, 5)) {
         } else if (!strncmp("accel=", *argv, 6)) {
         } else {

Re: [PATCH 06 of 10] xl: Allocate memory for libxl_device_nic string members

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306963069 -7200
> # Node ID eb7216a75b7d7a5de93c717401b447545022b582
> # Parent  3e5e8eaf2fe8352e584e7498fde21d6e76c3475b
> xl: Allocate memory for libxl_device_nic string members
> 
> Do not use argv directly - libxl_device_nic_destroy will try to free() it.
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4280,12 +4280,14 @@
>                  nic.mac[i] = val;
>              }
>          } else if (!strncmp("bridge=", *argv, 7)) {
> -            nic.bridge = (*argv) + 7;
> +            free(nic.bridge);
> +            nic.bridge = strdup((*argv) + 7);

For parsing the cfg files (which has a similar pattern) we defined a
helper, xlu_cfg_replace_string. I think a similar helper for the non
xlu_cfg case (e.g. replace_string) would be good here too.

Ian.


>          } else if (!strncmp("ip=", *argv, 3)) {
>              free(nic.ip);
>              nic.ip = strdup((*argv) + 3);
>          } else if (!strncmp("script=", *argv, 6)) {
> -            nic.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)) {
>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
> @@ -4293,9 +4295,11 @@
>              }
>              nic.backend_domid = val;
>          } else if (!strncmp("vifname=", *argv, 8)) {
> -            nic.ifname = (*argv) + 8;
> +			free(nic.ifname);
> +            nic.ifname = strdup((*argv) + 8);
>          } else if (!strncmp("model=", *argv, 6)) {
> -            nic.model = (*argv) + 6;
> +            free(nic.model);
> +            nic.model = strdup((*argv) + 6);
>          } else if (!strncmp("rate=", *argv, 5)) {
>          } else if (!strncmp("accel=", *argv, 6)) {
>          } else {
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH 07 of 10] xl: Fix 'script' param parsing in network-attach

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306963105 -7200
# Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0
# Parent  eb7216a75b7d7a5de93c717401b447545022b582
xl: Fix 'script' param parsing in network-attach

Fix 'script=' string length

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4285,9 +4285,9 @@
         } else if (!strncmp("ip=", *argv, 3)) {
             free(nic.ip);
             nic.ip = strdup((*argv) + 3);
-        } else if (!strncmp("script=", *argv, 6)) {
+        } else if (!strncmp("script=", *argv, 7)) {
             free(nic.script);
-            nic.script = strdup((*argv) + 6);
+            nic.script = strdup((*argv) + 7);
         } else if (!strncmp("backend=", *argv, 8)) {
             if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
                 fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");

Re: [PATCH 07 of 10] xl: Fix 'script' param parsing in network-attach

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306963105 -7200
> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0
> # Parent  eb7216a75b7d7a5de93c717401b447545022b582
> xl: Fix 'script' param parsing in network-attach
> 
> Fix 'script=' string length
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4285,9 +4285,9 @@
>          } else if (!strncmp("ip=", *argv, 3)) {
>              free(nic.ip);
>              nic.ip = strdup((*argv) + 3);
> -        } else if (!strncmp("script=", *argv, 6)) {
> +        } else if (!strncmp("script=", *argv, 7)) {
>              free(nic.script);
> -            nic.script = strdup((*argv) + 6);
> +            nic.script = strdup((*argv) + 7);

Good catch.

The pre-existing use of all those
	strncmp(A, *argv, open-coded-sizeof(A))
must be a source of many such errors. A helper function (or macro) is
probably the way to go. Do you fancy coding that up?

Ian.

>          } else if (!strncmp("backend=", *argv, 8)) {
>              if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 07 of 10] xl: Fix 'script' param parsing in network-attach

[Marek Marczykowski]

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

On 03.06.2011 10:16, Ian Campbell wrote:
> On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
>> # HG changeset patch
>> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
>> # Date 1306963105 -7200
>> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0
>> # Parent  eb7216a75b7d7a5de93c717401b447545022b582
>> xl: Fix 'script' param parsing in network-attach
>>
>> Fix 'script=' string length
>>
>> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
>>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -4285,9 +4285,9 @@
>>          } else if (!strncmp("ip=", *argv, 3)) {
>>              free(nic.ip);
>>              nic.ip = strdup((*argv) + 3);
>> -        } else if (!strncmp("script=", *argv, 6)) {
>> +        } else if (!strncmp("script=", *argv, 7)) {
>>              free(nic.script);
>> -            nic.script = strdup((*argv) + 6);
>> +            nic.script = strdup((*argv) + 7);
> 
> Good catch.
> 
> The pre-existing use of all those
> 	strncmp(A, *argv, open-coded-sizeof(A))
> must be a source of many such errors. A helper function (or macro) is
> probably the way to go. Do you fancy coding that up?

And the same size must be used in strdup offset...
Maybe something like this (solving also problem in 06 patch):
-----------
#define COMPARE_AND_REPLACE(pattern, dest) \
    } else if (!strncmp(pattern, *argv, sizeof(pattern))) { \
        free(dest); \
        dest = strdup((*argv) + sizeof(pattern));

(...)
    COMPARE_AND_REPLACE("script=", nic.script)
    COMPARE_AND_REPLACE("ip=", nic.ip)
---------
Looks weird because of no semicolon at the end of lines, but should
works. Unfortunately cannot be used in all places (like one below)...
What do you think?

> 
> Ian.
> 
>>          } else if (!strncmp("backend=", *argv, 8)) {
>>              if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
>>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");



-- 
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: [PATCH 07 of 10] xl: Fix 'script' param parsing in network-attach

[Ian Campbell]

On Sat, 2011-06-04 at 00:49 +0100, Marek Marczykowski wrote:
> On 03.06.2011 10:16, Ian Campbell wrote:
> > On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> >> # HG changeset patch
> >> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> >> # Date 1306963105 -7200
> >> # Node ID 6811aa543e69379557ff7391ea3db8a5e7f7dde0
> >> # Parent  eb7216a75b7d7a5de93c717401b447545022b582
> >> xl: Fix 'script' param parsing in network-attach
> >>
> >> Fix 'script=' string length
> >>
> >> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>
> >>
> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >> --- a/tools/libxl/xl_cmdimpl.c
> >> +++ b/tools/libxl/xl_cmdimpl.c
> >> @@ -4285,9 +4285,9 @@
> >>          } else if (!strncmp("ip=", *argv, 3)) {
> >>              free(nic.ip);
> >>              nic.ip = strdup((*argv) + 3);
> >> -        } else if (!strncmp("script=", *argv, 6)) {
> >> +        } else if (!strncmp("script=", *argv, 7)) {
> >>              free(nic.script);
> >> -            nic.script = strdup((*argv) + 6);
> >> +            nic.script = strdup((*argv) + 7);
> > 
> > Good catch.
> > 
> > The pre-existing use of all those
> > 	strncmp(A, *argv, open-coded-sizeof(A))
> > must be a source of many such errors. A helper function (or macro) is
> > probably the way to go. Do you fancy coding that up?
> 
> And the same size must be used in strdup offset...
> Maybe something like this (solving also problem in 06 patch):
> -----------
> #define COMPARE_AND_REPLACE(pattern, dest) \
>     } else if (!strncmp(pattern, *argv, sizeof(pattern))) { \
>         free(dest); \
>         dest = strdup((*argv) + sizeof(pattern));
> 
> (...)
>     COMPARE_AND_REPLACE("script=", nic.script)
>     COMPARE_AND_REPLACE("ip=", nic.ip)
> ---------
> Looks weird because of no semicolon at the end of lines, but should
> works.

Generally you can fix the semi-colon thing by omitting the last one from
the macro itself.

> Unfortunately cannot be used in all places (like one below)...
> What do you think?

How about (untested), requires a local "char *oparg":

static int match_option_size(const char *prefix, size_t len,
                             char *arg, char **argopt)
{
    int rc = strncmp(prefix, argv, len);
    if (!rc) *argopt = argv+len;
    return !rc
}
#define match_option(_prefix, _arg, _oparg) \
    match_option_size((_prefix), (_arg), sizeof((_prefix)), &(_oparg));

static void replace_string(char **str, const char *val)
{
    free(*str);\
    *str = strdup(val);
}

then you have:

    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");
            ...
        }
    }

with a bit of macro pasting in match_option you can probably push the
"=" down into the macro so each callsite doesn't need it.

Ian.

> 
> > 
> > Ian.
> > 
> >>          } else if (!strncmp("backend=", *argv, 8)) {
> >>              if(libxl_name_to_domid(&ctx, ((*argv) + 8), &val)) {
> >>                  fprintf(stderr, "Specified backend domain does not exist, defaulting to Dom0\n");
> 
> 
> 

[PATCH 08 of 10] xen.lowlevel.xl: Return None on empty domain name

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306963128 -7200
# Node ID d03bd7a830bff8162b6b0564d27b3f36ca52d8bb
# Parent  6811aa543e69379557ff7391ea3db8a5e7f7dde0
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
@@ -418,8 +418,11 @@
         return NULL;
 
     domname = libxl_domid_to_name(&self->ctx, domid);
-    ret = PyString_FromString(domname);
-    free(domname);
+    if (domname) {
+        ret = PyString_FromString(domname);
+        free(domname);
+    } else
+        return Py_None;
 
     return ret;
 }

Re: [PATCH 08 of 10] xen.lowlevel.xl: Return None on empty domain name

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306963128 -7200
> # Node ID d03bd7a830bff8162b6b0564d27b3f36ca52d8bb
> # Parent  6811aa543e69379557ff7391ea3db8a5e7f7dde0
> 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
> @@ -418,8 +418,11 @@
>          return NULL;
>  
>      domname = libxl_domid_to_name(&self->ctx, domid);
> -    ret = PyString_FromString(domname);
> -    free(domname);
> +    if (domname) {
> +        ret = PyString_FromString(domname);
> +        free(domname);
> +    } else
> +        return Py_None;

Please do
           ret = Py_None;
instead. Having a mixture the single return location and individual
returns isn't nice.

Personally I'd just initialise ret to Py_None in the first place and
omit the else.

Ian.

>  
>      return ret;
>  }
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306963174 -7200
# Node ID 10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403
# Parent  d03bd7a830bff8162b6b0564d27b3f36ca52d8bb
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
@@ -1722,6 +1722,7 @@
                              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;
@@ -1740,7 +1741,11 @@
                 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

[PATCH 10 of 10] libxl: "script:" prefix in block device description to setup vbd by hotplug scripts

[Marek Marczykowski]

# HG changeset patch
# User Marek Marczykowski <marmarek@mimuw.edu.pl>
# Date 1306963204 -7200
# Node ID 1e3bf3cb3944402e01c57c400a02fe4293153458
# Parent  10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403
libxl: "script:" prefix in block device description to setup vbd by hotplug scripts

Implements old behaviour of block-attach - only write parameters to xenstore
and left all work for hotplug scripts. Allows to directly reuse xen block
scripts (and maybe some custom ones). Example use:

xl block-attach vm "script:nbd 1.1.1.1 9999" xvdg w

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
@@ -896,6 +896,13 @@
         }
     }
 
+    if (disk->backend == DISK_BACKEND_SCRIPT) {
+        delimiter = strchr(file_name, ':');
+        if (!delimiter)
+            return ERROR_INVAL;
+        return 0;
+    }
+
     if ( stat(file_name, &stat_buf) != 0 ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
         return ERROR_INVAL;
@@ -965,6 +972,24 @@
 
             device.backend_kind = DEVICE_VBD;
             break;
+        case DISK_BACKEND_SCRIPT:
+			{
+				char *delimiter;
+				backend_type = libxl__strdup(&gc, disk->pdev_path);
+				delimiter = strchr(backend_type, ':');
+				if (!delimiter) {
+					rc = ERROR_FAIL;
+					goto out_free;
+				}
+				*delimiter = '\0';
+				flexarray_append(back, "params");
+				flexarray_append(back, delimiter+1);
+                flexarray_append(back, "scripted");
+                flexarray_append(back, "1");
+
+				device.backend_kind = DEVICE_VBD;
+			}
+            break;
         case DISK_BACKEND_TAP:
             if (libxl__blktap_enabled(&gc) && disk->format != DISK_FORMAT_EMPTY) {
                 const char *dev = libxl__blktap_devpath(&gc,
@@ -1060,7 +1085,7 @@
     device.backend_domid    = disk->backend_domid;
     device.backend_devid    = devid;
     device.backend_kind     = 
-        (disk->backend == DISK_BACKEND_PHY) ? DEVICE_VBD : DEVICE_TAP;
+        (disk->backend == DISK_BACKEND_PHY || disk->backend == DISK_BACKEND_SCRIPT) ? DEVICE_VBD : DEVICE_TAP;
     device.domid            = disk->domid;
     device.devid            = devid;
     device.kind             = DEVICE_VBD;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -185,6 +185,7 @@
     DISK_BACKEND_PHY,
     DISK_BACKEND_TAP,
     DISK_BACKEND_QDISK,
+    DISK_BACKEND_SCRIPT,
 } libxl_disk_backend;
 
 typedef enum {
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
@@ -139,6 +139,7 @@
         case DISK_BACKEND_QDISK: return "qdisk";
         case DISK_BACKEND_TAP: return "tap";
         case DISK_BACKEND_PHY: return "phy";
+        case DISK_BACKEND_SCRIPT: return "script";
         default: return NULL;
     }
 }
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
@@ -282,6 +282,8 @@
 
     if (!strcmp(s, "phy")) {
         *backend = DISK_BACKEND_PHY;
+    } else if (!strcmp(s, "script")) {
+        *backend = DISK_BACKEND_SCRIPT;
     } else if (!strcmp(s, "file")) {
         *backend = DISK_BACKEND_TAP;
     } else if (!strcmp(s, "tap")) {
@@ -553,8 +555,13 @@
     disk->domid = domid;
     be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
     disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)));
-    val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
-    libxl_string_to_backend(ctx, val, &(disk->backend));
+    val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/scripted", be_path));
+    if (val && atoi(val))
+        disk->backend = DISK_BACKEND_SCRIPT;
+    else {
+        val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
+        libxl_string_to_backend(ctx, val, &(disk->backend));
+    }
     disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)));
     val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
     disk->unpluggable = !strcmp(val, "1");
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -481,6 +481,10 @@
                     state = DSTATE_PHYSPATH;
                     disk->format = DISK_FORMAT_RAW;
                     disk->backend = DISK_BACKEND_PHY;
+                }else if ( !strcmp(tok, "script") ) {
+                    state = DSTATE_PHYSPATH;
+                    disk->format = DISK_FORMAT_RAW;
+                    disk->backend = DISK_BACKEND_SCRIPT;
                 }else if ( !strcmp(tok, "file") ) {
                     state = DSTATE_PHYSPATH;
                     disk->format = DISK_FORMAT_RAW;
@@ -4435,6 +4439,8 @@
     tok = strtok(argv[optind+1], ":");
     if (!strcmp(tok, "phy")) {
         disk.backend = DISK_BACKEND_PHY;
+    } else if (!strcmp(tok, "script")) {
+        disk.backend = DISK_BACKEND_SCRIPT;
     } else if (!strcmp(tok, "file")) {
         disk.backend = DISK_BACKEND_TAP;
     } else if (!strcmp(tok, "tap")) {

Re: [PATCH 10 of 10] libxl: "script:" prefix in block device description to setup vbd by hotplug scripts

[Ian Campbell]

On Thu, 2011-06-02 at 23:35 +0100, Marek Marczykowski wrote:
> # HG changeset patch
> # User Marek Marczykowski <marmarek@mimuw.edu.pl>
> # Date 1306963204 -7200
> # Node ID 1e3bf3cb3944402e01c57c400a02fe4293153458
> # Parent  10bdf8aa5b8a1cb2178f2b9cbcc9ce2fd5644403
> libxl: "script:" prefix in block device description to setup vbd by hotplug scripts
> 
> Implements old behaviour of block-attach - only write parameters to xenstore
> and left all work for hotplug scripts. Allows to directly reuse xen block
> scripts (and maybe some custom ones). Example use:
> 
> xl block-attach vm "script:nbd 1.1.1.1 9999" xvdg w
> 
> Signed-off-by: Marek Marczykowski <marmarek@mimuw.edu.pl>

I didn't review this one, I think it probably interacts badly with (or
more likely is obsoleted by) Ian Jackson's recent disk parsing cleanup
series.

Ian.

> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -896,6 +896,13 @@
>          }
>      }
>  
> +    if (disk->backend == DISK_BACKEND_SCRIPT) {
> +        delimiter = strchr(file_name, ':');
> +        if (!delimiter)
> +            return ERROR_INVAL;
> +        return 0;
> +    }
> +
>      if ( stat(file_name, &stat_buf) != 0 ) {
>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
>          return ERROR_INVAL;
> @@ -965,6 +972,24 @@
>  
>              device.backend_kind = DEVICE_VBD;
>              break;
> +        case DISK_BACKEND_SCRIPT:
> +			{
> +				char *delimiter;
> +				backend_type = libxl__strdup(&gc, disk->pdev_path);
> +				delimiter = strchr(backend_type, ':');
> +				if (!delimiter) {
> +					rc = ERROR_FAIL;
> +					goto out_free;
> +				}
> +				*delimiter = '\0';
> +				flexarray_append(back, "params");
> +				flexarray_append(back, delimiter+1);
> +                flexarray_append(back, "scripted");
> +                flexarray_append(back, "1");
> +
> +				device.backend_kind = DEVICE_VBD;
> +			}
> +            break;
>          case DISK_BACKEND_TAP:
>              if (libxl__blktap_enabled(&gc) && disk->format != DISK_FORMAT_EMPTY) {
>                  const char *dev = libxl__blktap_devpath(&gc,
> @@ -1060,7 +1085,7 @@
>      device.backend_domid    = disk->backend_domid;
>      device.backend_devid    = devid;
>      device.backend_kind     = 
> -        (disk->backend == DISK_BACKEND_PHY) ? DEVICE_VBD : DEVICE_TAP;
> +        (disk->backend == DISK_BACKEND_PHY || disk->backend == DISK_BACKEND_SCRIPT) ? DEVICE_VBD : DEVICE_TAP;
>      device.domid            = disk->domid;
>      device.devid            = devid;
>      device.kind             = DEVICE_VBD;
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -185,6 +185,7 @@
>      DISK_BACKEND_PHY,
>      DISK_BACKEND_TAP,
>      DISK_BACKEND_QDISK,
> +    DISK_BACKEND_SCRIPT,
>  } libxl_disk_backend;
>  
>  typedef enum {
> 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
> @@ -139,6 +139,7 @@
>          case DISK_BACKEND_QDISK: return "qdisk";
>          case DISK_BACKEND_TAP: return "tap";
>          case DISK_BACKEND_PHY: return "phy";
> +        case DISK_BACKEND_SCRIPT: return "script";
>          default: return NULL;
>      }
>  }
> 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
> @@ -282,6 +282,8 @@
>  
>      if (!strcmp(s, "phy")) {
>          *backend = DISK_BACKEND_PHY;
> +    } else if (!strcmp(s, "script")) {
> +        *backend = DISK_BACKEND_SCRIPT;
>      } else if (!strcmp(s, "file")) {
>          *backend = DISK_BACKEND_TAP;
>      } else if (!strcmp(s, "tap")) {
> @@ -553,8 +555,13 @@
>      disk->domid = domid;
>      be_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/backend", diskpath));
>      disk->pdev_path = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/params", be_path)));
> -    val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
> -    libxl_string_to_backend(ctx, val, &(disk->backend));
> +    val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/scripted", be_path));
> +    if (val && atoi(val))
> +        disk->backend = DISK_BACKEND_SCRIPT;
> +    else {
> +        val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/type", be_path));
> +        libxl_string_to_backend(ctx, val, &(disk->backend));
> +    }
>      disk->vdev = strdup(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/dev", be_path)));
>      val = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/removable", be_path));
>      disk->unpluggable = !strcmp(val, "1");
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -481,6 +481,10 @@
>                      state = DSTATE_PHYSPATH;
>                      disk->format = DISK_FORMAT_RAW;
>                      disk->backend = DISK_BACKEND_PHY;
> +                }else if ( !strcmp(tok, "script") ) {
> +                    state = DSTATE_PHYSPATH;
> +                    disk->format = DISK_FORMAT_RAW;
> +                    disk->backend = DISK_BACKEND_SCRIPT;
>                  }else if ( !strcmp(tok, "file") ) {
>                      state = DSTATE_PHYSPATH;
>                      disk->format = DISK_FORMAT_RAW;
> @@ -4435,6 +4439,8 @@
>      tok = strtok(argv[optind+1], ":");
>      if (!strcmp(tok, "phy")) {
>          disk.backend = DISK_BACKEND_PHY;
> +    } else if (!strcmp(tok, "script")) {
> +        disk.backend = DISK_BACKEND_SCRIPT;
>      } else if (!strcmp(tok, "file")) {
>          disk.backend = DISK_BACKEND_TAP;
>      } else if (!strcmp(tok, "tap")) {
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel