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

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

[Anthony PERARD]

Hi,

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.

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

Anthony PERARD (9):
  libxl_internal: Remove lost comment
  libxl: Pointer on usage of libxl__domain_userdata_lock
  libxl_internal: Split out userdata lock function
  libxl_internal: Create new lock for devices hotplug via QMP
  libxl_disk: Reorganise libxl_cdrom_insert
  libxl_disk: Cut libxl_cdrom_insert into steps ..
  libxl: Move qmp_parameters_* prototypes to libxl_internal.h
  libxl_disk: Use ev_qmp in libxl_cdrom_insert
  libxl_disk: Implement missing timeout for libxl_cdrom_insert

 tools/libxl/libxl_disk.c     | 330 ++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.c |  77 +++++++-
 tools/libxl/libxl_internal.h |  73 ++++++--
 tools/libxl/libxl_qmp.c      |  89 ++++------
 4 files changed, 426 insertions(+), 143 deletions(-)

-- 
Anthony PERARD


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

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

[Anthony PERARD]

Hi,

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.

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

Anthony PERARD (9):
  libxl_internal: Remove lost comment
  libxl: Pointer on usage of libxl__domain_userdata_lock
  libxl_internal: Split out userdata lock function
  libxl_internal: Create new lock for devices hotplug via QMP
  libxl_disk: Reorganise libxl_cdrom_insert
  libxl_disk: Cut libxl_cdrom_insert into steps ..
  libxl: Move qmp_parameters_* prototypes to libxl_internal.h
  libxl_disk: Use ev_qmp in libxl_cdrom_insert
  libxl_disk: Implement missing timeout for libxl_cdrom_insert

 tools/libxl/libxl_disk.c     | 330 ++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.c |  77 +++++++-
 tools/libxl/libxl_internal.h |  73 ++++++--
 tools/libxl/libxl_qmp.c      |  89 ++++------
 4 files changed, 426 insertions(+), 143 deletions(-)

-- 
Anthony PERARD


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

[PATCH 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>
---
 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 44e0221284..98a1ee6159 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2744,13 +2744,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 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>
---
 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 44e0221284..98a1ee6159 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2744,13 +2744,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

Re: [PATCH 1/9] libxl_internal: Remove lost comment

[Wei Liu]

On Tue, Apr 09, 2019 at 05:45:34PM +0100, Anthony PERARD wrote:
> 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>

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

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

[Wei Liu]

On Tue, Apr 09, 2019 at 05:45:34PM +0100, Anthony PERARD wrote:
> 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>

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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 1/9] libxl_internal: Remove lost comment"):
> 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.

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

[PATCH 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>
---
 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 98a1ee6159..702acc6d5d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4500,6 +4500,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 comments about libxl__ao_device and "Algorithm for handling device
+ * removal" on how the libxl-json lock / json_lock can be used.
  */
 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 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>
---
 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 98a1ee6159..702acc6d5d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4500,6 +4500,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 comments about libxl__ao_device and "Algorithm for handling device
+ * removal" on how the libxl-json lock / json_lock can be used.
  */
 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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 2/9] libxl: Pointer on usage of libxl__domain_userdata_lock"):
> It is currently difficult to know how/when/why the userdata lock is
> supposed to be used. Add some pointers to the hotplug comments.

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

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 98a1ee6159..702acc6d5d 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4500,6 +4500,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 comments about libxl__ao_device and "Algorithm for handling device
> + * removal" on how the libxl-json lock / json_lock can be used.

Better grammar for this would be:

  + * See the comment for libxl__ao_device, and "Algorithm for
  + * handling device removal", for information about using the
  + * libxl-json lock / json_lock.

Thanks,
Ian.

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

[PATCH 3/9] libxl_internal: Split out userdata lock function

[Anthony PERARD]

We are going to create a new lock and want to reuse the same machinery.
Also, hide the detail of struct libxl__domain_userdata_lock as this is
only useful as a pointer by the rest of libxl.

No functional changes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.c | 50 +++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |  5 +---
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f492dae5ff..fa0bbc3960 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -397,21 +397,26 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
     return value;
 }
 
+typedef struct {
+    libxl__carefd *carefd;
+    char *path; /* path of the lock file itself */
+} libxl__generic_lock;
+static void libxl__unlock_generic(libxl__generic_lock * const lock);
+
 /* Portability note: this lock utilises flock(2) so a proper implementation of
  * flock(2) is required.
  */
-libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
-                                                         uint32_t domid)
+static int libxl__lock_generic(libxl__gc *gc, libxl_domid domid,
+                               libxl__generic_lock * const lock,
+                               const char *lock_name)
 {
-    libxl__domain_userdata_lock *lock = NULL;
     const char *lockfile;
     int fd;
     struct stat stab, fstab;
 
-    lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+    lockfile = libxl__userdata_path(gc, domid, lock_name, "l");
     if (!lockfile) goto out;
 
-    lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
     lock->path = libxl__strdup(NOGC, lockfile);
 
     while (true) {
@@ -464,14 +469,14 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
     if (libxl_domain_info(CTX, NULL, domid))
         goto out;
 
-    return lock;
+    return 0;
 
 out:
-    if (lock) libxl__unlock_domain_userdata(lock);
-    return NULL;
+    if (lock) libxl__unlock_generic(lock);
+    return ERROR_FAIL;
 }
 
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+static void libxl__unlock_generic(libxl__generic_lock * const lock)
 {
     /* It's important to unlink the file before closing fd to avoid
      * the following race (if close before unlink):
@@ -490,6 +495,33 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
     if (lock->path) unlink(lock->path);
     if (lock->carefd) libxl__carefd_close(lock->carefd);
     free(lock->path);
+}
+
+
+struct libxl__domain_userdata_lock {
+    libxl__generic_lock lock;
+};
+
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+                                                         uint32_t domid)
+{
+    libxl__domain_userdata_lock *lock = NULL;
+    int rc;
+
+    lock = libxl__zalloc(NOGC, sizeof(*lock));
+    rc = libxl__lock_generic(gc, domid, &lock->lock,
+                             "domain-userdata-lock");
+    if (rc) {
+        libxl__unlock_domain_userdata(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
+void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+{
+    libxl__unlock_generic(&lock->lock);
     free(lock);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 702acc6d5d..f1aefaf98a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4486,10 +4486,7 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
 int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
 
 /* Portability note: a proper flock(2) implementation is required */
-typedef struct {
-    libxl__carefd *carefd;
-    char *path; /* path of the lock file itself */
-} libxl__domain_userdata_lock;
+typedef struct libxl__domain_userdata_lock libxl__domain_userdata_lock;
 /* The CTX_LOCK must be held around uses of this lock */
 libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
                                                          uint32_t domid);
-- 
Anthony PERARD


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

[Xen-devel] [PATCH 3/9] libxl_internal: Split out userdata lock function

[Anthony PERARD]

We are going to create a new lock and want to reuse the same machinery.
Also, hide the detail of struct libxl__domain_userdata_lock as this is
only useful as a pointer by the rest of libxl.

No functional changes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.c | 50 +++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |  5 +---
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f492dae5ff..fa0bbc3960 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -397,21 +397,26 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
     return value;
 }
 
+typedef struct {
+    libxl__carefd *carefd;
+    char *path; /* path of the lock file itself */
+} libxl__generic_lock;
+static void libxl__unlock_generic(libxl__generic_lock * const lock);
+
 /* Portability note: this lock utilises flock(2) so a proper implementation of
  * flock(2) is required.
  */
-libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
-                                                         uint32_t domid)
+static int libxl__lock_generic(libxl__gc *gc, libxl_domid domid,
+                               libxl__generic_lock * const lock,
+                               const char *lock_name)
 {
-    libxl__domain_userdata_lock *lock = NULL;
     const char *lockfile;
     int fd;
     struct stat stab, fstab;
 
-    lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
+    lockfile = libxl__userdata_path(gc, domid, lock_name, "l");
     if (!lockfile) goto out;
 
-    lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
     lock->path = libxl__strdup(NOGC, lockfile);
 
     while (true) {
@@ -464,14 +469,14 @@ libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
     if (libxl_domain_info(CTX, NULL, domid))
         goto out;
 
-    return lock;
+    return 0;
 
 out:
-    if (lock) libxl__unlock_domain_userdata(lock);
-    return NULL;
+    if (lock) libxl__unlock_generic(lock);
+    return ERROR_FAIL;
 }
 
-void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+static void libxl__unlock_generic(libxl__generic_lock * const lock)
 {
     /* It's important to unlink the file before closing fd to avoid
      * the following race (if close before unlink):
@@ -490,6 +495,33 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
     if (lock->path) unlink(lock->path);
     if (lock->carefd) libxl__carefd_close(lock->carefd);
     free(lock->path);
+}
+
+
+struct libxl__domain_userdata_lock {
+    libxl__generic_lock lock;
+};
+
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+                                                         uint32_t domid)
+{
+    libxl__domain_userdata_lock *lock = NULL;
+    int rc;
+
+    lock = libxl__zalloc(NOGC, sizeof(*lock));
+    rc = libxl__lock_generic(gc, domid, &lock->lock,
+                             "domain-userdata-lock");
+    if (rc) {
+        libxl__unlock_domain_userdata(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
+void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
+{
+    libxl__unlock_generic(&lock->lock);
     free(lock);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 702acc6d5d..f1aefaf98a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4486,10 +4486,7 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl)
 int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl);
 
 /* Portability note: a proper flock(2) implementation is required */
-typedef struct {
-    libxl__carefd *carefd;
-    char *path; /* path of the lock file itself */
-} libxl__domain_userdata_lock;
+typedef struct libxl__domain_userdata_lock libxl__domain_userdata_lock;
 /* The CTX_LOCK must be held around uses of this lock */
 libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
                                                          uint32_t domid);
-- 
Anthony PERARD


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

Re: [Xen-devel] [PATCH 3/9] libxl_internal: Split out userdata lock function

[Ian Jackson]

Anthony PERARD writes ("[PATCH 3/9] libxl_internal: Split out userdata lock function"):
> We are going to create a new lock and want to reuse the same machinery.
> Also, hide the detail of struct libxl__domain_userdata_lock as this is
> only useful as a pointer by the rest of libxl.
> 
> No functional changes.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.c | 50 +++++++++++++++++++++++++++++-------
>  tools/libxl/libxl_internal.h |  5 +---
>  2 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index f492dae5ff..fa0bbc3960 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -397,21 +397,26 @@ int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
>      return value;
>  }
>  
> +typedef struct {
> +    libxl__carefd *carefd;
> +    char *path; /* path of the lock file itself */
> +} libxl__generic_lock;
> +static void libxl__unlock_generic(libxl__generic_lock * const lock);
                                                          ^
