[RFC,XEN,for-4.13,2/4] libxl: Introduce libxl__ev_qmplock

Commit Message

Anthony PERARD Oct. 25, 2019, 5:05 p.m. UTC

This lock will be used to prevent concurrent access the QEMU's QMP
socket. It is based on libxl__ev_devlock implementation and have the
same properties.

There is one addition to the ev_devlock API,
libxl__ev_qmplock_dispose, which allow to cancel the lock operation
while it is in Active state.

Signed-off-by: Anthony PERARD <[e-mail]>
---
 tools/libxl/libxl_internal.c | 31 ++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h | 14 ++++++++++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

Comments

Ian Jackson Oct. 28, 2019, 11:40 a.m. UTC | #1

Thanks.  I approve of the general approach, and the code reuse, but I
have some qualms about the resulting layering structure and the
boilerplate wrappers.  I have some suggestions for how this might look
better.

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 2/4] libxl: Introduce libxl__ev_qmplock"):
> This lock will be used to prevent concurrent access the QEMU's QMP
> socket. It is based on libxl__ev_devlock implementation and have the
> same properties.
...
> +void libxl__ev_qmplock_init(libxl__ev_qmplock *lock)
> +{
> +    libxl__ev_devlock_init(lock);
> +}
> +
> +void libxl__ev_qmplock_lock(libxl__egc *egc, libxl__ev_qmplock *lock)
> +{
> +    ev_lock_lock(egc, lock, "qmp-socket-lock");
> +}

This produces a lot of rather pointless functions.  Also the layering
is anomalous: one of these locks is primary and most of the calls for
the other are implemented in terms of the other.

One possible alternative approach would be as follows:

1. Rename devlock to slowlock everywhere.  Expect everyone including
   qmp to call libxl__ev_slowlock_*.

2. Perhaps, put const char *userdata_userid in the lock structure.
   Have it set by libxl__ev_slowlock_init rather than by _lock.  (This
   centralises things a bit and may reduce duplication or improve
   error messages or something.)

3. Perhaps wrap up libxl__ev_slowlock_init with two functions
   [libxl__ev_]devlock_init and libxl__ev_qmplock_init, and rename
   libxl__ev_slowlock_init to libxl__ev_slowlock_init_internal.

This avoids having to provide trivial wrappers for all the functions.
if you do all of this including (3) then the API is slightly anomalous
in that there are several distinct init functions but only one set of
operation functions but this seems OK to me.

What do you think ?

Thanks,
Ian.

Patch

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 0750b69cba61..afbb01c5722f 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -583,17 +583,25 @@  void libxl__ev_devlock_init(libxl__ev_devlock *lock)
     lock->held = false;
 }
 
+static void ev_lock_lock(libxl__egc *egc, libxl__ev_devlock *lock,
+                         const char *userdata_userid);
 static void ev_lock_prepare_fork(libxl__egc *egc, libxl__ev_devlock *lock);
 static void ev_lock_child_callback(libxl__egc *egc, libxl__ev_child *child,
                                    pid_t pid, int status);
 
 void libxl__ev_devlock_lock(libxl__egc *egc, libxl__ev_devlock *lock)
+{
+    ev_lock_lock(egc, lock, "libxl-device-changes-lock");
+}
+
+static void ev_lock_lock(libxl__egc *egc, libxl__ev_devlock *lock,
+                         const char *userdata_userid)
 {
     STATE_AO_GC(lock->ao);
     const char *lockfile;
 
     lockfile = libxl__userdata_path(gc, lock->domid,
-                                    "libxl-device-changes-lock", "l");
+                                    userdata_userid, "l");
     if (!lockfile) goto out;
     lock->path = libxl__strdup(NOGC, lockfile);
 
@@ -757,6 +765,27 @@  void libxl__ev_devlock_unlock(libxl__gc *gc, libxl__ev_devlock *lock)
     libxl__ev_devlock_init(lock);
 }
 
+void libxl__ev_qmplock_init(libxl__ev_qmplock *lock)
+{
+    libxl__ev_devlock_init(lock);
+}
+
+void libxl__ev_qmplock_lock(libxl__egc *egc, libxl__ev_qmplock *lock)
+{
+    ev_lock_lock(egc, lock, "qmp-socket-lock");
+}
+
+void libxl__ev_qmplock_unlock(libxl__gc *gc, libxl__ev_qmplock *lock)
+{
+    libxl__ev_devlock_unlock(gc, lock);
+}
+
+void libxl__ev_qmplock_dispose(libxl__gc *gc, libxl__ev_qmplock *lock)
+{
+    libxl__ev_child_kill(lock->ao, &lock->child);
+    libxl__ev_devlock_unlock(gc, lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5823890703ad..115c79d034d4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -197,6 +197,7 @@  typedef struct libxl__device_type libxl__device_type;
 typedef struct libxl__json_object libxl__json_object;
 typedef struct libxl__carefd libxl__carefd;
 typedef struct libxl__ev_devlock libxl__ev_devlock;
+typedef struct libxl__ev_devlock libxl__ev_qmplock;
 typedef struct libxl__dm_resume_state libxl__dm_resume_state;
 typedef struct libxl__ao_device libxl__ao_device;
 typedef struct libxl__multidev libxl__multidev;
@@ -4725,6 +4726,19 @@  _hidden void libxl__ev_devlock_init(libxl__ev_devlock *);
 _hidden void libxl__ev_devlock_lock(libxl__egc *, libxl__ev_devlock *);
 _hidden void libxl__ev_devlock_unlock(libxl__gc *, libxl__ev_devlock *);
 
+/* libxl__ev_qmplock
+ *
+ * See "Lock for device hotplug, qmp_lock." as it is similar but is used
+ * to regulate access the QEMU's QMP socket.
+ *
+ * libxl__ev_qmplock_dispose: Idle/Active/LockAcquired -> Idle
+ *   The callback will not be called anymore.
+ */
+_hidden void libxl__ev_qmplock_init(libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_lock(libxl__egc *, libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_unlock(libxl__gc *, libxl__ev_qmplock *);
+_hidden void libxl__ev_qmplock_dispose(libxl__gc *, libxl__ev_qmplock *);
+
 /* Send control commands over xenstore and wait for an Ack. */
 _hidden int libxl__domain_pvcontrol(libxl__egc *egc,
                                     libxl__xswait_state *pvcontrol,