[Xen-devel] [PATCH v2 0/9] libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv

[Xen-devel] [PATCH v2 0/9] libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv

[Anthony PERARD]

Hi,

Changes in v2:
- New libxl__ev_lock, which actually respect lock hierarchy (it's outside of
  CTX_LOCK).
- some smaller changes detailed in patch notes.

This patch series fix libxl_cdrom_insert to work with a depriviledge QEMU. For
that, we need to use libxl__ev_qmp.  For that, we need a new lock because
userdata_lock can't be used anymore.

FYI: I don't think that enough yet to migrate a depriviledged QEMU. We may need
to open disks/cdrom in libxl before starting QEMU, similar to what this patch
series do when inserting a new cdrom.

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-slow-lock-v2

Anthony PERARD (9):
  libxl_internal: Remove lost comment
  libxl: Pointer on usage of libxl__domain_userdata_lock
  libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP
  libxl: Add optimisation to ev_lock
  libxl_disk: Reorganise libxl_cdrom_insert
  libxl_disk: Cut libxl_cdrom_insert into steps ..
  libxl_disk: Implement missing timeout for libxl_cdrom_insert
  libxl: Move qmp_parameters_* prototypes to libxl_internal.h
  libxl_disk: Use ev_qmp in libxl_cdrom_insert

 tools/libxl/Makefile         |   3 +
 tools/libxl/libxl_disk.c     | 341 ++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.c | 193 ++++++++++++++++++++
 tools/libxl/libxl_internal.h | 105 +++++++++--
 tools/libxl/libxl_qmp.c      |  89 ++++-----
 5 files changed, 601 insertions(+), 130 deletions(-)

-- 
Anthony PERARD


Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-slow-lock-v2

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 1/9] libxl_internal: Remove lost comment

[Anthony PERARD]

That comment as been separated from the function it defines by
4197d3abbb3055d3798254eb7ba239bfb5824360, but then was not useful
anymore when the libxl__device_disk_add() prototype was removed by
22ea8ad02e465e32cd40887c750b55c3a997a288.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3be5c644c1..5f90c210af 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2718,13 +2718,6 @@ struct libxl__multidev {
  *                   DONE.
  */
 
-/* AO operation to connect a disk device, called by
- * libxl_device_disk_add and libxl__add_disks. This function calls
- * libxl__wait_device_connection to wait for the device to
- * finish the connection (might involve executing hotplug scripts).
- *
- * Once finished, aodev->callback will be executed.
- */
 /*
  * As of Xen 4.5 we maintain various information, including hotplug
  * device information, in JSON files, so that we can use this JSON
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 2/9] libxl: Pointer on usage of libxl__domain_userdata_lock

[Anthony PERARD]

It is currently difficult to know how/when/why the userdata lock is
supposed to be used. Add some pointers to the hotplug comments.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5f90c210af..ca7206aaac 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4474,6 +4474,12 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock);
  * data store. The registry entry in libxl private data store
  * is "libxl-json".
  * Caller must hold user data lock.
+ *
+ * Other names used for this lock throughout the libxl code are json_lock,
+ * libxl__domain_userdata_lock, "libxl-json", data store lock.
+ *
+ * See the comment for libxl__ao_device, and "Algorithm for handling device
+ * removal", for information about using the libxl-json lock / json_lock.
  */
 int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid,
                                     libxl_domain_config *d_config);
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP

[Anthony PERARD]

The current lock `domain_userdata_lock' can't be used when modification
to a guest is done by sending command to QEMU, this is a slow process
and requires to call CTX_UNLOCK, which is not possible while holding
the `domain_userdata_lock'.

To resolve this issue, we create a new lock which can take over part
of the job of the json_lock.

This lock is outside CTX_LOCK in the lock hierarchy.
libxl__ev_lock_get will have CTX_UNLOCK before trying to grab the
ev_lock. The callback is used to notify when the ev_lock have been
acquired.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    Should libxl__ev_unlock() kill the child?
    
    That would mean that we would need to handle the case where the process
    trying to grab the lock got impatient and decided that it was taking too
    long.
    
    v2:
    - new patch, to replace 2 patch
      implement a different lock

 tools/libxl/libxl_internal.c | 174 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  76 ++++++++++++++-
 2 files changed, 246 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f492dae5ff..3906a0512d 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -575,6 +575,180 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
