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

[PATCH 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(-)

-- 
Anthony PERARD

[PATCH 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 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.
---
 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..0f6e632 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++, "qemu-dm");
 
+    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

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

[Ian Campbell]

On Thu, 2010-12-09 at 19:42 +0000, anthony.perard@citrix.com wrote:
> 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.
> ---
>  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..0f6e632 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++, "qemu-dm");

This bit doesn't seem to correspond to the patch description, was the
change deliberate or did it slip in by mistake?

(my kingdom for flexarray_append(...))

Ian.

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

[Anthony PERARD]

On Fri, 10 Dec 2010, Ian Campbell wrote:

> On Thu, 2010-12-09 at 19:42 +0000, anthony.perard@citrix.com wrote:
> > 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.
> > ---
> >  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..0f6e632 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++, "qemu-dm");
>
> This bit doesn't seem to correspond to the patch description, was the
> change deliberate or did it slip in by mistake?

This change was deliberate. As this hunk don't really change anything, I
just don't explain it in the commit message. But I should have said the
following in the description.

Also it changes argv[0] of the device model from "qemu-system-xen" to
"qemu-dm".

I will resend the patch.

> (my kingdom for flexarray_append(...))

It will be too easy with this functions :)

-- 
Anthony PERARD

[PATCH 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.

Also it changes argv[0] of the device model from "qemu-system-xen" to
"qemu-dm".

---
 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..0f6e632 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++, "qemu-dm");
 
+    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

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

[Ian Campbell]

On Fri, 2010-12-10 at 12:29 +0000, Anthony Perard wrote:
> On Fri, 10 Dec 2010, Ian Campbell wrote:
> 
> > On Thu, 2010-12-09 at 19:42 +0000, anthony.perard@citrix.com wrote:
> > >
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 0feb93f..0f6e632 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++, "qemu-dm");
> >
> > This bit doesn't seem to correspond to the patch description, was the
> > change deliberate or did it slip in by mistake?
> 
> This change was deliberate. As this hunk don't really change anything, I
> just don't explain it in the commit message. But I should have said the
> following in the description.
> 
> Also it changes argv[0] of the device model from "qemu-system-xen" to
> "qemu-dm".

In the non-stubdom case convention would be for argv[0] to contain
info->device_model (or is it "libxl__abs_path(&gc, info->device_model,
libxl_libexec_path()),"? eurk).

In the stubdom case I guess it might as well be whatever we like.

Ian.

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

[Anthony PERARD]

On Fri, 10 Dec 2010, Ian Campbell wrote:

> On Fri, 2010-12-10 at 12:29 +0000, Anthony Perard wrote:
> > On Fri, 10 Dec 2010, Ian Campbell wrote:
> >
> > > On Thu, 2010-12-09 at 19:42 +0000, anthony.perard@citrix.com wrote:
> > > >
> > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > index 0feb93f..0f6e632 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++, "qemu-dm");
> > >
> > > This bit doesn't seem to correspond to the patch description, was the
> > > change deliberate or did it slip in by mistake?
> >
> > This change was deliberate. As this hunk don't really change anything, I
> > just don't explain it in the commit message. But I should have said the
> > following in the description.
> >
> > Also it changes argv[0] of the device model from "qemu-system-xen" to
> > "qemu-dm".
>
> In the non-stubdom case convention would be for argv[0] to contain
> info->device_model (or is it "libxl__abs_path(&gc, info->device_model,
> libxl_libexec_path()),"? eurk).

Yes, this will be a little bit better, with info->device_model, like a
shell would do.

> In the stubdom case I guess it might as well be whatever we like.

-- 
Anthony PERARD