Spurious space.  (Many later occurrences, too.)

> +static int libxl__lock_generic(libxl__gc *gc, libxl_domid domid,
> +                               libxl__generic_lock * const lock,
> +                               const char *lock_name)
>  {
...
> -    if (lock) libxl__unlock_domain_userdata(lock);
> -    return NULL;
> +    if (lock) libxl__unlock_generic(lock);

I think the interface to libxl__lock_generic implies !!lock, so there
is no need to test it here.

I find the legal states of a libxl__generic_lock a bit confusing.
It's only an internal struct here, but I think we need either more
forgiving rules, or explicit commentary.

As it is, the implementation of libxl__lock_generic assumes that on
entry lock->carefd and lock->path are both NULL, and there is no
function to create such a situation.  We're relying on the callers'
memsets.

> -void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
> +static void libxl__unlock_generic(libxl__generic_lock * const lock)
>  {
>      /* It's important to unlink the file before closing fd to avoid
>       * the following race (if close before unlink):
> @@ -490,6 +495,33 @@ void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock)
>      if (lock->path) unlink(lock->path);
>      if (lock->carefd) libxl__carefd_close(lock->carefd);

And this leaves lock->path and lock->carefd containing invalid
values.  Hazardous!

How about using formal state annotations ?

(I think, having looked at the one call site, that with the current
callers all follow the unwritten rules.)

Ian.

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

[PATCH 4/9] libxl_internal: Create new 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.

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

---

Quote from Ian:
> The invariant that we want to maintain is:
>
>   * Nothing may exist in the primary config without
>     a corresponding entry in libxl-json.
[...]
> How about the following scheme.  We split the libxl-json lock into
> two.  I'm going to call them the fast lock and the slow lock.
>
>   * The fast lock is the existing libxl-json lock.
>
>   * The slow lock is outside the libxl-json lock in the lock
>     hierarchy.  It is also outside the libxl_ctx lock.  It is
>     to be acquired by an ao event callback.
>
>   * No-one may read or edit the libxl-json without holding the fast
>     lock across their read operation, or their read/modify/write
>     cycle.
>
>   * However, there are special rules for thing removal/addition, for
>     things added/removed via qmp.  Call these `qmp things'.  It is
>     permissible to add or remove a qmp thing across two separate
>     acquisitions of the fast lock, one to read the old state of the
>     thing, and one to read/modify/write to update (only) the new state
>     of the thing.  This is subject to the thing add/removal rule, from
>     before, which becomes:
>
>   * You may not cause a thing to be added to the primary config
>     unless you have held the relevant thing lock continuously
>     since ensuring that the libxl-json config describes it.
>
>   * Conversely you may not cause a thing to be removed from the
>     libxl-json unless you have held the relevant thing lock
>     continuously since ensuring the thing is absent from the primary
>     config.
>
>   * The `relevant thing lock' is the slow lock for qmp things, and the
>     fast lock for other things.
>
>   * Acquiring the fast lock fails for a destroyed domain, as at
>     present.
---
 tools/libxl/libxl_internal.c | 27 +++++++++++++++++++++++++
 tools/libxl/libxl_internal.h | 39 ++++++++++++++++++++++++++++++++----
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index fa0bbc3960..db281ac259 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -607,6 +607,33 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
+struct libxl__domain_qmp_lock {
+    libxl__generic_lock lock;
+};
+
+libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
+                                               libxl_domid domid)
+{
+    libxl__domain_qmp_lock *lock = NULL;
+    int rc;
+
+    lock = libxl__zalloc(NOGC, sizeof(*lock));
+    rc = libxl__lock_generic(gc, domid, &lock->lock,
+                             "libxl-device-changes-lock");
+    if (rc) {
+        libxl__unlock_domain_qmp(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
+void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock)
+{
+    libxl__unlock_generic(&lock->lock);
+    free(lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f1aefaf98a..43b44f2c30 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2561,6 +2561,21 @@ typedef struct libxl__ao_device libxl__ao_device;
 typedef struct libxl__multidev libxl__multidev;
 typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
 
+/*
+ * Lock for device hotplug, qmp_lock.
+ *
+ * This lock is outside the json_lock lock in lock hierarchy.
+ * It is also outside the libxl_ctx lock.
+ * It is to be acquired by an ao event callback.
+ *
+ * It is to be acquired when adding/removing devices or making changes
+ * to them via QMP, when this is a slow operation.
+ */
+typedef struct libxl__domain_qmp_lock libxl__domain_qmp_lock;
+_hidden libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
+                                                           libxl_domid domid);
+_hidden void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock);
+
 /* This functions sets the necessary libxl__ao_device struct values to use
  * safely inside functions. It marks the operation as "active"
  * since we need to be sure that all device status structs are set
@@ -2749,11 +2764,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)
@@ -2768,6 +2783,22 @@ struct libxl__multidev {
  *       end for loop
  *   unlock json config
  *
+ * Or in case QEMU is the primary config, this pattern can be use:
+ *   lock qmp (qmp_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
+ *
  * Device removal routines are not touched.
  *
  * Here is the proof that we always maintain that invariant and we
-- 
Anthony PERARD


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

[Xen-devel] [PATCH 4/9] libxl_internal: Create new 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.

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

---

Quote from Ian:
> The invariant that we want to maintain is:
>
>   * Nothing may exist in the primary config without
>     a corresponding entry in libxl-json.
[...]
> How about the following scheme.  We split the libxl-json lock into
> two.  I'm going to call them the fast lock and the slow lock.
>
>   * The fast lock is the existing libxl-json lock.
>
>   * The slow lock is outside the libxl-json lock in the lock
>     hierarchy.  It is also outside the libxl_ctx lock.  It is
>     to be acquired by an ao event callback.
>
>   * No-one may read or edit the libxl-json without holding the fast
>     lock across their read operation, or their read/modify/write
>     cycle.
>
>   * However, there are special rules for thing removal/addition, for
>     things added/removed via qmp.  Call these `qmp things'.  It is
>     permissible to add or remove a qmp thing across two separate
>     acquisitions of the fast lock, one to read the old state of the
>     thing, and one to read/modify/write to update (only) the new state
>     of the thing.  This is subject to the thing add/removal rule, from
>     before, which becomes:
>
>   * You may not cause a thing to be added to the primary config
>     unless you have held the relevant thing lock continuously
>     since ensuring that the libxl-json config describes it.
>
>   * Conversely you may not cause a thing to be removed from the
>     libxl-json unless you have held the relevant thing lock
>     continuously since ensuring the thing is absent from the primary
>     config.
>
>   * The `relevant thing lock' is the slow lock for qmp things, and the
>     fast lock for other things.
>
>   * Acquiring the fast lock fails for a destroyed domain, as at
>     present.
---
 tools/libxl/libxl_internal.c | 27 +++++++++++++++++++++++++
 tools/libxl/libxl_internal.h | 39 ++++++++++++++++++++++++++++++++----
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index fa0bbc3960..db281ac259 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -607,6 +607,33 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
+struct libxl__domain_qmp_lock {
+    libxl__generic_lock lock;
+};
+
+libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
+                                               libxl_domid domid)
+{
+    libxl__domain_qmp_lock *lock = NULL;
+    int rc;
+
+    lock = libxl__zalloc(NOGC, sizeof(*lock));
+    rc = libxl__lock_generic(gc, domid, &lock->lock,
+                             "libxl-device-changes-lock");
+    if (rc) {
+        libxl__unlock_domain_qmp(lock);
+        return NULL;
+    }
+
+    return lock;
+}
+
+void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock)
+{
+    libxl__unlock_generic(&lock->lock);
+    free(lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f1aefaf98a..43b44f2c30 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2561,6 +2561,21 @@ typedef struct libxl__ao_device libxl__ao_device;
 typedef struct libxl__multidev libxl__multidev;
 typedef void libxl__device_callback(libxl__egc*, libxl__ao_device*);
 
+/*
+ * Lock for device hotplug, qmp_lock.
+ *
+ * This lock is outside the json_lock lock in lock hierarchy.
+ * It is also outside the libxl_ctx lock.
+ * It is to be acquired by an ao event callback.
+ *
+ * It is to be acquired when adding/removing devices or making changes
+ * to them via QMP, when this is a slow operation.
+ */
+typedef struct libxl__domain_qmp_lock libxl__domain_qmp_lock;
+_hidden libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
+                                                           libxl_domid domid);
+_hidden void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock);
+
 /* This functions sets the necessary libxl__ao_device struct values to use
  * safely inside functions. It marks the operation as "active"
  * since we need to be sure that all device status structs are set
@@ -2749,11 +2764,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)
@@ -2768,6 +2783,22 @@ struct libxl__multidev {
  *       end for loop
  *   unlock json config
  *
+ * Or in case QEMU is the primary config, this pattern can be use:
+ *   lock qmp (qmp_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
+ *
  * Device removal routines are not touched.
  *
  * Here is the proof that we always maintain that invariant and we
-- 
Anthony PERARD


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

Re: [Xen-devel] [PATCH 4/9] libxl_internal: Create new lock for devices hotplug via QMP

[Ian Jackson]

Anthony PERARD writes ("[PATCH 4/9] libxl_internal: Create new 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'.
...
> +struct libxl__domain_qmp_lock {
> +    libxl__generic_lock lock;
> +};
> +
> +libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
> +                                               libxl_domid domid)
> +{
> +    libxl__domain_qmp_lock *lock = NULL;
> +    int rc;
> +
> +    lock = libxl__zalloc(NOGC, sizeof(*lock));
> +    rc = libxl__lock_generic(gc, domid, &lock->lock,
> +                             "libxl-device-changes-lock");
> +    if (rc) {
> +        libxl__unlock_domain_qmp(lock);
> +        return NULL;
> +    }
> +
> +    return lock;

This is almost identical to the libxl__lock_domain_userdata which
appeared in the previous patch.  That suggests that the factorisation
here is at the wrong layer.

> +void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock)
> +{
> +    libxl__unlock_generic(&lock->lock);
> +    free(lock);
> +}

This is completely identical to libxl__unlock_domain_userdata.  The
only reason we have to two of these is so that the two locks are
distinguished by the type of the lock struct.  That means that you
have to call
   libxl__unlock_domain_qmp(foo->qmp_lock)
but
   libxl__unlock_domain_userdate(foo->userdata_lock)
or some such, and the compiler will spot if you write
   libxl__unlock_domain_userdata(foo->qmp_lock)
or something.  But is it really useful to have to write the `qmp' vs
`userdata' thing twice ?

> + * It is to be acquired by an ao event callback.

I think there is a worse problem here, though.  This lock is probably
*inside* some lock from the caller.  So usual libxl rules apply and
you may not block to acquaire it.

Ie libxl__lock_domain_qmp must be one of these things that takes a
little ev state struct and makes a callback.

At the syscall level, acquiring an fcntl lock cannot be selected on.
The options are to poll, or to spawn a thread, or to fork.

Currently libxl does not call pthread_create, although maybe it could
do.  To safely create a thread you have to do a dance with
sigprocmask, to avoid signal delivery onto your thread.

Maybe it would be better to try once with LOCK_NB, and to fork if this
is not successful.  But it would be simpler to always fork...

> + * Or in case QEMU is the primary config, this pattern can be use:
> + *   lock qmp (qmp_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

This doesn't show the ctx locks but I think that is fine.  So I think
this description is correct.

Ian.

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

Re: [Xen-devel] [PATCH 4/9] libxl_internal: Create new lock for devices hotplug via QMP

[Anthony PERARD]

On Tue, Jun 04, 2019 at 06:11:23PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH 4/9] libxl_internal: Create new 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'.
> ...
> > +struct libxl__domain_qmp_lock {
> > +    libxl__generic_lock lock;
> > +};
> > +
> > +libxl__domain_qmp_lock *libxl__lock_domain_qmp(libxl__gc *gc,
> > +                                               libxl_domid domid)
> > +{
> > +    libxl__domain_qmp_lock *lock = NULL;
> > +    int rc;
> > +
> > +    lock = libxl__zalloc(NOGC, sizeof(*lock));
> > +    rc = libxl__lock_generic(gc, domid, &lock->lock,
> > +                             "libxl-device-changes-lock");
> > +    if (rc) {
> > +        libxl__unlock_domain_qmp(lock);
> > +        return NULL;
> > +    }
> > +
> > +    return lock;
> 
> This is almost identical to the libxl__lock_domain_userdata which
> appeared in the previous patch.  That suggests that the factorisation
> here is at the wrong layer.
> 
> > +void libxl__unlock_domain_qmp(libxl__domain_qmp_lock *lock)
> > +{
> > +    libxl__unlock_generic(&lock->lock);
> > +    free(lock);
> > +}
> 
> This is completely identical to libxl__unlock_domain_userdata.  The
> only reason we have to two of these is so that the two locks are
> distinguished by the type of the lock struct.  That means that you
> have to call
>    libxl__unlock_domain_qmp(foo->qmp_lock)
> but
>    libxl__unlock_domain_userdate(foo->userdata_lock)
> or some such, and the compiler will spot if you write
>    libxl__unlock_domain_userdata(foo->qmp_lock)
> or something.  But is it really useful to have to write the `qmp' vs
> `userdata' thing twice ?

So, instead of that interface, how about a different one that uses the
same C type for both kind of lock?

    libxl__lock *libxl__lock_domain_userdata(libxl__gc *, libxl_domid);
    libxl__lock *libxl__lock_domain_qmp(libxl__gc *, libxl_domid);
    void libxl__unlock(libxl__lock *);

Or maybe avoid having two functions for locking and use a #define/enum
instead:
    libxl__lock_domain(gc, domid, LOCK_USERDATA);
    libxl__lock_domain(gc, domid, LOCK_QMP);

I'm not sur what should the last parameter be. Either a string or a
enum. An enum would be better because the lock filename would be all in
a single location in the source code.

What do you think? Would the first proposal be enough to avoid having to
write `userdata' or `qmp' twice on unlock?

> > + * It is to be acquired by an ao event callback.
> 
> I think there is a worse problem here, though.  This lock is probably
> *inside* some lock from the caller.  So usual libxl rules apply and
> you may not block to acquaire it.
> 
> Ie libxl__lock_domain_qmp must be one of these things that takes a
> little ev state struct and makes a callback.
> 
> At the syscall level, acquiring an fcntl lock cannot be selected on.
> The options are to poll, or to spawn a thread, or to fork.
> 
> Currently libxl does not call pthread_create, although maybe it could
> do.  To safely create a thread you have to do a dance with
> sigprocmask, to avoid signal delivery onto your thread.
> 
> Maybe it would be better to try once with LOCK_NB, and to fork if this
> is not successful.  But it would be simpler to always fork...

After our talk IRL, I'll go the fork route.
Also, I'm thinking to always fork when libxl is built with "debug=y",
and allow the optimisation of trying first with LOCK_NB when built with
"debug=n", so the forked code will actually be tested regulary.

Thanks,

-- 
Anthony PERARD

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

Re: [Xen-devel] [PATCH 4/9] libxl_internal: Create new lock for devices hotplug via QMP

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 4/9] libxl_internal: Create new lock for devices hotplug via QMP"):
> So, instead of that interface, how about a different one that uses the
> same C type for both kind of lock?
> 
>     libxl__lock *libxl__lock_domain_userdata(libxl__gc *, libxl_domid);
>     libxl__lock *libxl__lock_domain_qmp(libxl__gc *, libxl_domid);
>     void libxl__unlock(libxl__lock *);

I think that would be fine.

> Or maybe avoid having two functions for locking and use a #define/enum
> instead:
>     libxl__lock_domain(gc, domid, LOCK_USERDATA);
>     libxl__lock_domain(gc, domid, LOCK_QMP);

Or this.

But I think maybe this conversation will be superseded by the need to
redo the implementation which will result in a totally different API
for the slow lock, and probably a different state struct.

> What do you think? Would the first proposal be enough to avoid having to
> write `userdata' or `qmp' twice on unlock?

I don't *mind* the writing `userdata' or `qmp' twice.  I just
discounting it as a *benefit*.  I mind the duplicated implementation
code.

> > Maybe it would be better to try once with LOCK_NB, and to fork if this
> > is not successful.  But it would be simpler to always fork...
> 
> After our talk IRL, I'll go the fork route.
> Also, I'm thinking to always fork when libxl is built with "debug=y",
> and allow the optimisation of trying first with LOCK_NB when built with
> "debug=n", so the forked code will actually be tested regulary.

Good plan.

Ian.

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

[PATCH 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>
---
 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 79e30f8d52..f7a1b75ae1 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -666,7 +666,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;
@@ -677,16 +677,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;
@@ -740,23 +733,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.
      */