+void libxl__ev_lock_init(libxl__ev_lock *lock)
+{
+    libxl__ev_child_init(&lock->child);
+    lock->path = NULL;
+    lock->fd = -1;
+    lock->held = false;
+}
+
+static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock);
+static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
+                                   pid_t pid, int status);
+
+void libxl__ev_lock_get(libxl__egc *egc, libxl__ev_lock *lock)
+{
+    STATE_AO_GC(lock->ao);
+    const char *lockfile;
+
+    lockfile = libxl__userdata_path(gc, lock->domid,
+                                    "libxl-device-changes-lock", "l");
+    if (!lockfile) goto out;
+    lock->path = libxl__strdup(NOGC, lockfile);
+
+    ev_lock_prepare_fork(egc, lock);
+    return;
+out:
+    lock->callback(egc, lock, ERROR_LOCK_FAIL);
+}
+
+static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock)
+{
+    STATE_AO_GC(lock->ao);
+    pid_t pid;
+    int fd;
+
+    /* Convenience aliases */
+    libxl_domid domid = lock->domid;
+    const char *lockfile = lock->path;
+
+    lock->fd = open(lockfile, O_RDWR|O_CREAT, 0666);
+    if (lock->fd < 0) {
+        LOGED(ERROR, domid, "cannot open lockfile %s", lockfile);
+        goto out;
+    }
+    fd = lock->fd;
+
+    pid = libxl__ev_child_fork(gc, &lock->child, ev_lock_child_callback);
+    if (pid < 0)
+        goto out;
+    if (!pid) {
+        /* child */
+        int exit_val = 0;
+
+        /* Lock the file in exclusive mode, wait indefinitely to
+         * acquire the lock */
+        while (flock(fd, LOCK_EX)) {
+            switch (errno) {
+            case EINTR:
+                /* Signal received, retry */
+                continue;
+            default:
+                /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
+                LOGED(ERROR, domid,
+                      "unexpected error while trying to lock %s, fd=%d, errno=%d",
+                      lockfile, fd, errno);
+                exit_val = 1;
+                break;
+            }
+        }
+        _exit(exit_val);
+    }
+
+    /* Now that the child has the fd, set cloexec in the parent to prevent
+     * more leakage than necessary */
+    libxl_fd_set_cloexec(CTX, fd, 1);
+    return;
+out:
+    libxl__ev_unlock(gc, lock);
+    lock->callback(egc, lock, ERROR_LOCK_FAIL);
+}
+
+static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
+                                   pid_t pid, int status)
+{
+    EGC_GC;
+    libxl__ev_lock *lock = CONTAINER_OF(child, *lock, child);
+    struct stat stab, fstab;
+    int rc = ERROR_LOCK_FAIL;
+
+    /* Convenience aliases */
+    int fd = lock->fd;
+    const char *lockfile = lock->path;
+    libxl_domid domid = lock->domid;
+
+    if (status) {
+        libxl_report_child_exitstatus(CTX, XTL_ERROR, "flock child",
+                                      pid, status);
+        goto out;
+    }
+
+    if (fstat(fd, &fstab)) {
+        LOGED(ERROR, domid, "cannot fstat %s, fd=%d", lockfile, fd);
+        goto out;
+    }
+    if (stat(lockfile, &stab)) {
+        if (errno != ENOENT) {
+            LOGED(ERROR, domid, "cannot stat %s", lockfile);
+            goto out;
+        }
+    } else {
+        if (stab.st_dev == fstab.st_dev && stab.st_ino == fstab.st_ino) {
+            /* We held the lock */
+            lock->held = true;
+            rc = 0;
+            goto out;
+        }
+    }
+
+    /* We didn't grab the lock, let's try again */
+    flock(lock->fd, LOCK_UN);
+    close(lock->fd);
+    lock->fd = -1;
+    ev_lock_prepare_fork(egc, lock);
+    return;
+
+out:
+    if (lock->held) {
+        /* Check the domain is still there, if not we should release the
+         * lock and clean up.  */
+        if (libxl_domain_info(CTX, NULL, domid))
+            rc = ERROR_LOCK_FAIL;
+    }
+    if (rc) {
+        LOGD(ERROR, domid, "Failed to grab qmp-lock");
+        libxl__ev_unlock(gc, lock);
+    }
+    lock->callback(egc, lock, rc);
+}
+
+void libxl__ev_unlock(libxl__gc *gc, libxl__ev_lock *lock)
+{
+    int r;
+
+    assert(!libxl__ev_child_inuse(&lock->child));
+
+    /* It's important to unlink the file before releasing the lock to avoid
+     * the following race (if unlock/close before unlink):
+     *
+     *   P1 LOCK                         P2 UNLOCK
+     *   fd1 = open(lockfile)
+     *                                   unlock(fd2)
+     *   flock(fd1)
+     *   fstat and stat check success
+     *                                   unlink(lockfile)
+     *   return lock
+     *
+     * In above case P1 thinks it has got hold of the lock but
+     * actually lock is released by P2 (lockfile unlinked).
+     */
+    if (lock->path && lock->held)
+        unlink(lock->path);
+
+    if (lock->fd >= 0) {
+        /* We need to call unlock as the fd may have leaked into other
+         * processes */
+        r = flock(lock->fd, LOCK_UN);
+        if (r)
+            LOGED(ERROR, lock->domid, "failed to unlock fd=%d, path=%s",
+                  lock->fd, lock->path);
+        close(lock->fd);
+    }
+    free(lock->path);
+    libxl__ev_lock_init(lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ca7206aaac..2c2476b55b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -194,6 +194,7 @@ typedef struct libxl__osevent_hook_nexus libxl__osevent_hook_nexus;
 typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
 typedef struct libxl__json_object libxl__json_object;
 typedef struct libxl__carefd libxl__carefd;
+typedef struct libxl__ev_lock libxl__ev_lock;
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
 typedef void libxl__domain_create_cb(struct libxl__egc *egc,
@@ -2723,11 +2724,11 @@ struct libxl__multidev {
  * device information, in JSON files, so that we can use this JSON
  * file as a template to reconstruct domain configuration.
  *
- * In essense there are now two views of device state, one is xenstore,
- * the other is JSON file. We use xenstore as primary reference.
+ * In essense there are now two views of device state, one is the
+ * primary config (xenstore or QEMU), the other is JSON file.
  *
- * Here we maintain one invariant: every device in xenstore must have
- * an entry in JSON file.
+ * Here we maintain one invariant: every device in the primary config
+ * must have an entry in JSON file.
  *
  * All device hotplug routines should comply to following pattern:
  *   lock json config (json_lock)
@@ -2742,6 +2743,24 @@ struct libxl__multidev {
  *       end for loop
  *   unlock json config
  *
+ * Or in case QEMU is the primary config, this pattern can be use:
+ *   qmp_lock (libxl__ev_lock)
+ *      lock json config (json_lock)
+ *          read json config
+ *          update in-memory json config with new entry, replacing
+ *             any stale entry
+ *      unlock json config
+ *      apply new config to primary config
+ *      lock json config (json_lock)
+ *          read json config
+ *          update in-memory json config with new entry, replacing
+ *             any stale entry
+ *          write in-memory json config to disk
+ *      unlock json config
+ *   unlock qmp_lock
+ *   (CTX_LOCK can be acquired and released several time while holding the
+ *    qmp_lock)
+ *
  * Device removal routines are not touched.
  *
  * Here is the proof that we always maintain that invariant and we
@@ -4600,6 +4619,55 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
 {
     return GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
 }
+
+/*
+ * Lock for device hotplug, qmp_lock.
+ *
+ * libxl__ev_lock implement a lock that is outside of CTX_LOCK in the
+ * lock hierarchy. It can be used when one want to make QMP calls to QEMU,
+ * which may take a significant amount time.
+ * It is to be acquired by an ao event callback.
+ *
+ * It is to be acquired when adding/removing devices or making changes
+ * to them when this is a slow operation and json_lock isn't appropriate.
+ *
+ * Possible states of libxl__ev_lock:
+ *   Undefined
+ *    Might contain anything.
+ *  Idle
+ *    Struct contents are defined enough to pass to any
+ *    libxl__ev_lock_* function.
+ *    The struct does not contain references to any allocated private
+ *    resources so can be thrown away.
+ *  Active
+ *    Waiting to get a lock.
+ *    Needs to wait until the callback is called.
+ *  LockAcquired
+ *    libxl__ev_unlock will need to be called to release the lock and the
+ *    resources of libxl__ev_lock.
+ *
+ *  libxl__ev_lock_init: Undefined/Idle -> Idle
+ *  libxl__ev_lock_get:  Idle -> Active
+ *      May call callback synchronously.
+ *  libxl__ev_unlock:    LockAcquired/Idle -> Idle
+ *  callback:     When called: Active -> LockAcquired (on error: Idle)
+ *    The callback is only called once.
+ */
+struct libxl__ev_lock {
+    /* filled by user */
+    libxl__ao *ao;
+    libxl_domid domid;
+    void (*callback)(libxl__egc *, libxl__ev_lock *, int rc);
+    /* private to libxl__ev_lock* */
+    libxl__ev_child child;
+    char *path; /* path of the lock file itself */
+    int fd;
+    bool held;
+};
+_hidden void libxl__ev_lock_init(libxl__ev_lock *);
+_hidden void libxl__ev_lock_get(libxl__egc *, libxl__ev_lock *);
+_hidden void libxl__ev_unlock(libxl__gc *, libxl__ev_lock *);
+
 #endif
 
 /*
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 4/9] libxl: Add optimisation to ev_lock

[Anthony PERARD]

It will often be the case that the lock is free to grab. So we first
try to grab it before we have to fork. Even though in this case the
locks are grabbed in the wrong order in the lock hierarchy (ev_lock
should be outside of CTX_LOCK), it is fine to try without blocking. If
that failed, we will release CTX_LOCK and try to grab both lock again
in the right order.

That optimisation is only enabled in releases (debug=n) so the more
complicated code with fork is actually exercised.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - new patch

 tools/libxl/Makefile         |  3 +++
 tools/libxl/libxl_internal.c | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 6fdcbbddd6..4587a6fc9c 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -35,6 +35,9 @@ ifeq ($(CONFIG_LIBNL),y)
 CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
 endif
 CFLAGS_LIBXL += -Wshadow
+ifeq ($(debug),y)
+CFLAGS_LIBXL += -DCONFIG_DEBUG
+endif
 
 LIBXL_LIBS-$(CONFIG_ARM) += -lfdt
 
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 3906a0512d..fb1b9bfe52 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -620,6 +620,25 @@ static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock)
     }
     fd = lock->fd;
 
+    /* Enable this optimisation only in releases, so the fork code is
+     * exercised while libxl is built with debug=y. */
+#ifndef CONFIG_DEBUG
+    /*
+     * We try to grab the lock before forking as it is likely to be free.
+     * Even though we are supposed to CTX_UNLOCK before attempting to grab
+     * the ev_lock, it is fine to do a non-blocking request now with the
+     * CTX_LOCK held as if that fails we'll try again in a fork (CTX_UNLOCK
+     * will be called in libxl), that will avoid deadlocks.
+     */
+    int r = flock(fd, LOCK_EX | LOCK_NB);
+    if (!r) {
+        libxl_fd_set_cloexec(CTX, fd, 1);
+        /* We held a lock, no need to fork but we need to check it. */
+        ev_lock_child_callback(egc, &lock->child, 0, 0);
+        return;
+    }
+#endif
+
     pid = libxl__ev_child_fork(gc, &lock->child, ev_lock_child_callback);
     if (pid < 0)
         goto out;
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 5/9] libxl_disk: Reorganise libxl_cdrom_insert

[Anthony PERARD]

This is in preparation of cutting libxl_cdrom_insert into several
functions to allow asynchronous callbacks.

No functional changes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_disk.c | 58 ++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index bc9e2d5a74..45bf555061 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -647,7 +647,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
 {
     AO_CREATE(ctx, domid, ao_how);
     int num = 0, i;
-    libxl_device_disk *disks = NULL, disk_saved, disk_empty;
+    libxl_device_disk *disks = NULL, disk_saved;
     libxl_domain_config d_config;
     int rc, dm_ver;
     libxl__device device;
@@ -658,16 +658,9 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     flexarray_t *insert = NULL, *empty = NULL;
 
     libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_empty);
     libxl_device_disk_init(&disk_saved);
     libxl_device_disk_copy(ctx, &disk_saved, disk);
 
-    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_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
         rc = ERROR_FAIL;
@@ -721,23 +714,6 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     be_path = libxl__device_backend_path(gc, &device);
     libxl_path = libxl__device_libxl_path(gc, &device);
 
-    insert = flexarray_make(gc, 4, 1);
-
-    flexarray_append_pair(insert, "type",
-                          libxl__device_disk_string_of_backend(disk->backend));
-    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
-        flexarray_append_pair(insert, "params",
-                        GCSPRINTF("%s:%s",
-                            libxl__device_disk_string_of_format(disk->format),
-                            disk->pdev_path));
-    else
-        flexarray_append_pair(insert, "params", "");
-
-    empty = flexarray_make(gc, 4, 1);
-    flexarray_append_pair(empty, "type",
-                          libxl__device_disk_string_of_backend(disk->backend));
-    flexarray_append_pair(empty, "params", "");
-
     /* Note: CTX lock is already held at this point so lock hierarchy
      * is maintained.
      */
@@ -750,11 +726,27 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     /* We need to eject the original image first. This is implemented
      * by inserting empty media. JSON is not updated.
      */
+
     if (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);
+
         rc = libxl__qmp_insert_cdrom(gc, domid, &disk_empty);
+        libxl_device_disk_dispose(&disk_empty);
         if (rc) goto out;
     }
 
+    empty = flexarray_make(gc, 4, 1);
+    flexarray_append_pair(empty, "type",
+                          libxl__device_disk_string_of_backend(disk->backend));
+    flexarray_append_pair(empty, "params", "");
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -781,6 +773,10 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc < 0) goto out;
     }
 
+    /*
+     * Now that the drive is empty, we can insert the new media.
+     */
+
     rc = libxl__get_domain_configuration(gc, domid, &d_config);
     if (rc) goto out;
 
@@ -794,6 +790,17 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc) goto out;
     }
 