[PATCH v1.3 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.

Also it changes argv[0] of the device model from "qemu-system-xen" to
"qemu-dm".

---
 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..0f6e632 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 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 0f6e632..0ddeac0 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 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 0ddeac0..c8fe82c 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

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

[Ian Jackson]

anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> 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.

Is that understood by qemu-xen ?

Ian.

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

[Anthony PERARD]

On Fri, 10 Dec 2010, Ian Jackson wrote:

> anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> > 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.
>
> Is that understood by qemu-xen ?

Not really, it is understood by Qemu only when we have this:
#if defined(CONFIG_XEN) && !defined(CONFIG_DM).

And even in this case, it will do nothing because the variable that
receives the option is write only, it is never read.

So the answer is no, it is not understood by Qemu.

-- 
Anthony PERARD

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

[Anthony PERARD]

On Fri, 10 Dec 2010, Anthony PERARD wrote:

> On Fri, 10 Dec 2010, Ian Jackson wrote:
>
> > anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> > > 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.
> >
> > Is that understood by qemu-xen ?


> Not really, it is understood by Qemu only when we have this:
> #if defined(CONFIG_XEN) && !defined(CONFIG_DM).
>
> And even in this case, it will do nothing because the variable that
> receives the option is write only, it is never read.
>
> So the answer is no, it is not understood by Qemu.

You will read qemux-xen, here ...

-- 
Anthony PERARD

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

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> On Fri, 10 Dec 2010, Ian Jackson wrote:
> 
> > anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> > > 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.
> >
> > Is that understood by qemu-xen ?
> 
> Not really, it is understood by Qemu only when we have this:
> #if defined(CONFIG_XEN) && !defined(CONFIG_DM).
> 
> And even in this case, it will do nothing because the variable that
> receives the option is write only, it is never read.
> 
> So the answer is no, it is not understood by Qemu.

Your reply seems confusing to me.  Are we talking about the same
things ?

By "qemu-xen" I mean the old qemu tree which we are still using for
xen-unstable.  By "understood" I mean that the program will accept the
option.  You write "Qemu" but by "Qemu" I would normally understand
the current upstream qemu (presumably with your Xen patch series).

If the option is not understood by qemu-xen then current xen-unstable
will break if we apply your patch, surely ?

Ian.

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

[Anthony PERARD]

On Mon, 13 Dec 2010, Ian Jackson wrote:

> Anthony PERARD writes ("Re: [Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> > On Fri, 10 Dec 2010, Ian Jackson wrote:
> >
> > > anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> > > > 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.
> > >
> > > Is that understood by qemu-xen ?
> >
> > Not really, it is understood by Qemu only when we have this:
> > #if defined(CONFIG_XEN) && !defined(CONFIG_DM).
> >
> > And even in this case, it will do nothing because the variable that
> > receives the option is write only, it is never read.
> >
> > So the answer is no, it is not understood by Qemu.
>
> Your reply seems confusing to me.  Are we talking about the same
> things ?

Sorry, I make a typo in my mail. I was speaking about qemu-xen in my
reply, and not qemu upstream.

> By "qemu-xen" I mean the old qemu tree which we are still using for
> xen-unstable.  By "understood" I mean that the program will accept the
> option.  You write "Qemu" but by "Qemu" I would normally understand
> the current upstream qemu (presumably with your Xen patch series).
>
> If the option is not understood by qemu-xen then current xen-unstable
> will break if we apply your patch, surely ?

No, because the arguments for qemu-xen are not make by the same
function. There are two functions that do almost the same thing:
  - libxl_build_device_model_args_old
      this function make argument for qemu-xen, the one in xen-unstable.
  - libxl_build_device_model_args_new
      and this one make arguments for qemu (upstream).

-- 
Anthony PERARD

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

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [PATCH 4/5] libxl: Makes libxl be able to call Qemu upstream for XenPV guest."):
> No, because the arguments for qemu-xen are not make by the same
> function. There are two functions that do almost the same thing:
>   - libxl_build_device_model_args_old
>       this function make argument for qemu-xen, the one in xen-unstable.
>   - libxl_build_device_model_args_new
>       and this one make arguments for qemu (upstream).

Ah, thanks, I hadn't spotted that.

Ian.

[PATCH 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 c8fe82c..c79cca7 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 5/5] libxl: Lists qdisk device in libxl_device_disk_list

[Ian Jackson]

anthony.perard@citrix.com writes ("[Xen-devel] [PATCH 5/5] libxl: Lists qdisk device in libxl_device_disk_list"):
> 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.

Did you compile this ?

libxl.c:2559: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:2559: error: passing argument 1 of 'strlen' from incompatible pointer type
libxl.c:2559: error: passing argument 1 of '__strdup' from incompatible pointer type
make: *** [libxl.o] Error 1

Ian.