diff mbox series

[8/9] libxl_disk: Use ev_qmp in libxl_cdrom_insert

Message ID 20190409164542.30274-9-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv | expand

Commit Message

Anthony PERARD April 9, 2019, 4:45 p.m. UTC 
libxl_cdrom_insert is now asynchronous when QEMU is involve. And the
cdrom is now openned by libxl before sending a file descriptor to QEMU.

The "opaque" parametre of the "add-fd" can help to figure out what a
fdset in QEMU is used for. It can be queried by "query-fdsets".

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_disk.c     | 131 ++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |   1 -
 tools/libxl/libxl_qmp.c      |  18 -----
 3 files changed, 105 insertions(+), 45 deletions(-)

Comments

Ian Jackson June 4, 2019, 5:45 p.m. UTC | #1 
Anthony PERARD writes ("[PATCH 8/9] libxl_disk: Use ev_qmp in libxl_cdrom_insert"):
> libxl_cdrom_insert is now asynchronous when QEMU is involve. And the
> cdrom is now openned by libxl before sending a file descriptor to QEMU.

This patch has much of the meat.  It seems to contain the appropriate
pieces and I generally like it.

I have only minor style quibbles.


I think conventional English grammar in commit messages is to use the
present tense for the situation which exists *before* the commit in
question.  I think you mean:

  Make libxl_cdrom_insert asynchronous when QEMU is involved.  And
  have the cdrom opened by libxl, sending a file descriptor to QEMU.