@@ -769,11 +745,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;
@@ -800,6 +792,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;
 
@@ -813,6 +809,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;
@@ -850,7 +857,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 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>
---
 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 79e30f8d52..f7a1b75ae1 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -666,7 +666,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;
@@ -677,16 +677,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;
@@ -740,23 +733,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.
      */
@@ -769,11 +745,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;
@@ -800,6 +792,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;
 
@@ -813,6 +809,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;
@@ -850,7 +857,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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 5/9] libxl_disk: Reorganise libxl_cdrom_insert"):
> This is in preparation of cutting libxl_cdrom_insert into several
> functions to allow asynchronous callbacks.

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

[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

[Anthony PERARD]

.. and use a new "slow" lock.

This patch cut libxl_cdrom_insert into different step/function but there
are still called synchronously. A later patch will call them
asynchronously when QMP is involved.

The json_lock has been replaced by the qmp_lock for protection against
concurrent changes to the cdrom. The json_lock is now only used when
reading/modifying the domain userdata.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_disk.c | 172 ++++++++++++++++++++++++++++++---------
 1 file changed, 135 insertions(+), 37 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index f7a1b75ae1..14dfc67971 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -661,24 +661,38 @@ 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__domain_qmp_lock *qmp_lock;
+    int dm_ver;
+} 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_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_device_disk *disks = NULL;
+    int rc;
+    libxl__cdrom_insert_state *cis;
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_saved);
-    libxl_device_disk_copy(ctx, &disk_saved, disk);
+    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_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -697,8 +711,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;
@@ -727,17 +741,8 @@ 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;
-
-    be_path = libxl__device_backend_path(gc, &device);
-    libxl_path = libxl__device_libxl_path(gc, &device);
-
-    /* Note: CTX lock is already held at this point so lock hierarchy
-     * is maintained.
-     */
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
+    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);
+    if (!cis->qmp_lock) {
         rc = ERROR_LOCK_FAIL;
         goto out;
     }
