[Xen-devel] [RFC XEN PATCH for-4.13 4/4] libxl_qmp: Have a lock for QMP socket access

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

FIXME: The case where something failed when trying to acquired the
   lock isn't handled yet.

This patch workaround the fact that it's not possible to connect
multiple time to a single QMP socket. QEMU listen on the socket with
a backlog value of 1, which mean that on Linux when concurrent thread
call connect() on the socket, they get EAGAIN.

To work around this, we use a new lock.

Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
---
 tools/libxl/libxl_internal.h | 29 ++++++++++------
 tools/libxl/libxl_qmp.c      | 65 +++++++++++++++++++++++++++++-------
 2 files changed, 71 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ef6655587b79..d650188586e9 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -364,6 +364,18 @@ struct libxl__ev_child {
     LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
 };
 
+struct libxl__ev_devlock {
+    /* filled by user */
+    libxl__ao *ao;
+    libxl_domid domid;
+    void (*callback)(libxl__egc *, libxl__ev_devlock *, int rc);
+    /* private to libxl__ev_devlock* */
+    libxl__ev_child child;
+    char *path; /* path of the lock file itself */
+    int fd;
+    bool held;
+};
+
 /*
  * QMP asynchronous calls
  *
@@ -428,6 +440,8 @@ _hidden void libxl__ev_qmp_dispose(libxl__gc *gc, 
libxl__ev_qmp *ev);
 typedef enum {
     /* initial state */
     qmp_state_disconnected = 1,
+    /* waiting for lock */
+    qmp_state_waiting_lock,
     /* connected to QMP socket, waiting for greeting message */
     qmp_state_connecting,
     /* qmp_capabilities command sent, waiting for reply */
@@ -461,6 +475,7 @@ struct libxl__ev_qmp {
     libxl__carefd *cfd;
     libxl__ev_fd efd;
     libxl__qmp_state state;
+    libxl__ev_qmplock lock;
     int id;
     int next_id;        /* next id to use */
     /* receive buffer */
@@ -4686,6 +4701,9 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc 
*gc, int domid)
  * which may take a significant amount time.
  * It is to be acquired by an ao event callback.
  *
+ * If libxl__ev_devlock is needed, it should be acquired while every
+ * libxl__ev_qmp are Idle for the current domain.
+ *
  * It is to be acquired when adding/removing devices or making changes
  * to them when this is a slow operation and json_lock isn't appropriate.
  *
@@ -4711,17 +4729,6 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc 
*gc, int domid)
  *  callback:     When called: Active -> LockAcquired (on error: Idle)
  *    The callback is only called once.
  */
-struct libxl__ev_devlock {
-    /* filled by user */
-    libxl__ao *ao;
-    libxl_domid domid;
-    void (*callback)(libxl__egc *, libxl__ev_devlock *, int rc);
-    /* private to libxl__ev_devlock* */
-    libxl__ev_child child;
-    char *path; /* path of the lock file itself */
-    int fd;
-    bool held;
-};
 _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 *);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f0e0b50bd1c5..1ac50a95a42d 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1084,6 +1084,7 @@ static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp 
*ev,
  *
  * qmp_state     External   cfd    efd     id     rx_buf* tx_buf* msg*
  * disconnected   Idle       NULL   Idle    reset  free    free    free
+ * waiting_lock   Active     open   Idle    reset  used    free    set
  * connecting     Active     open   IN      reset  used    free    set
  * cap.neg        Active     open   IN|OUT  sent   used    cap_neg set
  * cap.neg        Active     open   IN      sent   used    free    set