+    insert = flexarray_make(gc, 4, 1);
+    flexarray_append_pair(insert, "type",
+                      libxl__device_disk_string_of_backend(disk->backend));
+    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
+        flexarray_append_pair(insert, "params",
+                    GCSPRINTF("%s:%s",
+                        libxl__device_disk_string_of_format(disk->format),
+                        disk->pdev_path));
+    else
+        flexarray_append_pair(insert, "params", "");
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -831,7 +838,6 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
 out:
     libxl__xs_transaction_abort(gc, &t);
     libxl__device_list_free(&libxl__disk_devtype, disks, num);
-    libxl_device_disk_dispose(&disk_empty);
     libxl_device_disk_dispose(&disk_saved);
     libxl_domain_config_dispose(&d_config);
 
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

[Anthony PERARD]

.. and use a new "slow" lock to avoid holding the userdata lock across
several functions.

This patch cuts libxl_cdrom_insert into different step/function but
there are still called synchronously. (Taking the ev_lock is the only
step that might be asynchronous.) A later patch will call them
asynchronously when QMP is involved.

Thee userdata lock (json_lock) use to protect against concurrent change
of cdrom is replaced by an ev_lock which can be held across different
CTX_LOCK sections. The json_lock is still used when reading/modifying
the domain userdata (mandatory) and update xenstore (mostly because
it's updated as the same time as the userdata).

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---

Notes:
    v2:
    - rewrite patch description
    - rework use of the new lock

 tools/libxl/libxl_disk.c | 196 +++++++++++++++++++++++++++++++--------
 1 file changed, 155 insertions(+), 41 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 45bf555061..5f13a622f9 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -642,24 +642,43 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
     return rc;
 }
 