@@ -746,7 +751,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
      * 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);
@@ -761,6 +766,48 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc) goto out;
     }
 
+    rc = 0;
+
+out:
+    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 */
+    }
+    return AO_INPROGRESS;
+}
+
+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));
@@ -799,16 +846,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));
@@ -849,21 +946,22 @@ 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);
-
-    if (rc) return AO_CREATE_FAIL(rc);
-    return AO_INPROGRESS;
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc)
+{
+    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);
 }
 
 /* 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 6/9] libxl_disk: Cut libxl_cdrom_insert into steps ..

[Anthony PERARD]

.. and use a new "slow" lock.

This patch cut libxl_cdrom_insert into different step/function but there
are still called synchronously. A later patch will call them
asynchronously when QMP is involved.

The json_lock has been replaced by the qmp_lock for protection against
concurrent changes to the cdrom. The json_lock is now only used when
reading/modifying the domain userdata.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_disk.c | 172 ++++++++++++++++++++++++++++++---------
 1 file changed, 135 insertions(+), 37 deletions(-)

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index f7a1b75ae1..14dfc67971 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -661,24 +661,38 @@ 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__domain_qmp_lock *qmp_lock;
+    int dm_ver;
+} 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_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_device_disk *disks = NULL;
+    int rc;
+    libxl__cdrom_insert_state *cis;
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_saved);
-    libxl_device_disk_copy(ctx, &disk_saved, disk);
+    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_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -697,8 +711,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;
@@ -727,17 +741,8 @@ 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;
-
-    be_path = libxl__device_backend_path(gc, &device);
-    libxl_path = libxl__device_libxl_path(gc, &device);
-
-    /* Note: CTX lock is already held at this point so lock hierarchy
-     * is maintained.
-     */
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
+    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);
+    if (!cis->qmp_lock) {
         rc = ERROR_LOCK_FAIL;
         goto out;
     }
