[PATCH 0/3] libxl cd-insert/eject with qemu-xen

[PATCH 0/3] libxl cd-insert/eject with qemu-xen

[Anthony PERARD]

This patch series provides the facility to eject and insert a cdrom when the
used device-model is qemu-xen. The only difference between both device-model is
a call to a QMP command as `xl cd-insert ...` will still update xenstore, even
if it's not used by QEMU.

Anthony PERARD (3):
  libxl_qmp, Introduce libxl__qmp_insert_cdrom.
  libxl_dm: Set an id to cdrom drives with qemuu.
  libxl: Fix cd-insert with qemu-xen.

 tools/libxl/libxl.c          |   12 ++++++------
 tools/libxl/libxl_dm.c       |    7 ++++---
 tools/libxl/libxl_internal.h |    1 +
 tools/libxl/libxl_qmp.c      |   35 +++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 9 deletions(-)

-- 
Anthony PERARD

[PATCH 1/3] libxl_qmp, Introduce libxl__qmp_insert_cdrom.

[Anthony PERARD]

This function can eject or change the CDROM for a guest that use qemu-xen as a
device-model.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |    1 +
 tools/libxl/libxl_qmp.c      |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c369fd3..eb8786c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1389,6 +1389,7 @@ _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
 _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename);
 /* Set dirty bitmap logging status */
 _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable);