+typedef struct {
+    libxl__ao *ao;
+    libxl_domid domid;
+    libxl_device_disk *disk;
+    libxl_device_disk disk_saved;
+    libxl__ev_lock qmp_lock;
+    int dm_ver;
+} libxl__cdrom_insert_state;
+
+static void cdrom_insert_lock_acquired(libxl__egc *, libxl__ev_lock *,
+                                       int rc);
+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_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc);
+
 int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
                        const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
     int num = 0, i;
-    libxl_device_disk *disks = NULL, disk_saved;
-    libxl_domain_config d_config;
-    int rc, dm_ver;
-    libxl__device device;
-    const char *be_path, *libxl_path;
-    char * tmp;
-    libxl__domain_userdata_lock *lock = NULL;
-    xs_transaction_t t = XBT_NULL;
-    flexarray_t *insert = NULL, *empty = NULL;
-
-    libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_saved);
-    libxl_device_disk_copy(ctx, &disk_saved, disk);
+    libxl_device_disk *disks = NULL;
+    int rc;
+    libxl__cdrom_insert_state *cis;
+
+    GCNEW(cis);
+    cis->ao = ao;
+    cis->domid = domid;
+    cis->disk = disk;
+    libxl_device_disk_init(&cis->disk_saved);
+    libxl_device_disk_copy(ctx, &cis->disk_saved, disk);
+    libxl__ev_lock_init(&cis->qmp_lock);
+    cis->qmp_lock.ao = ao;
+    cis->qmp_lock.domid = domid;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -678,8 +697,8 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
-    dm_ver = libxl__device_model_version_running(gc, domid);
-    if (dm_ver == -1) {
+    cis->dm_ver = libxl__device_model_version_running(gc, domid);
+    if (cis->dm_ver == -1) {
         LOGD(ERROR, domid, "Cannot determine device model version");
         rc = ERROR_FAIL;
         goto out;
@@ -708,40 +727,82 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         disk->format = LIBXL_DISK_FORMAT_EMPTY;
     }
 
-    rc = libxl__device_from_disk(gc, domid, disk, &device);
-    if (rc) goto out;
+out:
+    libxl__device_list_free(&libxl__disk_devtype, disks, num);
+    if (rc) {
+        cdrom_insert_done(egc, cis, rc); /* must be last */
+    } else {
+        cis->qmp_lock.callback = cdrom_insert_lock_acquired;
+        libxl__ev_lock_get(egc, &cis->qmp_lock); /* must be last */
+    }
+    return AO_INPROGRESS;
+}
 
-    be_path = libxl__device_backend_path(gc, &device);
-    libxl_path = libxl__device_libxl_path(gc, &device);
+static void cdrom_insert_lock_acquired(libxl__egc *egc,
+                                       libxl__ev_lock *lock,
+                                       int rc)
+{
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(lock, *cis, qmp_lock);
+    STATE_AO_GC(cis->ao);
 
-    /* Note: CTX lock is already held at this point so lock hierarchy
-     * is maintained.
-     */
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
-        rc = ERROR_LOCK_FAIL;
-        goto out;
-    }
+    if (rc) goto out;
 
     /* We need to eject the original image first. This is implemented
      * by inserting empty media. JSON is not updated.
      */
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+    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.vdev = libxl__strdup(NOGC, cis->disk->vdev);
         disk_empty.pdev_path = libxl__strdup(NOGC, "");
         disk_empty.is_cdrom = 1;
-        libxl__device_disk_setdefault(gc, domid, &disk_empty, false);
+        libxl__device_disk_setdefault(gc, cis->domid, &disk_empty, false);
 
-        rc = libxl__qmp_insert_cdrom(gc, domid, &disk_empty);
+        rc = libxl__qmp_insert_cdrom(gc, cis->domid, &disk_empty);
         libxl_device_disk_dispose(&disk_empty);
         if (rc) goto out;
     }
 
+    cdrom_insert_ejected(egc, cis); /* must be last */
+    return;
+
+out:
+    cdrom_insert_done(egc, cis, rc); /* must be last */
+}
+
+static void cdrom_insert_ejected(libxl__egc *egc,
+                                 libxl__cdrom_insert_state *cis)
+{
+    EGC_GC;
+    int rc;
+    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl__device device;
+    const char *be_path, *libxl_path;
+    flexarray_t *empty = NULL;
+    xs_transaction_t t = XBT_NULL;
+    char *tmp;
+    libxl_domain_config d_config;
+
+    /* convenience aliases */
+    libxl_domid domid = cis->domid;
+    libxl_device_disk *disk = cis->disk;
+
+    libxl_domain_config_init(&d_config);
+
+    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    if (rc) goto out;
+    be_path = libxl__device_backend_path(gc, &device);
+    libxl_path = libxl__device_libxl_path(gc, &device);
+
+    data_lock = libxl__lock_domain_userdata(gc, domid);
+    if (!data_lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
     empty = flexarray_make(gc, 4, 1);
     flexarray_append_pair(empty, "type",
                           libxl__device_disk_string_of_backend(disk->backend));
@@ -780,16 +841,66 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     rc = libxl__get_domain_configuration(gc, domid, &d_config);
     if (rc) goto out;
 
-    device_add_domain_config(gc, &d_config, &libxl__disk_devtype, &disk_saved);
+    device_add_domain_config(gc, &d_config, &libxl__disk_devtype,
+                             &cis->disk_saved);
 
     rc = libxl__dm_check_start(gc, &d_config, domid);
     if (rc) goto out;
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         rc = libxl__qmp_insert_cdrom(gc, domid, disk);
         if (rc) goto out;
     }
 
+    rc = 0;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+    libxl_domain_config_dispose(&d_config);
+    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 */
+    }
+}
+
+static void cdrom_insert_inserted(libxl__egc *egc,
+                                  libxl__cdrom_insert_state *cis)
+{
+    EGC_GC;
+    int rc;
+    libxl__domain_userdata_lock *data_lock = NULL;
+    libxl_domain_config d_config;
+    flexarray_t *insert = NULL;
+    xs_transaction_t t = XBT_NULL;
+    libxl__device device;
+    const char *be_path, *libxl_path;
+    char *tmp;
+
+    /* convenience aliases */
+    libxl_domid domid = cis->domid;
+    libxl_device_disk *disk = cis->disk;
+
+    libxl_domain_config_init(&d_config);
+
+    rc = libxl__device_from_disk(gc, domid, disk, &device);
+    if (rc) goto out;
+    be_path = libxl__device_backend_path(gc, &device);
+    libxl_path = libxl__device_libxl_path(gc, &device);
+
+    data_lock = libxl__lock_domain_userdata(gc, domid);
+    if (!data_lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
+    if (rc) goto out;
+
+    device_add_domain_config(gc, &d_config, &libxl__disk_devtype,
+                             &cis->disk_saved);
+
     insert = flexarray_make(gc, 4, 1);
     flexarray_append_pair(insert, "type",
                       libxl__device_disk_string_of_backend(disk->backend));
@@ -830,21 +941,24 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc < 0) goto out;
     }
 