>          if (rc) goto out;
> +        asynchronous_callback = true;
> +    } else {
> +        asynchronous_callback = false;
...
> -    } else {
> -        cdrom_insert_ejected(egc, cis); /* must be last */
> +    } else if (!asynchronous_callback) {
> +        /* Only called if no asynchronous callback are set. */
> +        cdrom_insert_ejected(egc, &cis->qmp, NULL, 0); /* must be last */

This flag variable is pretty ugly.  Indeed the exit path from this
function is quite fiddly now.  But I can't think of a much prettier
way, and it looks like it is correct to me.

Another possibility would be to move all the variables like t and
d_config into the libxl__cdrom_insert_state, and then the cleanup
would be centralised.  But the lock lifetime of the userdata lock
might be wrong.

So, err, I guess, leave it like this unless you have any better ideas.

> -    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> -        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
> +    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
> +        disk->format != LIBXL_DISK_FORMAT_EMPTY) {
> +        libxl__json_object *args = NULL;
> +

That this doesn't ever leak payload_fd is not entirely clear.  How
about adding, here

  +        assert(qmp->payload_fd == -1);

?  Since the rule seems to be that the exit path will clean it up, but
that implies that overwriting it might leak a previous fd (of which
there isn't one right now...)

> +        qmp->payload_fd = open(disk->pdev_path, O_RDONLY);
> +        if (qmp->payload_fd < 0) {
> +            LOGED(ERROR, domid, "Failed to open cdrom file %s",
> +                  disk->pdev_path);
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        /* This free form parameter is not use by QEMU or libxl. */
> +        QMP_PARAMETERS_SPRINTF(&args, "opaque", "%s:%s",
> +                               libxl_disk_format_to_string(disk->format),
> +                               disk->pdev_path);
> +        qmp->callback = cdrom_insert_addfd_cb;
> +        rc = libxl__ev_qmp_send(gc, qmp, "add-fd", args);


Assuming you at least change the commit message, and regardless of
your opinion about the assert:

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

Thanks,
Ian.
diff mbox series

Patch

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 14dfc67971..785c8a27e7 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -668,12 +668,15 @@  typedef struct {
     libxl_device_disk disk_saved;
     libxl__domain_qmp_lock *qmp_lock;
     int dm_ver;
+    libxl__ev_qmp qmp;
 } libxl__cdrom_insert_state;
 
-static void cdrom_insert_ejected(libxl__egc *egc,
-                                 libxl__cdrom_insert_state *cis);
-static void cdrom_insert_inserted(libxl__egc *egc,
-                                  libxl__cdrom_insert_state *cis);
+static void cdrom_insert_ejected(libxl__egc *egc, libxl__ev_qmp *,
+                                 const libxl__json_object *, int rc);
+static void cdrom_insert_addfd_cb(libxl__egc *egc, libxl__ev_qmp *,
+                                  const libxl__json_object *, int rc);
+static void cdrom_insert_inserted(libxl__egc *egc, libxl__ev_qmp *,
+                                  const libxl__json_object *, int rc);
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc);
@@ -686,6 +689,7 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     libxl_device_disk *disks = NULL;
     int rc;
     libxl__cdrom_insert_state *cis;
+    bool asynchronous_callback = false;
 
     GCNEW(cis);
     cis->ao = ao;
@@ -693,6 +697,10 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     cis->disk = disk;
     libxl_device_disk_init(&cis->disk_saved);
     libxl_device_disk_copy(ctx, &cis->disk_saved, disk);
+    libxl__ev_qmp_init(&cis->qmp);
+    cis->qmp.ao = ao;
+    cis->qmp.domid = domid;
+    cis->qmp.payload_fd = -1;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -747,23 +755,21 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
-    /* We need to eject the original image first. This is implemented
-     * by inserting empty media. JSON is not updated.
+    /* We need to eject the original image first.
+     * JSON is not updated.
      */
 
     if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        libxl_device_disk disk_empty;
-
-        libxl_device_disk_init(&disk_empty);
-        disk_empty.format = LIBXL_DISK_FORMAT_EMPTY;
-        disk_empty.vdev = libxl__strdup(NOGC, disk->vdev);
-        disk_empty.pdev_path = libxl__strdup(NOGC, "");
-        disk_empty.is_cdrom = 1;
-        libxl__device_disk_setdefault(gc, domid, &disk_empty, false);
+        libxl__json_object *args = NULL;
+        int devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
 
-        rc = libxl__qmp_insert_cdrom(gc, domid, &disk_empty);
-        libxl_device_disk_dispose(&disk_empty);
+        QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", devid);
+        cis->qmp.callback = cdrom_insert_ejected;
+        rc = libxl__ev_qmp_send(gc, &cis->qmp, "eject", args);
         if (rc) goto out;
+        asynchronous_callback = true;
+    } else {
+        asynchronous_callback = false;
     }
 
     rc = 0;
@@ -772,17 +778,20 @@  int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     libxl__device_list_free(&libxl__disk_devtype, disks, num);
     if (rc) {
         cdrom_insert_done(egc, cis, rc); /* must be last */
-    } else {
-        cdrom_insert_ejected(egc, cis); /* must be last */
+    } else if (!asynchronous_callback) {
+        /* Only called if no asynchronous callback are set. */
+        cdrom_insert_ejected(egc, &cis->qmp, NULL, 0); /* must be last */
     }
     return AO_INPROGRESS;
 }
 
 static void cdrom_insert_ejected(libxl__egc *egc,
-                                 libxl__cdrom_insert_state *cis)
+                                 libxl__ev_qmp *qmp,
+                                 const libxl__json_object *response,
+                                 int rc)
 {
     EGC_GC;
-    int rc;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
     libxl__domain_userdata_lock *data_lock = NULL;
     libxl__device device;
     const char *be_path, *libxl_path;
@@ -790,6 +799,7 @@  static void cdrom_insert_ejected(libxl__egc *egc,
     xs_transaction_t t = XBT_NULL;
     char *tmp;
     libxl_domain_config d_config;
+    bool asynchronous_callback = false;
 
     /* convenience aliases */
     libxl_domid domid = cis->domid;
@@ -797,6 +807,8 @@  static void cdrom_insert_ejected(libxl__egc *egc,
 
     libxl_domain_config_init(&d_config);
 
+    if (rc) goto out;
+
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc) goto out;
     be_path = libxl__device_backend_path(gc, &device);
@@ -852,9 +864,28 @@  static void cdrom_insert_ejected(libxl__egc *egc,
     rc = libxl__dm_check_start(gc, &d_config, domid);
     if (rc) goto out;
 
-    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
+        disk->format != LIBXL_DISK_FORMAT_EMPTY) {
+        libxl__json_object *args = NULL;
+
+        qmp->payload_fd = open(disk->pdev_path, O_RDONLY);
+        if (qmp->payload_fd < 0) {
+            LOGED(ERROR, domid, "Failed to open cdrom file %s",
+                  disk->pdev_path);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* This free form parameter is not use by QEMU or libxl. */
+        QMP_PARAMETERS_SPRINTF(&args, "opaque", "%s:%s",
+                               libxl_disk_format_to_string(disk->format),
+                               disk->pdev_path);
+        qmp->callback = cdrom_insert_addfd_cb;
+        rc = libxl__ev_qmp_send(gc, qmp, "add-fd", args);
         if (rc) goto out;
+        asynchronous_callback = true;
+    } else {
+        asynchronous_callback = false;
     }
 
     rc = 0;
@@ -865,16 +896,58 @@  static void cdrom_insert_ejected(libxl__egc *egc,
     if (data_lock) libxl__unlock_domain_userdata(data_lock);
     if (rc) {
         cdrom_insert_done(egc, cis, rc); /* must be last */
-    } else {
-        cdrom_insert_inserted(egc, cis); /* must be last */
+    } else if (!asynchronous_callback) {
+        /* Only called if no asynchronous callback are set. */
+        cdrom_insert_inserted(egc, qmp, NULL, 0); /* must be last */
+    }
+}
+
+static void cdrom_insert_addfd_cb(libxl__egc *egc,
+                                  libxl__ev_qmp *qmp,
+                                  const libxl__json_object *response,
+                                  int rc)
+{
+    EGC_GC;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
+    libxl__json_object *args = NULL;
+    const libxl__json_object *o;
+    int devid;
+    int fdset;
+
+    /* convenience aliases */
+    libxl_device_disk *disk = cis->disk;
+
+    close(qmp->payload_fd);
+    qmp->payload_fd = -1;
+
+    if (rc) goto out;
+
+    o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
+    if (!o) {
+        rc = ERROR_FAIL;
+        goto out;
     }
+    fdset = libxl__json_object_get_integer(o);
+
+    devid = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+    QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", devid);
+    QMP_PARAMETERS_SPRINTF(&args, "target", "/dev/fdset/%d", fdset);
+    libxl__qmp_param_add_string(gc, &args, "arg",
+        libxl__qemu_disk_format_string(disk->format));
+    qmp->callback = cdrom_insert_inserted;
+    rc = libxl__ev_qmp_send(gc, qmp, "change", args);
+out:
+    if (rc)
+        cdrom_insert_done(egc, cis, rc); /* must be last */
 }
 
 static void cdrom_insert_inserted(libxl__egc *egc,
-                                  libxl__cdrom_insert_state *cis)
+                                  libxl__ev_qmp *qmp,
+                                  const libxl__json_object *response,
+                                  int rc)
 {
     EGC_GC;
-    int rc;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
     libxl__domain_userdata_lock *data_lock = NULL;
     libxl_domain_config d_config;
     flexarray_t *insert = NULL;
@@ -889,6 +962,8 @@  static void cdrom_insert_inserted(libxl__egc *egc,
 
     libxl_domain_config_init(&d_config);
 
+    if (rc) goto out;
+
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc) goto out;
     be_path = libxl__device_backend_path(gc, &device);
@@ -959,6 +1034,10 @@  static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc)
 {
+    EGC_GC;
+
+    libxl__ev_qmp_dispose(gc, &cis->qmp);
+    if (cis->qmp.payload_fd >= 0) close(cis->qmp.payload_fd);
     if (cis->qmp_lock) libxl__unlock_domain_qmp(cis->qmp_lock);
     libxl_device_disk_dispose(&cis->disk_saved);
     libxl__ao_complete(egc, cis->ao, rc);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9401f988e5..004935ea25 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1984,7 +1984,6 @@  _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
 _hidden int libxl__qmp_restore(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);
 /* Add a virtual CPU */
 _hidden int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int index);
 /* Query the bitmap of CPUs */
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index b6a691d9fc..25d3764f18 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1059,24 +1059,6 @@  int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
                            NULL, NULL);
 }
 
-int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
-                            const libxl_device_disk *disk)
-{
-    libxl__json_object *args = NULL;
-    int dev_number = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
-
-    QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", dev_number);
-
-    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
-        return qmp_run_command(gc, domid, "eject", args, NULL, NULL);
-    } else {
-        libxl__qmp_param_add_string(gc, &args, "target", disk->pdev_path);
-        libxl__qmp_param_add_string(gc, &args, "arg",
-            libxl__qemu_disk_format_string(disk->format));
-        return qmp_run_command(gc, domid, "change", args, NULL, NULL);
-    }
-}
-
 int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int idx)
 {
     libxl__json_object *args = NULL;