+_hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk);
 /* close and free the QMP handler */
 _hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
 /* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 2c5559f..aebc733 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1024,6 +1024,41 @@ out:
     return rc;
 }
 
+int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
+                            const libxl_device_disk *disk)
+{
+    libxl__qmp_handler *qmp = NULL;
+    char *device_id;
+    int rc = 0;
+
+    device_id = libxl__sprintf(gc, "ide-%s", disk->vdev);
+    if (!device_id)
+        return ERROR_NOMEM;
+
+    qmp = libxl__qmp_initialize(gc, domid);
+    if (!qmp)
+        return ERROR_FAIL;
+
+    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
+        libxl__json_object *args = NULL;
+
+        args = qmp_parameters_add_string(gc, NULL, "device", device_id);
+        if (!args) {
+            rc = ERROR_NOMEM;
+            goto out;
+        }
+        rc = qmp_synchronous_send(qmp, "eject", args, NULL, NULL, qmp->timeout);
+        libxl__json_object_free(gc, args);
+    } else {
+        char *file = disk->pdev_path;
+        rc = qmp_change(gc, qmp, device_id, file, NULL);
+    }
+
+out:
+    libxl__qmp_close(qmp);
+    return rc;
+}
+
 int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
                                const libxl_domain_config *guest_config)
 {
-- 
Anthony PERARD

Re: [PATCH 1/3] libxl_qmp, Introduce libxl__qmp_insert_cdrom.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 1/3] libxl_qmp, Introduce libxl__qmp_insert_cdrom."):
> This function can eject or change the CDROM for a guest that use qemu-xen as a
> device-model.
...
> +int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
> +                            const libxl_device_disk *disk)
> +{
> +    libxl__qmp_handler *qmp = NULL;
> +    char *device_id;
> +    int rc = 0;
> +
> +    device_id = libxl__sprintf(gc, "ide-%s", disk->vdev);
> +    if (!device_id)
> +        return ERROR_NOMEM;

Use GCSPRINTF (and ditch the error check) ?

> +    qmp = libxl__qmp_initialize(gc, domid);
> +    if (!qmp)
> +        return ERROR_FAIL;

AIUI qemu only supports one concurrent user of its qmp.  You don't
seem to have anything here to deal with that.  Personally I think that
needs to be fixed in qemu.

> +    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> +        libxl__json_object *args = NULL;
> +
> +        args = qmp_parameters_add_string(gc, NULL, "device", device_id);

What is this function ?  I don't seem to be able to find it.  Perhaps
it's in another patch series of yours ?

Thanks,
Ian.

[PATCH 2/3] libxl_dm: Set an id to cdrom drives with qemuu.

[Anthony PERARD]

In order to eject and change a cdrom when using qemu-xen, this patch adds an id
the cdrom driver when starting the device model.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f2e9572..885fc46 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -549,11 +549,12 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             if (disks[i].is_cdrom) {
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
                     drive = libxl__sprintf
-                        (gc, "if=ide,index=%d,media=cdrom", disk);
+                        (gc, "if=ide,index=%d,media=cdrom,id=ide-%s",
+                         disk, disks[i].vdev);
                 else
                     drive = libxl__sprintf
-                        (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s",
-                         disks[i].pdev_path, disk, format);
+                        (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,id=ide-%s",
+                         disks[i].pdev_path, disk, format, disks[i].vdev);
             } else {
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
                     LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
-- 
Anthony PERARD

Re: [PATCH 2/3] libxl_dm: Set an id to cdrom drives with qemuu.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 2/3] libxl_dm: Set an id to cdrom drives with qemuu."):
> In order to eject and change a cdrom when using qemu-xen, this patch
> adds an id the cdrom driver when starting the device model.

> +                        (gc, "if=ide,index=%d,media=cdrom,id=ide-%s",
> +                         disk, disks[i].vdev);

You should not use the vdev string for this purpose because several
different vdev strings can refer to the same virtual device.

You should use the dev number instead.

Ian.

[PATCH 3/3] libxl: Fix cd-insert with qemu-xen.

[Anthony PERARD]

If qemu-xen is used as a device model, the command to insert, change or eject a
cdrom will go through QMP. XenStore is still updated even if QEMU will not read
from it.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 00ddc0e..ed89929 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2167,12 +2167,6 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         rc = ERROR_FAIL;
         goto out;
     }
-    if (dm_ver != LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
-        LOG(ERROR, "cdrom-insert does not work with %s",
-            libxl_device_model_version_to_string(dm_ver));
-        rc = ERROR_INVAL;
-        goto out;
-    }
 
     disks = libxl_device_disk_list(ctx, domid, &num);
     for (i = 0; i < num; i++) {
@@ -2196,6 +2190,12 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
 
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc) goto out;
+
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
+        if (rc) goto out;
+    }
+
     path = libxl__device_backend_path(gc, &device);
 
     insert = flexarray_make(4, 1);
-- 
Anthony PERARD

Re: [PATCH 3/3] libxl: Fix cd-insert with qemu-xen.

[Ian Jackson]

Anthony PERARD writes ("[PATCH 3/3] libxl: Fix cd-insert with qemu-xen."):
> If qemu-xen is used as a device model, the command to insert, change
> or eject a cdrom will go through QMP. XenStore is still updated even
> if QEMU will not read from it.

This looks plausible although I haven't tested it.

Ian.

Re: [PATCH 0/3] libxl cd-insert/eject with qemu-xen

[Anthony PERARD]

On Fri, Jul 27, 2012 at 6:17 PM, Anthony PERARD
<anthony.perard@citrix.com> wrote:
> This patch series provides the facility to eject and insert a cdrom when the
> used device-model is qemu-xen. The only difference between both device-model is
> a call to a QMP command as `xl cd-insert ...` will still update xenstore, even
> if it's not used by QEMU.

I forgot to tell, but this series is based on my previous series "Set
dirty log on qemu-xen." (for the libxl_qmp part)

> Anthony PERARD (3):
>   libxl_qmp, Introduce libxl__qmp_insert_cdrom.
>   libxl_dm: Set an id to cdrom drives with qemuu.
>   libxl: Fix cd-insert with qemu-xen.



-- 
Anthony PERARD

Re: [PATCH 0/3] libxl cd-insert/eject with qemu-xen

[Ian Jackson]

Anthony PERARD writes ("Re: [Xen-devel] [PATCH 0/3] libxl cd-insert/eject with qemu-xen"):
> On Fri, Jul 27, 2012 at 6:17 PM, Anthony PERARD
> <anthony.perard@citrix.com> wrote:
> > This patch series provides the facility to eject and insert a cdrom when the
> > used device-model is qemu-xen. The only difference between both device-model is
> > a call to a QMP command as `xl cd-insert ...` will still update xenstore, even
> > if it's not used by QEMU.
> 
> I forgot to tell, but this series is based on my previous series "Set
> dirty log on qemu-xen." (for the libxl_qmp part)

Aha, that's where that missing function is.

Ian.

Re: [PATCH 0/3] libxl cd-insert/eject with qemu-xen

[Ian Jackson]

Anthony PERARD writes ("[PATCH 0/3] libxl cd-insert/eject with qemu-xen"):
> This patch series provides the facility to eject and insert a cdrom
> when the used device-model is qemu-xen. The only difference between
> both device-model is a call to a QMP command as `xl cd-insert ...`
> will still update xenstore, even if it's not used by QEMU.

Thanks.  I'm afraid that this has missed the boat for 4.2 I think.

I'll review it anyway while I've got much of the related stuff
relatively fresh in my mind.

Ian.