-    /* success, no actual async */
-    libxl__ao_complete(egc, ao, 0);
-
     rc = 0;
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    libxl__device_list_free(&libxl__disk_devtype, disks, num);
-    libxl_device_disk_dispose(&disk_saved);
     libxl_domain_config_dispose(&d_config);
+    if (data_lock) libxl__unlock_domain_userdata(data_lock);
+    cdrom_insert_done(egc, cis, rc); /* must be last */
+}
 
-    if (lock) libxl__unlock_domain_userdata(lock);
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc)
+{
+    EGC_GC;
 
-    if (rc) return AO_CREATE_FAIL(rc);
-    return AO_INPROGRESS;
+    libxl__ev_unlock(gc, &cis->qmp_lock);
+    libxl_device_disk_dispose(&cis->disk_saved);
+    libxl__ao_complete(egc, cis->ao, rc);
 }
 
 /* libxl__alloc_vdev only works on the local domain, that is the domain
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 7/9] libxl_disk: Implement missing timeout for libxl_cdrom_insert

[Anthony PERARD]

After the patch "libxl_disk: Use ev_qmp in libxl_cdrom_insert"
there will not be any kind of timeout, add one back.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    Ian, in this patch, the timeout is setup after we have aquired the lock.
    Should we change that to also have a timeout waiting for the lock to be
    released?
    
    v2:
    - patch move earlier in the series to keep bisectability.

 tools/libxl/libxl_disk.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 5f13a622f9..7328a03e8a 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -649,6 +649,7 @@ typedef struct {
     libxl_device_disk disk_saved;
     libxl__ev_lock qmp_lock;
     int dm_ver;
+    libxl__ev_time time;
 } libxl__cdrom_insert_state;
 
 static void cdrom_insert_lock_acquired(libxl__egc *, libxl__ev_lock *,
@@ -657,6 +658,9 @@ 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_timout(libxl__egc *egc, libxl__ev_time *ev,
+                                const struct timeval *requested_abs,
+                                int rc);
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc);
@@ -679,6 +683,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     libxl__ev_lock_init(&cis->qmp_lock);
     cis->qmp_lock.ao = ao;
     cis->qmp_lock.domid = domid;
+    libxl__ev_time_init(&cis->time);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -747,6 +752,11 @@ static void cdrom_insert_lock_acquired(libxl__egc *egc,
 
     if (rc) goto out;
 
+    rc = libxl__ev_time_register_rel(ao, &cis->time,
+                                     cdrom_insert_timout,
+                                     LIBXL_HOTPLUG_TIMEOUT * 1000);
+    if (rc) goto out;
+
     /* We need to eject the original image first. This is implemented
      * by inserting empty media. JSON is not updated.
      */
@@ -950,12 +960,23 @@ static void cdrom_insert_inserted(libxl__egc *egc,
     cdrom_insert_done(egc, cis, rc); /* must be last */
 }
 
+static void cdrom_insert_timout(libxl__egc *egc, libxl__ev_time *ev,
+                                const struct timeval *requested_abs,
+                                int rc)
+{
+    EGC_GC;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(ev, *cis, time);
+    LOGD(ERROR, cis->domid, "cdrom insertion timed out");
+    cdrom_insert_done(egc, cis, rc);
+}
+
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc)
 {
     EGC_GC;
 
+    libxl__ev_time_deregister(gc, &cis->time);
     libxl__ev_unlock(gc, &cis->qmp_lock);
     libxl_device_disk_dispose(&cis->disk_saved);
     libxl__ao_complete(egc, cis->ao, rc);
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 8/9] libxl: Move qmp_parameters_* prototypes to libxl_internal.h

[Anthony PERARD]

.. and rename them to libxl__qmp_param_*.

This is to allow other files than libxl_qmp.c to make QMP calls with
parameters.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_internal.h | 15 ++++++++
 tools/libxl/libxl_qmp.c      | 75 +++++++++++++++++-------------------
 2 files changed, 51 insertions(+), 39 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2c2476b55b..2dc5a31755 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -470,6 +470,21 @@ struct libxl__ev_qmp {
     int msg_id;
 };
 