@@ -746,7 +751,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
      * 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);
@@ -761,6 +766,48 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc) goto out;
     }
 
+    rc = 0;
+
+out:
+    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 */
+    }
+    return AO_INPROGRESS;
+}
+
+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));
@@ -799,16 +846,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));
@@ -849,21 +946,22 @@ 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);
-
-    if (rc) return AO_CREATE_FAIL(rc);
-    return AO_INPROGRESS;
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc)
+{
+    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);
 }
 
 /* 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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps .."):
> This patch cut libxl_cdrom_insert into different step/function but there
> are still called synchronously. A later patch will call them
> asynchronously when QMP is involved.
> 
> The json_lock has been replaced by the qmp_lock for protection against
> concurrent changes to the cdrom. The json_lock is now only used when
> reading/modifying the domain userdata.


Your documentation for the qmp lock, taken roughly from my idea, is
that the qmp lock covers modifying things via qmp.  But I think here
you use it for updates via xenstore ?

I think this is OK because what matters is that any one thing is
always covered by the same lock - and here the cdrom is is a "thing".
But I think this means the commentary is wrong ?


> -    /* Note: CTX lock is already held at this point so lock hierarchy
> -     * is maintained.
> -     */
> -    lock = libxl__lock_domain_userdata(gc, domid);
> -    if (!lock) {
> +    cis->qmp_lock = libxl__lock_domain_qmp(gc, domid);

I think this is in fact precisely backwards.  The lock hierarchy is
*violated* here.  The qmp lock is supposed to be outside the ctx lock.

There will have to be an entrypoint for taking the qmp lock which
takes a gc and which makes a callback later.


AFAICT apart from these two aspects the rest of this code
reorganisation is good.

Thanks,
Ian.

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

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

[Anthony PERARD]

On Tue, Jun 04, 2019 at 06:29:14PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH 6/9] libxl_disk: Cut libxl_cdrom_insert into steps .."):
> > This patch cut libxl_cdrom_insert into different step/function but there
> > are still called synchronously. A later patch will call them
> > asynchronously when QMP is involved.
> > 
> > The json_lock has been replaced by the qmp_lock for protection against
> > concurrent changes to the cdrom. The json_lock is now only used when
> > reading/modifying the domain userdata.
> 
> 
> Your documentation for the qmp lock, taken roughly from my idea, is
> that the qmp lock covers modifying things via qmp.  But I think here
> you use it for updates via xenstore ?

