[PATCH V2 0/5] libxl: Some fixes related to Qemu upstream command lines.

[PATCH V2 0/5] libxl: Some fixes related to Qemu upstream command lines.

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

This series comes with some fixes for hvm guest with Qemu upstream, and completes
the function libxl_build_device_model_args_new to make XenPV guest works with
the Qemu upstream (when it's needed).

Anthony PERARD (5):
  libxl: fix double free of ifname, when makes args for qemu.
  libxl: strdup disk path before put it in qemu args array.
  libxl: Specify the target ram size to Qemu (new) when calling it
  libxl: Makes libxl be able to call Qemu upstream for XenPV guest.
  libxl: Lists qdisk device in libxl_device_disk_list

 tools/libxl/libxl.c      |   78 +++++++++++++++++++++++++++++++++++++---------
 tools/libxl/libxl.idl    |    1 +
 tools/libxl/xl_cmdimpl.c |    1 +
 3 files changed, 65 insertions(+), 15 deletions(-)

[PATCH V2 1/5] libxl: fix double free of ifname, when makes args for qemu.

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

In libxl_build_device_model_args_new, vifs[i].ifname can be free two
times, by the gc, and by freeing the vifs structures. This patch avoids
this.
---
 tools/libxl/libxl.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 33e5a2a..0feb93f 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1341,14 +1341,18 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
                 char *smac = libxl__sprintf(gc, "%02x:%02x:%02x:%02x:%02x:%02x",
                                            vifs[i].mac[0], vifs[i].mac[1], vifs[i].mac[2],
                                            vifs[i].mac[3], vifs[i].mac[4], vifs[i].mac[5]);
-                if (!vifs[i].ifname)
-                    vifs[i].ifname = libxl__sprintf(gc, "tap%d.%d", info->domid, vifs[i].devid);
+                char *ifname;
+                if (!vifs[i].ifname) {
+                    ifname = libxl__sprintf(gc, "tap%d.%d", info->domid, vifs[i].devid);
+                } else {
+                    ifname = vifs[i].ifname;
+                }
                 flexarray_set(dm_args, num++, "-net");
                 flexarray_set(dm_args, num++, libxl__sprintf(gc, "nic,vlan=%d,macaddr=%s,model=%s",
                             vifs[i].devid, smac, vifs[i].model));
                 flexarray_set(dm_args, num++, "-net");
                 flexarray_set(dm_args, num++, libxl__sprintf(gc, "tap,vlan=%d,ifname=%s,script=no",
-                            vifs[i].devid, vifs[i].ifname));
+                            vifs[i].devid, ifname));
                 ioemu_vifs++;
             }
         }
-- 
1.7.1

[PATCH V2 2/5] libxl: strdup disk path before put it in qemu args array.

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

In libxl_build_device_model_args_new, the path to the disk image are
freeed before there was actually use to make the arguments list of Qemu.
The patch strdups it.

This patch also changes argv[0] of the device model.
Now, it is the conventional argv[0], so is value come from
info->device_model.
---
 tools/libxl/libxl.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 0feb93f..18ea8b1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1256,9 +1256,9 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     if (!dm_args)
         return NULL;
 
-    flexarray_set(dm_args, num++, "qemu-system-xen");
-    flexarray_set(dm_args, num++, "-xen-domid");
+    flexarray_set(dm_args, num++, libxl__strdup(gc, info->device_model));
 