+/* QMP parameters helpers */
+
+_hidden void libxl__qmp_param_add_string(libxl__gc *gc,
+                                         libxl__json_object **param,
+                                         const char *name, const char *s);
+_hidden void libxl__qmp_param_add_bool(libxl__gc *gc,
+                                       libxl__json_object **param,
+                                       const char *name, bool b);
+_hidden void libxl__qmp_param_add_integer(libxl__gc *gc,
+                                          libxl__json_object **param,
+                                          const char *name, const int i);
+#define QMP_PARAMETERS_SPRINTF(args, name, format, ...) \
+    libxl__qmp_param_add_string(gc, args, name, \
+                                GCSPRINTF(format, __VA_ARGS__))
+
 
 /*
  * evgen structures, which are the state we use for generating
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 42c8ab8d8d..b6a691d9fc 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -752,9 +752,9 @@ static void qmp_parameters_common_add(libxl__gc *gc,
     flexarray_append((*param)->u.map, arg);
 }
 
-static void qmp_parameters_add_string(libxl__gc *gc,
-                                      libxl__json_object **param,
-                                      const char *name, const char *argument)
+void libxl__qmp_param_add_string(libxl__gc *gc,
+                                 libxl__json_object **param,
+                                 const char *name, const char *argument)
 {
     libxl__json_object *obj;
 
@@ -764,9 +764,9 @@ static void qmp_parameters_add_string(libxl__gc *gc,
     qmp_parameters_common_add(gc, param, name, obj);
 }
 
-static void qmp_parameters_add_bool(libxl__gc *gc,
-                                    libxl__json_object **param,
-                                    const char *name, bool b)
+void libxl__qmp_param_add_bool(libxl__gc *gc,
+                               libxl__json_object **param,
+                               const char *name, bool b)
 {
     libxl__json_object *obj;
 
@@ -775,9 +775,9 @@ static void qmp_parameters_add_bool(libxl__gc *gc,
     qmp_parameters_common_add(gc, param, name, obj);
 }
 
-static void qmp_parameters_add_integer(libxl__gc *gc,
-                                       libxl__json_object **param,
-                                       const char *name, const int i)
+void libxl__qmp_param_add_integer(libxl__gc *gc,
+                                  libxl__json_object **param,
+                                  const char *name, const int i)
 {
     libxl__json_object *obj;
 
@@ -787,9 +787,6 @@ static void qmp_parameters_add_integer(libxl__gc *gc,
     qmp_parameters_common_add(gc, param, name, obj);
 }
 
-#define QMP_PARAMETERS_SPRINTF(args, name, format, ...) \
-    qmp_parameters_add_string(gc, args, name, GCSPRINTF(format, __VA_ARGS__))
-
 /*
  * API
  */
@@ -943,7 +940,7 @@ int libxl__qmp_run_command_flexarray(libxl__gc *gc, int domid,
     for (i = 0; i < array->count; i += 2) {
         flexarray_get(array, i, &name);
         flexarray_get(array, i + 1, &value);
-        qmp_parameters_add_string(gc, &args, (char *)name, (char *)value);
+        libxl__qmp_param_add_string(gc, &args, (char *)name, (char *)value);
     }
 
     return qmp_run_command(gc, domid, cmd, args, NULL, NULL);
@@ -965,10 +962,10 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
     if (!hostaddr)
         return -1;
 
-    qmp_parameters_add_string(gc, &args, "driver", "xen-pci-passthrough");
+    libxl__qmp_param_add_string(gc, &args, "driver", "xen-pci-passthrough");
     QMP_PARAMETERS_SPRINTF(&args, "id", PCI_PT_QDEV_ID,
                            pcidev->bus, pcidev->dev, pcidev->func);
-    qmp_parameters_add_string(gc, &args, "hostaddr", hostaddr);
+    libxl__qmp_param_add_string(gc, &args, "hostaddr", hostaddr);
     if (pcidev->vdevfn) {
         QMP_PARAMETERS_SPRINTF(&args, "addr", "%x.%x",
                                PCI_SLOT(pcidev->vdevfn), PCI_FUNC(pcidev->vdevfn));
@@ -984,7 +981,7 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
      * reason to set the flag so this is ok.
      */
     if (pcidev->permissive)
-        qmp_parameters_add_bool(gc, &args, "permissive", true);
+        libxl__qmp_param_add_bool(gc, &args, "permissive", true);
 
     rc = qmp_synchronous_send(qmp, "device_add", args,
                               NULL, NULL, qmp->timeout);
@@ -1001,7 +998,7 @@ static int qmp_device_del(libxl__gc *gc, int domid, char *id)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_string(gc, &args, "id", id);
+    libxl__qmp_param_add_string(gc, &args, "id", id);
     return qmp_run_command(gc, domid, "device_del", args, NULL, NULL);
 }
 
@@ -1023,7 +1020,7 @@ int libxl__qmp_restore(libxl__gc *gc, int domid, const char *state_file)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_string(gc, &args, "filename", state_file);
+    libxl__qmp_param_add_string(gc, &args, "filename", state_file);
 
     return qmp_run_command(gc, domid, "xen-load-devices-state", args,
                            NULL, NULL);
@@ -1035,10 +1032,10 @@ static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
     libxl__json_object *args = NULL;
     int rc = 0;
 
-    qmp_parameters_add_string(gc, &args, "device", device);
-    qmp_parameters_add_string(gc, &args, "target", target);
+    libxl__qmp_param_add_string(gc, &args, "device", device);
+    libxl__qmp_param_add_string(gc, &args, "target", target);
     if (arg) {
-        qmp_parameters_add_string(gc, &args, "arg", arg);
+        libxl__qmp_param_add_string(gc, &args, "arg", arg);
     }
 
     rc = qmp_synchronous_send(qmp, "change", args,
@@ -1056,7 +1053,7 @@ int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_bool(gc, &args, "enable", enable);
+    libxl__qmp_param_add_bool(gc, &args, "enable", enable);
 
     return qmp_run_command(gc, domid, "xen-set-global-dirty-log", args,
                            NULL, NULL);
@@ -1073,8 +1070,8 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
     if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
         return qmp_run_command(gc, domid, "eject", args, NULL, NULL);
     } else {
-        qmp_parameters_add_string(gc, &args, "target", disk->pdev_path);
-        qmp_parameters_add_string(gc, &args, "arg",
+        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);
     }
@@ -1084,7 +1081,7 @@ int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int idx)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_integer(gc, &args, "id", idx);
+    libxl__qmp_param_add_integer(gc, &args, "id", idx);
 
     return qmp_run_command(gc, domid, "cpu-add", args, NULL, NULL);
 }
@@ -1142,10 +1139,10 @@ int libxl__qmp_nbd_server_start(libxl__gc *gc, int domid,
      *   }
      * }
      */
-    qmp_parameters_add_string(gc, &data, "host", host);
-    qmp_parameters_add_string(gc, &data, "port", port);
+    libxl__qmp_param_add_string(gc, &data, "host", host);
+    libxl__qmp_param_add_string(gc, &data, "port", port);
 
-    qmp_parameters_add_string(gc, &addr, "type", "inet");
+    libxl__qmp_param_add_string(gc, &addr, "type", "inet");
     qmp_parameters_common_add(gc, &addr, "data", data);
 
     qmp_parameters_common_add(gc, &args, "addr", addr);
@@ -1157,8 +1154,8 @@ int libxl__qmp_nbd_server_add(libxl__gc *gc, int domid, const char *disk)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_string(gc, &args, "device", disk);
-    qmp_parameters_add_bool(gc, &args, "writable", true);
+    libxl__qmp_param_add_string(gc, &args, "device", disk);
+    libxl__qmp_param_add_bool(gc, &args, "writable", true);
 
     return qmp_run_command(gc, domid, "nbd-server-add", args, NULL, NULL);
 }