That xenstore code update is kind of weird, libxl writes it but nothing
really reads it.

For the eject part of the xenstore update, it probably doesn't matter if
the json_lock is held or not. But for the insert part of the xenstore
update, libxl writes the domain config while in the middle of the xenstore
transaction. So in the second case, the json_lock needs to be held.

> I think this is OK because what matters is that any one thing is
> always covered by the same lock - and here the cdrom is is a "thing".
> But I think this means the commentary is wrong ?

I can try to rewrite the commentary to make it less wrong. Or change the
code so the comment applies to it. (I'll go with rewriting the patch
description for now.)

-- 
Anthony PERARD

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

[PATCH 7/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>
---
 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 43b44f2c30..9401f988e5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -469,6 +469,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 7/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>
---
 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 43b44f2c30..9401f988e5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -469,6 +469,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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 7/9] libxl: Move qmp_parameters_* prototypes to libxl_internal.h"):
> .. and rename them to libxl__qmp_param_*.
> 
> This is to allow other files than libxl_qmp.c to make QMP calls with
> parameters.

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

[PATCH 8/9] libxl_disk: Use ev_qmp in libxl_cdrom_insert

[Anthony PERARD]

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

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;
-- 
Anthony PERARD


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

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

[Anthony PERARD]

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

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;
-- 
Anthony PERARD


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

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

[Ian Jackson]

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.

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

[PATCH 9/9] libxl_disk: Implement missing timeout for libxl_cdrom_insert

[Anthony PERARD]

Since the previous patch "libxl_disk: Use ev_qmp in libxl_cdrom_insert",
there are no kind of timeout anymore, add one back.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 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 785c8a27e7..8ccbdf0da0 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -669,6 +669,7 @@ typedef struct {
     libxl__domain_qmp_lock *qmp_lock;
     int dm_ver;
     libxl__ev_qmp qmp;
+    libxl__ev_time time;
 } libxl__cdrom_insert_state;
 
 static void cdrom_insert_ejected(libxl__egc *egc, libxl__ev_qmp *,
@@ -677,6 +678,9 @@ 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);
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc);
@@ -697,6 +701,7 @@ 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_time_init(&cis->time);
     libxl__ev_qmp_init(&cis->qmp);
     cis->qmp.ao = ao;
     cis->qmp.domid = domid;
@@ -755,6 +760,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         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.
      * JSON is not updated.
      */
@@ -1030,12 +1040,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_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);
-- 
Anthony PERARD


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

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

[Anthony PERARD]

Since the previous patch "libxl_disk: Use ev_qmp in libxl_cdrom_insert",
there are no kind of timeout anymore, add one back.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 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 785c8a27e7..8ccbdf0da0 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -669,6 +669,7 @@ typedef struct {
     libxl__domain_qmp_lock *qmp_lock;
     int dm_ver;
     libxl__ev_qmp qmp;
+    libxl__ev_time time;
 } libxl__cdrom_insert_state;
 
 static void cdrom_insert_ejected(libxl__egc *egc, libxl__ev_qmp *,
@@ -677,6 +678,9 @@ 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);
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc);
@@ -697,6 +701,7 @@ 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_time_init(&cis->time);
     libxl__ev_qmp_init(&cis->qmp);
     cis->qmp.ao = ao;
     cis->qmp.domid = domid;
@@ -755,6 +760,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         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.
      * JSON is not updated.
      */
@@ -1030,12 +1040,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_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);
-- 
Anthony PERARD


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

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

[Ian Jackson]

Anthony PERARD writes ("[PATCH 9/9] libxl_disk: Implement missing timeout for libxl_cdrom_insert"):
> Since the previous patch "libxl_disk: Use ev_qmp in libxl_cdrom_insert",
> there are no kind of timeout anymore, add one back.

Hrm.  The patch itself looks good, so

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

But I wonder if this could somehow be placed earlier to preserve
bisectability.

Thanks,
Ian.

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

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

[Anthony PERARD]

On Tue, Jun 04, 2019 at 06:47:32PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH 9/9] libxl_disk: Implement missing timeout for libxl_cdrom_insert"):
> > Since the previous patch "libxl_disk: Use ev_qmp in libxl_cdrom_insert",
> > there are no kind of timeout anymore, add one back.
> 
> Hrm.  The patch itself looks good, so
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> But I wonder if this could somehow be placed earlier to preserve
> bisectability.

I think it would be possible to place the patch right after "libxl_disk:
Cut libxl_cdrom_insert into steps", even though the timeout will never
get a chance to actually fire. (Before "Use ev_qmp ...", everything is
synchronous.)

-- 
Anthony PERARD

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

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

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 9/9] libxl_disk: Implement missing timeout for libxl_cdrom_insert"):
> On Tue, Jun 04, 2019 at 06:47:32PM +0100, Ian Jackson wrote:
> > But I wonder if this could somehow be placed earlier to preserve
> > bisectability.
> 
> I think it would be possible to place the patch right after "libxl_disk:
> Cut libxl_cdrom_insert into steps", even though the timeout will never
> get a chance to actually fire. (Before "Use ev_qmp ...", everything is
> synchronous.)

Sounds good to me.

Ian.

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

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

[Anthony PERARD]

Ping?

On Tue, Apr 09, 2019 at 05:45:33PM +0100, Anthony PERARD wrote:
> 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.
> 
> Patch series available in this git branch:
> https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-slow-lock-v1
> 
> Anthony PERARD (9):
>   libxl_internal: Remove lost comment
>   libxl: Pointer on usage of libxl__domain_userdata_lock
>   libxl_internal: Split out userdata lock function
>   libxl_internal: Create new lock for devices hotplug via QMP
>   libxl_disk: Reorganise libxl_cdrom_insert
>   libxl_disk: Cut libxl_cdrom_insert into steps ..
>   libxl: Move qmp_parameters_* prototypes to libxl_internal.h
>   libxl_disk: Use ev_qmp in libxl_cdrom_insert
>   libxl_disk: Implement missing timeout for libxl_cdrom_insert
> 
>  tools/libxl/libxl_disk.c     | 330 ++++++++++++++++++++++++++++-------
>  tools/libxl/libxl_internal.c |  77 +++++++-
>  tools/libxl/libxl_internal.h |  73 ++++++--
>  tools/libxl/libxl_qmp.c      |  89 ++++------
>  4 files changed, 426 insertions(+), 143 deletions(-)

-- 
Anthony PERARD

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