@@ -1153,6 +1154,10 @@ static void qmp_ev_ensure_reading_writing(libxl__gc *gc, 
libxl__ev_qmp *ev)
 {
     short events = POLLIN;
 
+    if (ev->state == qmp_state_waiting_lock)
+        /* We can't modifie the efd yet, as it isn't registered. */
+        return;
+
     if (ev->tx_buf)
         events |= POLLOUT;
     else if ((ev->state == qmp_state_waiting_reply) && ev->msg)
@@ -1168,9 +1173,13 @@ static void qmp_ev_set_state(libxl__gc *gc, 
libxl__ev_qmp *ev,
     switch (new_state) {
     case qmp_state_disconnected:
         break;
-    case qmp_state_connecting:
+    case qmp_state_waiting_lock:
         assert(ev->state == qmp_state_disconnected);
         break;
+    case qmp_state_connecting:
+        assert(ev->state == qmp_state_disconnected ||
+               ev->state == qmp_state_waiting_lock);
+        break;
     case qmp_state_capability_negotiation:
         assert(ev->state == qmp_state_connecting);
         break;
@@ -1231,20 +1240,20 @@ static int 
qmp_error_class_to_libxl_error_code(libxl__gc *gc,
 
 /* Setup connection */
 
-static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
+static void qmp_ev_lock_aquired(libxl__egc *, libxl__ev_qmplock *, int rc);
+
+static int qmp_ev_connect(libxl__egc *egc, libxl__ev_qmp *ev)
     /* disconnected -> connecting but with `msg` free
      * on error: broken */
 {
+    EGC_GC;
     int fd;
-    int rc, r;
-    struct sockaddr_un un;
-    const char *qmp_socket_path;
-
-    assert(ev->state == qmp_state_disconnected);
+    int rc;
 
-    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+    /* Convenience aliases */
+    libxl__ev_qmplock *lock = &ev->lock;
 
-    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+    assert(ev->state == qmp_state_disconnected);
 
     libxl__carefd_begin();
     fd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -1258,6 +1267,35 @@ static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp 
*ev)
     if (rc)
         goto out;
 
+    qmp_ev_set_state(gc, ev, qmp_state_waiting_lock);
+
+    lock->ao = ev->ao;
+    lock->domid = ev->domid;
+    lock->callback = qmp_ev_lock_aquired;
+    libxl__ev_qmplock_lock(egc, &ev->lock);
+
+    return 0;
+
+out:
+    return rc;
+}
+
+static void qmp_ev_lock_aquired(libxl__egc *egc, libxl__ev_qmplock *lock,
+                                int rc)
+{
+    libxl__ev_qmp *ev = CONTAINER_OF(lock, *ev, lock);
+    EGC_GC;
+    const char *qmp_socket_path;
+    struct sockaddr_un un;
+    int r;
+
+    if (rc)
+        goto out;
+
+    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+
+    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+
     rc = libxl__prepare_sockaddr_un(gc, &un, qmp_socket_path,
                                     "QMP socket");
     if (rc)
@@ -1279,10 +1317,10 @@ static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp 
*ev)
 
     qmp_ev_set_state(gc, ev, qmp_state_connecting);
 
-    return 0;
+    return;
 
 out:
-    return rc;
+    LOGD(ERROR, ev->domid, "connect failed");
 }
 
 /* QMP FD callbacks */
@@ -1779,6 +1817,8 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qemu_version.major = -1;
     ev->qemu_version.minor = -1;
     ev->qemu_version.micro = -1;
+
+    libxl__ev_qmplock_init(&ev->lock);
 }
 
 int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
@@ -1798,7 +1838,7 @@ int libxl__ev_qmp_send(libxl__egc *egc, libxl__ev_qmp *ev,
 
     /* Connect to QEMU if not already connected */
     if (ev->state == qmp_state_disconnected) {
-        rc = qmp_ev_connect(gc, ev);
+        rc = qmp_ev_connect(egc, ev);
         if (rc)
             goto error;
     }
@@ -1830,6 +1870,7 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp 
*ev)
 
     libxl__ev_fd_deregister(gc, &ev->efd);
     libxl__carefd_close(ev->cfd);
+    libxl__ev_qmplock_dispose(gc, &ev->lock);
 
     libxl__ev_qmp_init(ev);
 }
-- 
Anthony PERARD

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