@@ -1167,8 +1164,8 @@ int libxl__qmp_start_replication(libxl__gc *gc, int domid, bool primary)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_bool(gc, &args, "enable", true);
-    qmp_parameters_add_bool(gc, &args, "primary", primary);
+    libxl__qmp_param_add_bool(gc, &args, "enable", true);
+    libxl__qmp_param_add_bool(gc, &args, "primary", primary);
 
     return qmp_run_command(gc, domid, "xen-set-replication", args, NULL, NULL);
 }
@@ -1189,8 +1186,8 @@ int libxl__qmp_stop_replication(libxl__gc *gc, int domid, bool primary)
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_bool(gc, &args, "enable", false);
-    qmp_parameters_add_bool(gc, &args, "primary", primary);
+    libxl__qmp_param_add_bool(gc, &args, "enable", false);
+    libxl__qmp_param_add_bool(gc, &args, "primary", primary);
 
     return qmp_run_command(gc, domid, "xen-set-replication", args, NULL, NULL);
 }
@@ -1205,11 +1202,11 @@ int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid, const char *parent,
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_string(gc, &args, "parent", parent);
+    libxl__qmp_param_add_string(gc, &args, "parent", parent);
     if (child)
-        qmp_parameters_add_string(gc, &args, "child", child);
+        libxl__qmp_param_add_string(gc, &args, "child", child);
     if (node)
-        qmp_parameters_add_string(gc, &args, "node", node);
+        libxl__qmp_param_add_string(gc, &args, "node", node);
 
     return qmp_run_command(gc, domid, "x-blockdev-change", args, NULL, NULL);
 }
@@ -1246,7 +1243,7 @@ int libxl__qmp_hmp(libxl__gc *gc, int domid, const char *command_line,
 {
     libxl__json_object *args = NULL;
 
-    qmp_parameters_add_string(gc, &args, "command-line", command_line);
+    libxl__qmp_param_add_string(gc, &args, "command-line", command_line);
 
     return qmp_run_command(gc, domid, "human-monitor-command", args,
                            hmp_callback, output);
@@ -1383,7 +1380,7 @@ static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
      * the save operation is for a live migration rather than for taking a
      * snapshot. */
     if (qmp_ev_qemu_compare_version(ev, 2, 11, 0) >= 0)
-        qmp_parameters_add_bool(gc, &args, "live", dsps->live);
+        libxl__qmp_param_add_bool(gc, &args, "live", dsps->live);
     QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", fdset);
     rc = libxl__ev_qmp_send(gc, ev, "xen-save-devices-state", args);
     if (rc)
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 9/9] libxl_disk: Use ev_qmp in libxl_cdrom_insert

[Anthony PERARD]

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

The "opaque" parameter 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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---

Notes:
    v2:
    - acked
    - assert that there's no payload_fd before openning a cdrom file
    - renamed flag `asynchronous_callback' -> `has_callback'

 tools/libxl/libxl_disk.c     | 126 +++++++++++++++++++++++++++--------
 tools/libxl/libxl_internal.h |   1 -
 tools/libxl/libxl_qmp.c      |  18 -----
 3 files changed, 100 insertions(+), 45 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index 7328a03e8a..a95719ce9e 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -650,14 +650,17 @@ typedef struct {
     libxl__ev_lock qmp_lock;
     int dm_ver;
     libxl__ev_time time;
+    libxl__ev_qmp qmp;
 } libxl__cdrom_insert_state;
 
 static void cdrom_insert_lock_acquired(libxl__egc *, libxl__ev_lock *,
                                        int rc);
-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_timout(libxl__egc *egc, libxl__ev_time *ev,
                                 const struct timeval *requested_abs,
                                 int rc);
@@ -684,6 +687,10 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     cis->qmp_lock.ao = ao;
     cis->qmp_lock.domid = domid;
     libxl__ev_time_init(&cis->time);
+    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) {
@@ -757,26 +764,22 @@ static void cdrom_insert_lock_acquired(libxl__egc *egc,
                                      LIBXL_HOTPLUG_TIMEOUT * 1000);
     if (rc) 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, cis->disk->vdev);
-        disk_empty.pdev_path = libxl__strdup(NOGC, "");
-        disk_empty.is_cdrom = 1;
-        libxl__device_disk_setdefault(gc, cis->domid, &disk_empty, false);
+        libxl__json_object *args = NULL;
+        int devid = libxl__device_disk_dev_number(cis->disk->vdev,
+                                                  NULL, NULL);
 
-        rc = libxl__qmp_insert_cdrom(gc, cis->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;
+    } else {
+        cdrom_insert_ejected(egc, &cis->qmp, NULL, 0); /* must be last */
     }
-
-    cdrom_insert_ejected(egc, cis); /* must be last */
     return;
 
 out:
@@ -784,10 +787,12 @@ static void cdrom_insert_lock_acquired(libxl__egc *egc,
 }
 
 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;
@@ -795,6 +800,7 @@ static void cdrom_insert_ejected(libxl__egc *egc,
     xs_transaction_t t = XBT_NULL;
     char *tmp;
     libxl_domain_config d_config;
+    bool has_callback = false;
 
     /* convenience aliases */
     libxl_domid domid = cis->domid;
@@ -802,6 +808,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);
@@ -857,9 +865,29 @@ 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;
+
+        assert(qmp->payload_fd == -1);
+        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;
+        has_callback = true;
+    } else {
+        has_callback = false;
     }
 
     rc = 0;