+    flexarray_set(dm_args, num++, "-xen-domid");
     flexarray_set(dm_args, num++, libxl__sprintf(gc, "%d", info->domid));
 
     if (info->dom_name) {
@@ -1378,10 +1378,10 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     for (i; i < nb; i++) {
         if ( disks[i].is_cdrom ) {
             flexarray_set(dm_args, num++, "-cdrom");
-            flexarray_set(dm_args, num++, disks[i].physpath);
-        }else{
+            flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
+        } else {
             flexarray_set(dm_args, num++, libxl__sprintf(gc, "-%s", disks[i].virtpath));
-            flexarray_set(dm_args, num++, disks[i].physpath);
+            flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
         }
         libxl_device_disk_destroy(&disks[i]);
     }
-- 
1.7.1

[PATCH V2 3/5] libxl: Specify the target ram size to Qemu (new) when calling it

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

This patch adds target_ram in device_model_info structure, to be used in
libxl_build_device_model_args_new. Qemu upstream needs to know about it.
---
 tools/libxl/libxl.c      |    4 ++++
 tools/libxl/libxl.idl    |    1 +
 tools/libxl/xl_cmdimpl.c |    1 +
 3 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 18ea8b1..f4fb2c6 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1374,6 +1374,10 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     else
         flexarray_set(dm_args, num++, "xenfv");
 
+    /* RAM Size */
+    flexarray_set(dm_args, num++, "-m");
+    flexarray_set(dm_args, num++, libxl__sprintf(gc, "%d", info->target_ram));
+
     disks = libxl_device_disk_list(libxl__gc_owner(gc), info->domid, &nb);
     for (i; i < nb; i++) {
         if ( disks[i].is_cdrom ) {
diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
index 8dd7749..89694b1 100644
--- a/tools/libxl/libxl.idl
+++ b/tools/libxl/libxl.idl
@@ -139,6 +139,7 @@ libxl_device_model_info = Struct("device_model_info",[
     ("device_model",     string),
     ("saved_state",      string),
     ("type",             libxl_qemu_machine_type),
+    ("target_ram",       uint32),
     ("videoram",         integer,           False, "size of the videoram in MB"),
     ("stdvga",           bool,              False, "stdvga enabled or disabled"),
     ("vnc",              bool,              False, "vnc enabled or disabled"),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5555319..897c5bc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -359,6 +359,7 @@ static void init_dm_info(libxl_device_model_info *dm_info,
 
     dm_info->dom_name = strdup(c_info->name);
     dm_info->device_model = strdup("qemu-dm");
+    dm_info->target_ram = b_info->target_memkb / 1024;
     dm_info->videoram = b_info->video_memkb / 1024;
     dm_info->apic = b_info->u.hvm.apic;
     dm_info->vcpus = b_info->max_vcpus;
-- 
1.7.1

[PATCH V2 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest.

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

In libxl_build_device_model_args_new:
  - Adds -xen-attach options to the list of arguments to Qemu.
  - Adds -vga xenfb options when vnc and sdl are not set.
  - Remove disk list from the command line for XenPV as they will be
    read from xenstore by Qemu.
---
 tools/libxl/libxl.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index f4fb2c6..57f7051 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1261,6 +1261,10 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     flexarray_set(dm_args, num++, "-xen-domid");
     flexarray_set(dm_args, num++, libxl__sprintf(gc, "%d", info->domid));
 
+    if (info->type == XENPV) {
+        flexarray_set(dm_args, num++, "-xen-attach");
+    }
+
     if (info->dom_name) {
         flexarray_set(dm_args, num++, "-name");
         flexarray_set(dm_args, num++, info->dom_name);
@@ -1292,6 +1296,12 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     if (info->sdl) {
         flexarray_set(dm_args, num++, "-sdl");
     }
+
+    if (info->type == XENPV && !info->nographic) {
+        flexarray_set(dm_args, num++, "-vga");
+        flexarray_set(dm_args, num++, "xenfb");
+    }
+
     if (info->keymap) {
         flexarray_set(dm_args, num++, "-k");
         flexarray_set(dm_args, num++, info->keymap);
@@ -1378,16 +1388,18 @@ static char ** libxl_build_device_model_args_new(libxl__gc *gc,
     flexarray_set(dm_args, num++, "-m");
     flexarray_set(dm_args, num++, libxl__sprintf(gc, "%d", info->target_ram));
 
-    disks = libxl_device_disk_list(libxl__gc_owner(gc), info->domid, &nb);
-    for (i; i < nb; i++) {
-        if ( disks[i].is_cdrom ) {
-            flexarray_set(dm_args, num++, "-cdrom");
-            flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
-        } else {
-            flexarray_set(dm_args, num++, libxl__sprintf(gc, "-%s", disks[i].virtpath));
-            flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
+    if (info->type == XENFV) {
+        disks = libxl_device_disk_list(libxl__gc_owner(gc), info->domid, &nb);
+        for (i; i < nb; i++) {
+            if (disks[i].is_cdrom) {
+                flexarray_set(dm_args, num++, "-cdrom");
+                flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
+            } else {
+                flexarray_set(dm_args, num++, libxl__sprintf(gc, "-%s", disks[i].virtpath));
+                flexarray_set(dm_args, num++, libxl__strdup(gc, disks[i].physpath));
+            }
+            libxl_device_disk_destroy(&disks[i]);
         }
-        libxl_device_disk_destroy(&disks[i]);
     }
     free(disks);
     flexarray_set(dm_args, num++, NULL);
-- 
1.7.1

[PATCH V2 5/5] libxl: Lists qdisk device in libxl_device_disk_list

[anthony.perard]

From: Anthony PERARD <anthony.perard@citrix.com>

As libxl switch to qdisk when blktap isn't available, this patch makes
libxl_device_disk_list also list qdisk device. So
libxl_build_device_model_args_new will be able to add qdisk device to
the command line options of Qemu.
---
 tools/libxl/libxl.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 57f7051..d4bf734 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2492,7 +2492,7 @@ int libxl_device_vkb_hard_shutdown(libxl_ctx *ctx, uint32_t domid)
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)
 {
     libxl__gc gc = LIBXL_INIT_GC(ctx);
-    char *be_path_tap, *be_path_vbd;
+    char *be_path_tap, *be_path_vbd, *be_path_qdisk;
     libxl_device_disk *dend, *disks, *ret = NULL;
     char **b, **l = NULL;
     unsigned int numl, len;
@@ -2500,6 +2500,7 @@ libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *n
 
     be_path_vbd = libxl__sprintf(&gc, "%s/backend/vbd/%d", libxl__xs_get_dompath(&gc, 0), domid);
     be_path_tap = libxl__sprintf(&gc, "%s/backend/tap/%d", libxl__xs_get_dompath(&gc, 0), domid);
+    be_path_qdisk = libxl__sprintf(&gc, "%s/backend/qdisk/%d", libxl__xs_get_dompath(&gc, 0), domid);
 
     b = l = libxl__xs_directory(&gc, XBT_NULL, be_path_vbd, &numl);
     if (l) {
@@ -2542,6 +2543,33 @@ libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *n
             disks->is_cdrom = !strcmp(type, "cdrom");
         }
     }
+    b = l = libxl__xs_directory(&gc, XBT_NULL, be_path_qdisk, &numl);
+    if (l) {
+        ret = realloc(ret, sizeof(libxl_device_disk) * (*num + numl));
+        disks = ret + *num;
+        *num += numl;
+        for (dend = ret + *num; disks < dend; ++disks, ++l) {
+            char *physpath_tmp;
+            disks->backend_domid = 0;
+            disks->domid = domid;
+            physpath_tmp = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/params", be_path_qdisk, *l), &len);
+            if (strchr(physpath_tmp, ':')) {
+                disks->physpath = strdup(strchr(physpath_tmp) + 1);
+                free(physpath_tmp);
+            } else {
+                disks->physpath = physpath_tmp;
+            }
+            libxl_string_to_phystype(ctx, libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/type", be_path_qdisk, *l)), &(disks->phystype));
+            disks->virtpath = xs_read(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "%s/%s/dev", be_path_qdisk, *l), &len);
+            disks->unpluggable = atoi(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/removable", be_path_qdisk, *l)));
+            if (!strcmp(libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/mode", be_path_qdisk, *l)), "w"))
+                disks->readwrite = 1;
+            else
+                disks->readwrite = 0;
+            type = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/device-type", libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/%s/frontend", be_path_qdisk, *l))));
+            disks->is_cdrom = !strcmp(type, "cdrom");
+        }
+    }
     libxl__free_all(&gc);
     return ret;
 }
-- 
1.7.1

Re: [PATCH V2 3/5] libxl: Specify the target ram size to Qemu (new) when calling it

[Ian Campbell]

On Mon, 2010-12-13 at 13:15 +0000, anthony.perard@citrix.com wrote:
> 
> +    dm_info->target_ram = b_info->target_memkb / 1024;
>      dm_info->videoram = b_info->video_memkb / 1024; 

Both of these end up rounding down, is that desirable?

Ian.

Re: [PATCH V2 3/5] libxl: Specify the target ram size to Qemu (new) when calling it

[Anthony PERARD]

On Mon, 13 Dec 2010, Ian Campbell wrote:

> On Mon, 2010-12-13 at 13:15 +0000, anthony.perard@citrix.com wrote:
> >
> > +    dm_info->target_ram = b_info->target_memkb / 1024;
> >      dm_info->videoram = b_info->video_memkb / 1024;
>
> Both of these end up rounding down, is that desirable?

They are both multiplied by 1024 from the config file before they are
stored in the b_info->*_memkb variables. So have them rounding down or
up will not change anything.

-- 
Anthony PERARD

Re: [PATCH V2 3/5] libxl: Specify the target ram size to Qemu (new) when calling it

[Ian Campbell]

On Mon, 2010-12-13 at 14:14 +0000, Anthony Perard wrote:
> On Mon, 13 Dec 2010, Ian Campbell wrote:
> 
> > On Mon, 2010-12-13 at 13:15 +0000, anthony.perard@citrix.com wrote:
> > >
> > > +    dm_info->target_ram = b_info->target_memkb / 1024;
> > >      dm_info->videoram = b_info->video_memkb / 1024;
> >
> > Both of these end up rounding down, is that desirable?
> 
> They are both multiplied by 1024 from the config file before they are
> stored in the b_info->*_memkb variables. So have them rounding down or
> up will not change anything.

Fair enough. 

Makes me wonder if b_info->foo_memkb has the correct units. Also since
libxl is supposed to be usable by other than xl relying on particular
subtle behaviour like this seems unwise. This patch does make things any
worse though I guess.

Ian.

Re: [PATCH V2 3/5] libxl: Specify the target ram size to Qemu (new) when calling it

[Stefano Stabellini]

On Mon, 13 Dec 2010, Ian Campbell wrote:
> On Mon, 2010-12-13 at 14:14 +0000, Anthony Perard wrote:
> > On Mon, 13 Dec 2010, Ian Campbell wrote:
> > 
> > > On Mon, 2010-12-13 at 13:15 +0000, anthony.perard@citrix.com wrote:
> > > >
> > > > +    dm_info->target_ram = b_info->target_memkb / 1024;
> > > >      dm_info->videoram = b_info->video_memkb / 1024;
> > >
> > > Both of these end up rounding down, is that desirable?
> > 
> > They are both multiplied by 1024 from the config file before they are
> > stored in the b_info->*_memkb variables. So have them rounding down or
> > up will not change anything.
> 
> Fair enough. 
> 
> Makes me wonder if b_info->foo_memkb has the correct units. Also since
> libxl is supposed to be usable by other than xl relying on particular
> subtle behaviour like this seems unwise. This patch does make things any
> worse though I guess.

I think we should define a macro or a simple inline function to do kb to
mb conversions, rounding up the result.

Re: [PATCH V2 5/5] libxl: Lists qdisk device in libxl_device_disk_list

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Mon, 13 Dec 2010, anthony.perard@citrix.com wrote:
> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> As libxl switch to qdisk when blktap isn't available, this patch makes
> libxl_device_disk_list also list qdisk device. So
> libxl_build_device_model_args_new will be able to add qdisk device to
> the command line options of Qemu.

This patch fails to compile:

libxl.c:2553:66: error: macro "strchr" requires 2 arguments, but only 1 given
cc1: warnings being treated as errors
libxl.c: In function ‘libxl_device_disk_list’:
libxl.c:2553: error: passing argument 1 of ‘strlen’ from incompatible pointer type
/usr/include/string.h:397: note: expected ‘const char *’ but argument is of type ‘char * (*)(const char *, int)’
libxl.c:2553: error: passing argument 1 of ‘__strdup’ from incompatible pointer type
/usr/include/bits/string2.h:1303: note: expected ‘const char *’ but argument is of type ‘char * (*)(const char *, int)’


I have applied patch 1, 2 and 4.
Next time please add an explicit signed-off-by line to your patches.

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

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