@@ -870,16 +898,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 (!has_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;
@@ -894,6 +964,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);
@@ -977,6 +1049,8 @@ static void cdrom_insert_done(libxl__egc *egc,
     EGC_GC;
 
     libxl__ev_time_deregister(gc, &cis->time);
+    libxl__ev_qmp_dispose(gc, &cis->qmp);
+    if (cis->qmp.payload_fd >= 0) close(cis->qmp.payload_fd);
     libxl__ev_unlock(gc, &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 2dc5a31755..9361db0e2c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1959,7 +1959,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;
-- 
Anthony PERARD


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP

[Ian Jackson]

Anthony PERARD writes ("[PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP"):
> The current lock `domain_userdata_lock' can't be used when modification
> to a guest is done by sending command to QEMU, this is a slow process
> and requires to call CTX_UNLOCK, which is not possible while holding
> the `domain_userdata_lock'.
> 
> To resolve this issue, we create a new lock which can take over part
> of the job of the json_lock.

Thanks.  This is basically fine.  I have only trivial comments.

> +void libxl__ev_lock_get(libxl__egc *egc, libxl__ev_lock *lock)

I wonder if this is the right name for this.  Effectively you have
called this lock "lock".  Maybe "dlock" or "devlock" or "sdlock" (slow
device lock) or something ?  Sorry for bikeshedding but hopefully
seddery will be easy.

> +static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock)
> +{
...
> +                /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
> +                LOGED(ERROR, domid,
> +                      "unexpected error while trying to lock %s, fd=%d, errno=%d",
> +                      lockfile, fd, errno);

LOGED prints strerror(errno) so you don't need to print the numeric
value with %d too.  That's what the E in its name is.

> +void libxl__ev_unlock(libxl__gc *gc, libxl__ev_lock *lock)
> +{
> +    int r;
> +
> +    assert(!libxl__ev_child_inuse(&lock->child));
> +
> +    /* It's important to unlink the file before releasing the lock to avoid
> +     * the following race (if unlock/close before unlink):
> +     *
> +     *   P1 LOCK                         P2 UNLOCK
> +     *   fd1 = open(lockfile)
> +     *                                   unlock(fd2)
> +     *   flock(fd1)
> +     *   fstat and stat check success
> +     *                                   unlink(lockfile)
> +     *   return lock
> +     *
> +     * In above case P1 thinks it has got hold of the lock but
> +     * actually lock is released by P2 (lockfile unlinked).
> +     */

I wonder if it would be better to refer to the other copy of this
comment by libxl__unlock_domain_userdata.

Regards,
Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 4/9] libxl: Add optimisation to ev_lock

[Ian Jackson]

Anthony PERARD writes ("[PATCH v2 4/9] libxl: Add optimisation to ev_lock"):
> It will often be the case that the lock is free to grab. So we first
> try to grab it before we have to fork. Even though in this case the
> locks are grabbed in the wrong order in the lock hierarchy (ev_lock
> should be outside of CTX_LOCK), it is fine to try without blocking. If
> that failed, we will release CTX_LOCK and try to grab both lock again
> in the right order.
> 
> That optimisation is only enabled in releases (debug=n) so the more
> complicated code with fork is actually exercised.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

[Ian Jackson]

Anthony PERARD writes ("[PATCH v2 6/9] libxl_disk: Cut libxl_cdrom_insert into steps .."):
> .. and use a new "slow" lock to avoid holding the userdata lock across
> several functions.
> 
> This patch cuts libxl_cdrom_insert into different step/function but
> there are still called synchronously. (Taking the ev_lock is the only
> step that might be asynchronous.) A later patch will call them
> asynchronously when QMP is involved.
> 
> Thee userdata lock (json_lock) use to protect against concurrent change
> of cdrom is replaced by an ev_lock which can be held across different
> CTX_LOCK sections. The json_lock is still used when reading/modifying
> the domain userdata (mandatory) and update xenstore (mostly because
> it's updated as the same time as the userdata).
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 0/9] libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv

[Ian Jackson]

Anthony PERARD writes ("[PATCH v2 0/9] libxl: New slow lock + fix libxl_cdrom_insert with QEMU depriv"):
> This patch series fix libxl_cdrom_insert to work with a depriviledge QEMU. For
> that, we need to use libxl__ev_qmp.  For that, we need a new lock because
> userdata_lock can't be used anymore.

Thanks for this and sorry for blocking you for so long!

I think this is basically ready to go.  I had some trivial comments on
3/.  After we've had a conversation about that, I guess you might
want to repost just that one patch rather than all 9.  Up to you.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP

[Anthony PERARD]

On Tue, Sep 17, 2019 at 04:44:30PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP"):
> > The current lock `domain_userdata_lock' can't be used when modification
> > to a guest is done by sending command to QEMU, this is a slow process
> > and requires to call CTX_UNLOCK, which is not possible while holding
> > the `domain_userdata_lock'.
> > 
> > To resolve this issue, we create a new lock which can take over part
> > of the job of the json_lock.
> 
> Thanks.  This is basically fine.  I have only trivial comments.
> 
> > +void libxl__ev_lock_get(libxl__egc *egc, libxl__ev_lock *lock)
> 
> I wonder if this is the right name for this.  Effectively you have
> called this lock "lock".  Maybe "dlock" or "devlock" or "sdlock" (slow
> device lock) or something ?  Sorry for bikeshedding but hopefully
> seddery will be easy.

"devlock" sounds fine. So we'll have "libxl__ev_devlock"
and "libxl__ev_devlock_get".

> > +static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_lock *lock)
> > +{
> ...
> > +                /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
> > +                LOGED(ERROR, domid,
> > +                      "unexpected error while trying to lock %s, fd=%d, errno=%d",
> > +                      lockfile, fd, errno);
> 
> LOGED prints strerror(errno) so you don't need to print the numeric
> value with %d too.  That's what the E in its name is.

Yes, simple copy-paste error, I'll remove the errno value.

> > +void libxl__ev_unlock(libxl__gc *gc, libxl__ev_lock *lock)
> > +{
> > +    int r;
> > +
> > +    assert(!libxl__ev_child_inuse(&lock->child));
> > +
> > +    /* It's important to unlink the file before releasing the lock to avoid
> > +     * the following race (if unlock/close before unlink):
> > +     *
> > +     *   P1 LOCK                         P2 UNLOCK
> > +     *   fd1 = open(lockfile)
> > +     *                                   unlock(fd2)
> > +     *   flock(fd1)
> > +     *   fstat and stat check success
> > +     *                                   unlink(lockfile)
> > +     *   return lock
> > +     *
> > +     * In above case P1 thinks it has got hold of the lock but
> > +     * actually lock is released by P2 (lockfile unlinked).
> > +     */
> 
> I wonder if it would be better to refer to the other copy of this
> comment by libxl__unlock_domain_userdata.

It would be probably fine. If the comment gets removed or the function
gets renamed, one can `git blame` to figure out what the reference is
for.

I'll replace the comment by this new one:
    /* See the rationale in libxl__unlock_domain_userdata()
     * about why we do unlink() before unlock(). */

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v2 3/9] libxl_internal: Introduce libxl__ev_lock for devices hotplug via QMP"):
> On Tue, Sep 17, 2019 at 04:44:30PM +0100, Ian Jackson wrote:
> > I wonder if this is the right name for this.  Effectively you have
> > called this lock "lock".  Maybe "dlock" or "devlock" or "sdlock" (slow
> > device lock) or something ?  Sorry for bikeshedding but hopefully
> > seddery will be easy.
> 
> "devlock" sounds fine. So we'll have "libxl__ev_devlock"
> and "libxl__ev_devlock_get".

OK.

Great, thanks.  I'll look forward to the respin.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel