All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU
@ 2018-04-16 17:32 Anthony PERARD
  2018-04-16 17:32 ` [RFC v2 1/9] libxl_event: Fix DEBUG prints Anthony PERARD
                   ` (8 more replies)
  0 siblings, 9 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

Patches 1 and 3 fix debug build.
Patch 2 add some documentation to libxl_qmp.c.
Patch 4 fix an issue with qmp_next().

Patches 5 and 6 fix save of a VM with a restricted QEMU.
And the last two patchs try to fix the restore path, but it is still WIP.

Checkout the last patch comments for more information.

Anthony PERARD (9):
  libxl_event: Fix DEBUG prints
  libxl_qmp: Documentation of the logic of the QMP client
  libxl_qmp: Fix use of DEBUG_RECEIVED
  libxl_qmp: Move the buffer realloc to the same scope level as read
  libxl: Learned to send FD through QMP to QEMU
  libxl: Have QEMU save its state to a file descriptor
  libxl_qmp: Implement query-status command
  HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  libxl_qmp: Add a warning to not trust QEMU

 tools/libxl/libxl_aoutils.c  | 156 ++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dm.c       |  44 +++++++--
 tools/libxl/libxl_event.c    |   8 +-
 tools/libxl/libxl_exec.c     |  28 ++++--
 tools/libxl/libxl_internal.h |  33 +++++++
 tools/libxl/libxl_qmp.c      | 167 ++++++++++++++++++++++++++++++-----
 6 files changed, 403 insertions(+), 33 deletions(-)

-- 
Anthony PERARD


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [RFC v2 1/9] libxl_event: Fix DEBUG prints
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-19  8:17   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client Anthony PERARD
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_event.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 484f9bab4d..0370b6acdd 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -248,6 +248,7 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
 short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events) {
     struct pollfd check;
     int r;
+    EGC_GC;
 
     for (;;) {
         check.fd = fd;
@@ -336,7 +337,7 @@ static void time_done_debug(libxl__gc *gc, const char *func,
                             libxl__ev_time *ev, int rc)
 {
 #ifdef DEBUG
-    libxl__log(CTX, XTL_DEBUG, -1,__FILE__,0,func,
+    libxl__log(CTX, XTL_DEBUG, -1, __FILE__, 0, func, INVALID_DOMID,
                "ev_time=%p done rc=%d .func=%p infinite=%d abs=%lu.%06lu",
                ev, rc, ev->func, ev->infinite,
                (unsigned long)ev->abs.tv_sec, (unsigned long)ev->abs.tv_usec);
@@ -445,6 +446,8 @@ void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
 
 static void time_occurs(libxl__egc *egc, libxl__ev_time *etime, int rc)
 {
+    EGC_GC;
+
     DBG("ev_time=%p occurs abs=%lu.%06lu",
         etime, (unsigned long)etime->abs.tv_sec,
         (unsigned long)etime->abs.tv_usec);
@@ -1192,6 +1195,7 @@ static int afterpoll_check_fd(libxl__poller *poller,
 static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents_ign)
 {
     short revents_current = libxl__fd_poll_recheck(egc, efd->fd, efd->events);
+    EGC_GC;
 
     DBG("ev_fd=%p occurs fd=%d events=%x revents_ign=%x revents_current=%x",
         efd, efd->fd, efd->events, revents_ign, revents_current);
@@ -2117,6 +2121,8 @@ int libxl_ao_abort(libxl_ctx *ctx, const libxl_asyncop_how *how)
 int libxl__ao_aborting(libxl__ao *ao)
 {
     libxl__ao *root = ao_nested_root(ao);
+    AO_GC;
+
     if (root->aborting) {
         DBG("ao=%p: aborting at explicit check (root=%p)", ao, root);
         return ERROR_ABORTED;
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
  2018-04-16 17:32 ` [RFC v2 1/9] libxl_event: Fix DEBUG prints Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-19  8:19   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED Anthony PERARD
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index d03cb51668..be23ffea6a 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -18,6 +18,42 @@
  * Specification, see in the QEMU repository.
  */
 
+/*
+ * Logic used to send command to QEMU
+ *
+ * qmp_open():
+ *  Will open a socket and connect to QEMU.
+ *
+ * qmp_next():
+ *  Will read data sent by QEMU and then call qmp_handle_response() once a
+ *  complete QMP message is received.
+ *  The function return on timeout/error or once every data received as been
+ *  processed.
+ *
+ * qmp_handle_response()
+ *  This process json messages received from QEMU and update different list and
+ *  may call callback function.
+ *  `libxl__qmp_handler.wait_for_id` is reset once a message with this ID is
+ *    processed.
+ *  `libxl__qmp_handler.callback_list`: list with ID of command sent and
+ *    optional assotiated callback function. The return value of a callback is
+ *    set in context.
+ *
+ * qmp_send():
+ *  Simply prepare a QMP command and send it to QEMU.
+ *  It also add a `struct callback_id_pair` on the
+ *  `libxl__qmp_handler.callback_list` via qmp_send_prepare().
+ *
+ * qmp_synchronous_send():
+ *  This function calls qmp_send(), then wait for QEMU to reply to the command.
+ *  The wait is done by calling qmp_next() over and over again until either
+ *  there is a responce for the command or there is an error.
+ *
+ *  An ID can be set for each QMP command, this is set into
+ *  `libxl__qmp_handler.wait_for_id`. qmp_next will check every response's ID
+ *  again this field and change the value of the field once the ID is found.
+ */
+
 #include "libxl_osdeps.h" /* must come before any other headers */
 
 #include <sys/un.h>
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
  2018-04-16 17:32 ` [RFC v2 1/9] libxl_event: Fix DEBUG prints Anthony PERARD
  2018-04-16 17:32 ` [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-19  8:22   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read Anthony PERARD
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

This patch fix complilation error with #define DEBUG_RECEIVED of the
macro DEBUG_REPORT_RECEIVED.

  error: field precision specifier ‘.*’ expects argument of type ‘int’, but argument 9 has type ‘ssize_t {aka long int}’

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index be23ffea6a..4d207c3842 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -522,7 +522,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
         }
         qmp->buffer[rd] = '\0';
 
-        DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, rd);
+        DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
 
         do {
             char *end = NULL;
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (2 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-23  9:03   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU Anthony PERARD
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

In qmp_next(), the inner loop should only try to parse messages from
QMP, if there is more than one.

The handling of the receive buffer ('incomplete'), should be done at the
same scope level as read(). It doesn't need to be handle more that once
after a read.

Before this patch, when on message what handled, the inner loop would
restart by adding the 'buffer' into 'incomplete' (after reallocation).
Since 'rd' was not reset, the buffer would be strcat a second time.
After that, the stream from the QMP server would have syntax error, and
the parsor would throw errors.

This is unlikely to happen as the receive buffer is very large. And
receiving two messages in a row is unlikely. In the current case, this
could be an event and a response to a command.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 4d207c3842..a25f445fb6 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
 
         DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
 
+        if (incomplete) {
+            size_t current_pos = s - incomplete;
+            incomplete = libxl__realloc(gc, incomplete,
+                                        incomplete_size + rd + 1);
+            strncat(incomplete + incomplete_size, qmp->buffer, rd);
+            s = incomplete + current_pos;
+            incomplete_size += rd;
+            s_end = incomplete + incomplete_size;
+        } else {
+            incomplete = libxl__strndup(gc, qmp->buffer, rd);
+            incomplete_size = rd;
+            s = incomplete;
+            s_end = s + rd;
+            rd = 0;
+        }
+
         do {
             char *end = NULL;
-            if (incomplete) {
-                size_t current_pos = s - incomplete;
-                incomplete = libxl__realloc(gc, incomplete,
-                                            incomplete_size + rd + 1);
-                strncat(incomplete + incomplete_size, qmp->buffer, rd);
-                s = incomplete + current_pos;
-                incomplete_size += rd;
-                s_end = incomplete + incomplete_size;
-            } else {
-                incomplete = libxl__strndup(gc, qmp->buffer, rd);
-                incomplete_size = rd;
-                s = incomplete;
-                s_end = s + rd;
-                rd = 0;
-            }
 
             end = strstr(s, "\r\n");
             if (end) {
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (3 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-23  9:04   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor Anthony PERARD
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

Adding the ability to send a file descriptor from libxl to QEMU via the
QMP interface. This will be use with the "add-fd" QMP command.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_qmp.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index a25f445fb6..28bc2cabcd 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -116,6 +116,9 @@ struct libxl__qmp_handler {
         int minor;
         int micro;
     } version;
+
+    /* File descriptor to send to QEMU on the next command */
+    int fd_to_send;
 };
 
 static int qmp_send(libxl__qmp_handler *qmp,
@@ -414,6 +417,8 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid)
 
     LIBXL_STAILQ_INIT(&qmp->callback_list);
 
+    qmp->fd_to_send = -1;
+
     return qmp;
 }
 
@@ -639,9 +644,16 @@ static int qmp_send(libxl__qmp_handler *qmp,
         goto out;
     }
 
-    if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, buf, strlen(buf),
-                            "QMP command", "QMP socket"))
-        goto out;
+    if (qmp->fd_to_send >= 0) {
+        if (libxl__sendmsg_fds(gc, qmp->qmp_fd, buf, strlen(buf),
+                               1, &qmp->fd_to_send, "QMP socket"))
+            goto out;
+        qmp->fd_to_send = -1;
+    } else {
+        if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, buf, strlen(buf),
+                                "QMP command", "QMP socket"))
+            goto out;
+    }
     if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
                             "CRLF", "QMP socket"))
         goto out;
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (4 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-23  9:20   ` Wei Liu
  2018-04-24  9:46   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 7/9] libxl_qmp: Implement query-status command Anthony PERARD
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

In case QEMU have restricted access to the system, open the file for it,
and QEMU will save its state to this file descritor.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 28bc2cabcd..3bb0f28bea 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -989,25 +989,61 @@ int libxl__qmp_system_wakeup(libxl__gc *gc, int domid)
     return qmp_run_command(gc, domid, "system_wakeup", NULL, NULL, NULL);
 }
 
+/* Find out which fdset have been allocated */
+static int qmp_fdset_add_fd_callback(libxl__qmp_handler *qmp,
+                                     const libxl__json_object *ret,
+                                     void *opaque)
+{
+    const libxl__json_object *o;
+    int fdset;
+
+    o = libxl__json_map_get("fdset-id", ret, JSON_INTEGER);
+    if (!o)
+        return 1;
+
+    fdset = libxl__json_object_get_integer(o);
+    *(int*)opaque = fdset;
+    return 0;
+}
+
 int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
 {
     libxl__json_object *args = NULL;
     libxl__qmp_handler *qmp = NULL;
     int rc;
+    int state_fd;
+    int new_fdset;
 
     qmp = libxl__qmp_initialize(gc, domid);
     if (!qmp)
         return ERROR_FAIL;
 
-    qmp_parameters_add_string(gc, &args, "filename", (char *)filename);
+    state_fd = open(filename, O_WRONLY | O_CREAT, 0600);
+    if (state_fd < 0) {
+        LOGED(ERROR, domid,
+              "Failed to open file %s for QEMU", filename);
+        goto out;
+    }
+
+    qmp->fd_to_send = state_fd;
+
+    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
+                              qmp_fdset_add_fd_callback, &new_fdset,
+                              qmp->timeout);
+    if (rc)
+        goto out;
 
     /* live parameter was added to QEMU 2.11. It signal QEMU that the save
      * operation is for a live migration rather that for taking a snapshot. */
     if (qmp_qemu_check_version(qmp, 2, 11, 0))
         qmp_parameters_add_bool(gc, &args, "live", live);
 
+    QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", new_fdset);
     rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
                               NULL, NULL, qmp->timeout);
+out:
+    if (state_fd >= 0)
+        close(state_fd);
     libxl__qmp_close(qmp);
     return rc;
 }
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 7/9] libxl_qmp: Implement query-status command
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (5 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-23  9:24   ` Wei Liu
  2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
  2018-04-16 17:32 ` [RFC v2 9/9] libxl_qmp: Add a warning to not trust QEMU Anthony PERARD
  8 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

It check via QMP if QEMU as reach the intended status.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6352380644..3764d26463 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1857,6 +1857,9 @@ _hidden int libxl__qmp_nbd_server_stop(libxl__gc *gc, int domid);
 _hidden int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid,
                                          const char *parant,
                                          const char *child, const char *node);
+_hidden int libxl__qmp_query_status(libxl__gc *gc,
+                                    int domid,
+                                    const char *intended_status);
 /* run a hmp command in qmp mode */
 _hidden int libxl__qmp_hmp(libxl__gc *gc, int domid, const char *command_line,
                            char **out);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 3bb0f28bea..83bd804219 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1246,6 +1246,45 @@ int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid, const char *parent,
     return qmp_run_command(gc, domid, "x-blockdev-change", args, NULL, NULL);
 }
 
+static int qmp_check_status(libxl__qmp_handler *qmp,
+                            const libxl__json_object *response,
+                            void *opaque)
+{
+    char **status = opaque;
+    GC_INIT(qmp->ctx);
+    const libxl__json_object *o;
+
+    o = libxl__json_map_get("status", response, JSON_STRING);
+    if (!o)
+        return 1;
+    *status = libxl__strdup(gc, libxl__json_object_get_string(o));
+    return 0;
+}
+
+int libxl__qmp_query_status(libxl__gc *gc,
+                            int domid,
+                            const char *intended_status)
+{
+    char *status = NULL;
+    int rc;
+
+    rc = qmp_run_command(gc, domid, "query-status", NULL,
+                         qmp_check_status, &status);
+    if (rc < 0)
+        return rc;
+    if (rc == 1)
+        /* QMP command returned unexpected result */
+        return ERROR_FAIL;
+
+    LOGD(DEBUG, domid, "query-status result: %s", status);
+    if (!strcmp(intended_status, status))
+        /* success and ready */
+        return 0;
+
+    /* command success, status != intended_status */
+    return ERROR_NOT_READY;
+}
+
 static int hmp_callback(libxl__qmp_handler *qmp,
                         const libxl__json_object *response,
                         void *opaque)
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (6 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 7/9] libxl_qmp: Implement query-status command Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  2018-04-16 18:09   ` Anthony PERARD
                     ` (2 more replies)
  2018-04-16 17:32 ` [RFC v2 9/9] libxl_qmp: Add a warning to not trust QEMU Anthony PERARD
  8 siblings, 3 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

When QEMU is restricted, the qemu on the receiving side cann't write
anything to xenstore once the migration is started. So it cann't tell
libxl that it is ready to continue running the guest.

In order to find out if QEMU is ready, we can issue QMP commands and
check if it respond.

But there is no QMP socket to connect to before qemu is started. But we
can uses different facility from qemu in order to setup some kind of
callback before starting QEMU. For that, we open a file descriptor and
give it to qemu.

This patch creates a pipe, give the write-end to qemu, and wait for
something to be written to it. (We could check if it is actually the QMP
greeting message.)

QEMU is asked to setup a QMP server on this pipe, but even if it is a
one-way only, qemu will write the QMP greeting message to the pipe.
This is done with:
-add-fd, to create a fdset which is use later.
-chardev 'file,path=/dev/fdset/1,append=true', this open a char device
on the write-end of the pipe, tell qemu that the FD is write-only, and
not to run truncate on it.
-mon, just start the QMP server on this new chardev.

With that, qemu will start the QMP server on the write-only fd, which is
enough to have the QMP greeting message. At this point, the QMP socket
is ready, and I think qemu is in the main-loop and ready to start the
emulation and respond to QMP commands.

This patch calls 'query-status', any response to that without error
means that QEMU is ready. If the status is "running", QEMU would already
have written the xenstore node if it could and is doing emulation.
(Any subsequent QMP command 'cont', libxl__qmp_resume(),  is not
changing anything.  If one adds '-S' to the QEMU command line, QEMU will
have the status "prelaunch" as a response to 'query-status', then QEMU
can be asked to start emulation with 'cont' via QMP.)

This patch copies most of "xswait" and call it "qmpwait". This is
probably not the best way forward due to duplication.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_aoutils.c  | 156 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dm.c       |  44 ++++++++--
 tools/libxl/libxl_exec.c     |  28 +++++--
 tools/libxl/libxl_internal.h |  30 +++++++
 4 files changed, 246 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 9e493cd487..7d654996c3 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -626,6 +626,162 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
                 what, (unsigned long)pid, sig);
 }
 
+/*----- qmpwait -----*/
+
+static libxl__ev_fd_callback qmpwait_fd_read_callback;
+static libxl__ev_time_callback qmpwait_timeout_callback;
+static void qmpwait_report_error(libxl__egc*, libxl__qmpwait_state*, int rc);
+
+void libxl__qmpwait_init(libxl__qmpwait_state *qmpwa)
+{
+    libxl__ev_time_init(&qmpwa->time_ev);
+    libxl__ev_fd_init(&qmpwa->notify_efd);
+}
+
+void libxl__qmpwait_stop(libxl__gc *gc, libxl__qmpwait_state *qmpwa)
+{
+    libxl__ev_time_deregister(gc, &qmpwa->time_ev);
+    libxl__ev_fd_deregister(gc, &qmpwa->notify_efd);
+    libxl__carefd_close(qmpwa->notify_cfd);
+    qmpwa->notify_cfd = NULL;
+}
+
+bool libxl__qmpwait_inuse(const libxl__qmpwait_state *qmpwa)
+{
+    bool time_inuse = libxl__ev_time_isregistered(&qmpwa->time_ev);
+    bool watch_inuse = libxl__ev_fd_isregistered(&qmpwa->notify_efd);
+    assert(time_inuse == watch_inuse);
+    return time_inuse;
+}
+
+int libxl__qmpwait_start(libxl__gc *gc, libxl__qmpwait_state *qmpwa)
+{
+    int rc;
+
+    rc = libxl__ev_time_register_rel(qmpwa->ao, &qmpwa->time_ev,
+                                     qmpwait_timeout_callback, qmpwa->timeout_ms);
+    if (rc) goto err;
+
+    rc = libxl__ev_fd_register(gc, &qmpwa->notify_efd,
+                               qmpwait_fd_read_callback,
+                               libxl__carefd_fd(qmpwa->notify_cfd),
+                               POLLIN);
+    if (rc) goto err;
+
+    return 0;
+
+ err:
+    libxl__qmpwait_stop(gc, qmpwa);
+    return rc;
+}
+
+static void qmpwait_fd_read_callback(libxl__egc *egc, libxl__ev_fd *ev,
+                                     int fd, short events, short revents)
+{
+    // checkout:
+    // - datacopier_readable
+    // - watchfd_callback
+
+    libxl__qmpwait_state *qmpwa = CONTAINER_OF(ev, *qmpwa, notify_efd);
+    libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, qmpwait.notify_efd);
+    STATE_AO_GC(qmpwa->ao);
+
+    // need to handle POLLERR
+    // POLLHUP and POLLIN might be both set.
+    if (revents & POLLHUP && !(revents & POLLIN)) {
+        LOG(DEBUG, "received POLLHUP on fd %d: read for %s",
+            fd, qmpwa->what);
+        libxl__ev_fd_deregister(gc, &qmpwa->notify_efd);
+        if (!qmpwa->life_time_read)
+            ss->failure_cb(egc, ss, ERROR_FAIL);
+        return;
+    }
+    if (revents & ~(POLLIN|POLLHUP)) {
+        LOG(ERROR, "unexpected poll event 0x%x on fd %d (expected POLLIN "
+            "and/or POLLHUP) reading %s",
+            revents, fd, qmpwa->what);
+        libxl__ev_fd_deregister(gc, &qmpwa->notify_efd);
+        return;
+    }
+    for (;;) {
+        int r;
+        char buffer[128];
+
+
+        r = read(fd, buffer, sizeof(buffer) - 1);
+        if (r < 0) {
+            if (errno == EINTR) continue;
+            assert(errno);
+            if (errno == EWOULDBLOCK) {
+                break;
+            }
+            LOGE(ERROR, "error reading %s", qmpwa->what);
+            /*datacopier_callback(egc, dc, ERROR_FAIL, 0, errno);*/
+            return;
+        }
+        if (r == 0) {
+            LOG(ERROR, "eof maybe on fd %d for %s",
+                fd, qmpwa->what);
+            return;
+        }
+        LOG(DEBUG, "have read %d from fd %d for %s",
+            r, fd, qmpwa->what);
+        buffer[r] = '\0';
+        LOG(DEBUG, "read: '%s'", buffer);
+        if (!qmpwa->life_time_read) {
+            qmpwa->life_time_read++;
+
+            int rc;
+            libxl__dm_spawn_state *dmss = CONTAINER_OF(ss, *dmss, spawn);
+
+            LOGD(DEBUG, dmss->guest_domid, "will query status via qmp");
+            rc = libxl__qmp_query_status(gc, dmss->guest_domid, "running");
+            if (rc) {
+                LOGD(DEBUG, dmss->guest_domid, "query status failed with: %d", rc);
+
+                if (rc == ERROR_NOT_READY){
+                    LOGD(DEBUG, dmss->guest_domid, "retry for prelaunch state");
+                    rc = libxl__qmp_query_status(gc, dmss->guest_domid, "prelaunch");
+                }
+            }
+            if (!rc)
+                libxl__spawn_initiate_detach(gc, ss);
+        }
+        // end of a QMP result
+        if (strstr(buffer, "\r\n")) {
+            libxl__dm_spawn_state *dmss = CONTAINER_OF(ss, *dmss, spawn);
+            LOGD(DEBUG, dmss->guest_domid, "have a full QMP line, close fd");
+            libxl__ev_fd_deregister(gc, &qmpwa->notify_efd);
+            libxl__carefd_close(qmpwa->notify_cfd);
+            qmpwa->notify_cfd = NULL;
+        }
+        break;
+    }
+}
+
+void qmpwait_timeout_callback(libxl__egc *egc, libxl__ev_time *ev,
+                             const struct timeval *requested_abs,
+                             int rc)
+{
+    EGC_GC;
+    libxl__qmpwait_state *qmpwa = CONTAINER_OF(ev, *qmpwa, time_ev);
+    LOG(DEBUG, "%s: qmpwait timeout", qmpwa->what);
+    qmpwait_report_error(egc, qmpwa, rc);
+}
+
+static void qmpwait_report_error(libxl__egc *egc, libxl__qmpwait_state *qmpwa,
+                                int rc)
+{
+    EGC_GC;
+    libxl__qmpwait_stop(gc, qmpwa);
+    /*qmpwa->callback(egc, qmpwa, rc, 0);*/
+    {
+        // reuse the xswait callback for the error handling.
+        libxl__spawn_state *ss = CONTAINER_OF(qmpwa, *ss, qmpwait);
+        qmpwa->callback(egc, &ss->xswait, rc, 0);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index b662395b2e..772350abd4 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -926,7 +926,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                                         const libxl_domain_config *guest_config,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
-                                        int *dm_state_fd)
+                                        int *dm_state_fd, int dm_notify_fd)
 {
     const libxl_domain_create_info *c_info = &guest_config->c_info;
     const libxl_domain_build_info *b_info = &guest_config->b_info;
@@ -973,6 +973,20 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, "-mon");
     flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
 
+    /* Monitor used at boot time */
+    if (dm_notify_fd >= 0) {
+        flexarray_vappend(dm_args,
+                          "-add-fd", GCSPRINTF("fd=%d,set=1", dm_notify_fd),
+                          // can't be a pipe, because pipe would needs to be r/w
+                          // but it's write only
+                          // append is because otherwise qemu will attempt to
+                          // truncate the fd
+                          "-chardev",
+                          "file,id=notify,path=/dev/fdset/1,append=on",
+                          "-mon", "chardev=notify,mode=control",
+                          NULL);
+    }
+
     for (i = 0; i < guest_config->num_channels; i++) {
         connection = guest_config->channels[i].connection;
         devid = guest_config->channels[i].devid;
@@ -1735,7 +1749,8 @@ static int libxl__build_device_model_args(libxl__gc *gc,
                                         const libxl_domain_config *guest_config,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
-                                        int *dm_state_fd)
+                                        int *dm_state_fd,
+                                        int dm_notify_fd)
 /* dm_state_fd may be NULL iff caller knows we are using old stubdom
  * and therefore will be passing a filename rather than a fd. */
 {
@@ -1751,7 +1766,7 @@ static int libxl__build_device_model_args(libxl__gc *gc,
         return libxl__build_device_model_args_new(gc, dm,
                                                   guest_domid, guest_config,
                                                   args, envs,
-                                                  state, dm_state_fd);
+                                                  state, dm_state_fd, dm_notify_fd);
     default:
         LOGED(ERROR, guest_domid, "unknown device model version %d",
               guest_config->b_info.device_model_version);
@@ -1971,7 +1986,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
                                          guest_config, &args, NULL,
-                                         d_state, NULL);
+                                         d_state, NULL, -1);
     if (ret) {
         ret = ERROR_FAIL;
         goto out;
@@ -2257,6 +2272,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     char **pass_stuff;
     const char *dm;
     int dm_state_fd = -1;
+    int dm_notify_fd[2] = { -1, -1 };
+    libxl__carefd *cf_read_end;
 
     if (libxl_defbool_val(b_info->device_model_stubdomain)) {
         abort();
@@ -2272,9 +2289,20 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         rc = ERROR_FAIL;
         goto out;
     }
+    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        libxl__carefd_begin();
+        rc = libxl_pipe(ctx, dm_notify_fd);
+        cf_read_end = libxl__carefd_opened(ctx, dm_notify_fd[0]);
+        if (rc)
+            goto out;
+        rc = libxl_fd_set_nonblock(CTX, libxl__carefd_fd(cf_read_end), 1);
+        if (rc)
+            goto out;
+    }
     rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
                                           &args, &envs, state,
-                                          &dm_state_fd);
+                                          &dm_state_fd,
+                                          dm_notify_fd[1]);
     if (rc)
         goto out;
 
@@ -2357,6 +2385,9 @@ retry_transaction:
     spawn->failure_cb = device_model_startup_failed;
     spawn->detached_cb = device_model_detached;
 
+    LOGD(DEBUG, domid, "Using QMP instead of xenstore for QEMU startup notification");
+    spawn->qmpwait.notify_cfd = cf_read_end;
+
     rc = libxl__spawn_spawn(egc, spawn);
     if (rc < 0)
         goto out_close;
@@ -2372,6 +2403,9 @@ out_close:
     if (logfile_w >= 0) close(logfile_w);
 out:
     if (dm_state_fd >= 0) close(dm_state_fd);
+    if (rc && cf_read_end)
+        libxl__carefd_close(cf_read_end);
+    if (dm_notify_fd[1] >= 0) close(dm_notify_fd[1]);
     if (rc)
         device_model_spawn_outcome(egc, dmss, rc);
 }
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 02e6c917f0..165b6e8410 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -272,6 +272,7 @@ void libxl__spawn_init(libxl__spawn_state *ss)
 {
     libxl__ev_child_init(&ss->mid);
     libxl__xswait_init(&ss->xswait);
+    libxl__qmpwait_init(&ss->qmpwait);
 }
 
 int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
@@ -284,13 +285,24 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     libxl__spawn_init(ss);
     ss->rc = ss->detaching = 0;
 
-    ss->xswait.ao = ao;
-    ss->xswait.what = GCSPRINTF("%s startup", ss->what);
-    ss->xswait.path = ss->xspath;
-    ss->xswait.timeout_ms = ss->timeout_ms;
-    ss->xswait.callback = spawn_watch_event;
-    rc = libxl__xswait_start(gc, &ss->xswait);
-    if (rc) goto out_err;
+    if (ss->qmpwait.notify_cfd) {
+        ss->qmpwait.ao = ao;
+        ss->qmpwait.what = GCSPRINTF("%s startup (QMP)", ss->what);
+        ss->qmpwait.timeout_ms = ss->timeout_ms;
+        ss->qmpwait.callback = spawn_watch_event;
+
+        rc = libxl__qmpwait_start(gc, &ss->qmpwait);
+        if (rc) goto out_err;
+
+    } else {
+        ss->xswait.ao = ao;
+        ss->xswait.what = GCSPRINTF("%s startup", ss->what);
+        ss->xswait.path = ss->xspath;
+        ss->xswait.timeout_ms = ss->timeout_ms;
+        ss->xswait.callback = spawn_watch_event;
+        rc = libxl__xswait_start(gc, &ss->xswait);
+        if (rc) goto out_err;
+    }
 
     pid_t middle = libxl__ev_child_fork(gc, &ss->mid, spawn_middle_death);
     if (middle ==-1) { rc = ERROR_FAIL; goto out_err; }
@@ -347,6 +359,7 @@ static void spawn_cleanup(libxl__gc *gc, libxl__spawn_state *ss)
 {
     assert(!libxl__ev_child_inuse(&ss->mid));
     libxl__xswait_stop(gc, &ss->xswait);
+    libxl__qmpwait_stop(gc, &ss->qmpwait);
 }
 
 static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
@@ -359,6 +372,7 @@ static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
     assert(libxl__ev_child_inuse(&ss->mid));
     assert(ss->detaching || ss->rc);
     libxl__xswait_stop(gc, &ss->xswait);
+    libxl__qmpwait_stop(gc, &ss->qmpwait);
 
     pid_t child = ss->mid.pid;
     r = kill(child, SIGKILL);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3764d26463..aa78b8e369 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -201,6 +201,7 @@ typedef struct libxl__ao libxl__ao;
 typedef struct libxl__aop_occurred libxl__aop_occurred;
 typedef struct libxl__osevent_hook_nexus libxl__osevent_hook_nexus;
 typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
+typedef struct libxl__carefd libxl__carefd;
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
 typedef void libxl__domain_create_cb(struct libxl__egc *egc,
@@ -1328,6 +1329,34 @@ bool libxl__xswait_inuse(const libxl__xswait_state *ss);
 
 int libxl__xswait_start(libxl__gc*, libxl__xswait_state*);
 
+/*-------- qmpwait: wait for QEMU to start QMP on a pipe -------*/
+
+typedef struct libxl__qmpwait_state  libxl__qmpwait_state;
+
+struct libxl__qmpwait_state {
+    /* caller must fill these in, and they must all remain valid */
+    libxl__ao *ao;
+    const char *what; /* for error msgs: noun phrase, what we're waiting for */
+    int timeout_ms; /* as for poll(2) */
+    /* read-end of a pipe where QEMU is going to write once QMP is available */
+    libxl__carefd *notify_cfd;
+    /* remaining fields are private to qmpwait */
+    libxl__ev_time time_ev;
+    libxl__ev_fd notify_efd;
+    // until better implementation
+    libxl__xswait_callback *callback;
+
+    /* Used to find out if something has been written to the pipe */
+    int life_time_read;
+};
+
+void libxl__qmpwait_init(libxl__qmpwait_state*);
+void libxl__qmpwait_stop(libxl__gc*, libxl__qmpwait_state*); /*idempotent*/
+bool libxl__qmpwait_inuse(const libxl__qmpwait_state *ss);
+
+int libxl__qmpwait_start(libxl__gc*, libxl__qmpwait_state*);
+
+
 /*
  * libxl__ev_devstate - waits a given time for a device to
  * reach a given state.  Follows the libxl_ev_* conventions.
@@ -1568,6 +1597,7 @@ struct libxl__spawn_state {
     int rc; /* might be non-0 whenever we are not Idle */
     libxl__ev_child mid; /* always in use whenever we are not Idle */
     libxl__xswait_state xswait;
+    libxl__qmpwait_state qmpwait;
 };
 
 static inline int libxl__spawn_inuse(const libxl__spawn_state *ss)
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RFC v2 9/9] libxl_qmp: Add a warning to not trust QEMU
  2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
                   ` (7 preceding siblings ...)
  2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
@ 2018-04-16 17:32 ` Anthony PERARD
  8 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 17:32 UTC (permalink / raw)
  To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu

... even if it is not the case for the current code.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 83bd804219..3b167f24dd 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -16,6 +16,9 @@
 /*
  * This file implement a client for QMP (QEMU Monitor Protocol). For the
  * Specification, see in the QEMU repository.
+ *
+ * WARNING - Do not trust QEMU when writing codes for new commands or when
+ *           improving the client code.
  */
 
 /*
-- 
Anthony PERARD


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

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
@ 2018-04-16 18:09   ` Anthony PERARD
  2018-04-17  9:18   ` Anthony PERARD
  2018-04-20 18:37   ` Ian Jackson
  2 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-16 18:09 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Wei Liu

On Mon, Apr 16, 2018 at 06:32:26PM +0100, Anthony PERARD wrote:
> -add-fd, to create a fdset which is use later.
> -chardev 'file,path=/dev/fdset/1,append=true', this open a char device
> on the write-end of the pipe, tell qemu that the FD is write-only, and
> not to run truncate on it.
> -mon, just start the QMP server on this new chardev.

There might be a better way than this. But the new way only exist in
2.12, and is not documented... (in --help).

With QEMU commit 0935700f8544033ebbd41e1f13cd528f8a58d24d (char: allow
passing pre-opened socket file descriptor at startup).

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
  2018-04-16 18:09   ` Anthony PERARD
@ 2018-04-17  9:18   ` Anthony PERARD
  2018-04-20 18:37   ` Ian Jackson
  2 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-17  9:18 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Jackson, Wei Liu

On Mon, Apr 16, 2018 at 06:32:26PM +0100, Anthony PERARD wrote:
> When QEMU is restricted, the qemu on the receiving side cann't write
> anything to xenstore once the migration is started. So it cann't tell
> libxl that it is ready to continue running the guest.
> 
> In order to find out if QEMU is ready, we can issue QMP commands and
> check if it respond.
> 
> But there is no QMP socket to connect to before qemu is started. But we
> can uses different facility from qemu in order to setup some kind of
> callback before starting QEMU. For that, we open a file descriptor and
> give it to qemu.
> 
> This patch creates a pipe, give the write-end to qemu, and wait for
> something to be written to it. (We could check if it is actually the QMP
> greeting message.)
> 
> QEMU is asked to setup a QMP server on this pipe, but even if it is a
> one-way only, qemu will write the QMP greeting message to the pipe.
> This is done with:
> -add-fd, to create a fdset which is use later.
> -chardev 'file,path=/dev/fdset/1,append=true', this open a char device
> on the write-end of the pipe, tell qemu that the FD is write-only, and
> not to run truncate on it.
> -mon, just start the QMP server on this new chardev.
> 
> With that, qemu will start the QMP server on the write-only fd, which is
> enough to have the QMP greeting message. At this point, the QMP socket
> is ready, and I think qemu is in the main-loop and ready to start the
> emulation and respond to QMP commands.
> 
> This patch calls 'query-status', any response to that without error
> means that QEMU is ready. If the status is "running", QEMU would already
> have written the xenstore node if it could and is doing emulation.
> (Any subsequent QMP command 'cont', libxl__qmp_resume(),  is not
> changing anything.  If one adds '-S' to the QEMU command line, QEMU will
> have the status "prelaunch" as a response to 'query-status', then QEMU
> can be asked to start emulation with 'cont' via QMP.)
> 
> This patch copies most of "xswait" and call it "qmpwait". This is
> probably not the best way forward due to duplication.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---

This email should have:

Changes in RFC v2:
- Have QEMU throw a event instead of polling on connect() to the QMP
  socket

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 1/9] libxl_event: Fix DEBUG prints
  2018-04-16 17:32 ` [RFC v2 1/9] libxl_event: Fix DEBUG prints Anthony PERARD
@ 2018-04-19  8:17   ` Wei Liu
  2018-04-19 11:01     ` Anthony PERARD
  0 siblings, 1 reply; 28+ messages in thread
From: Wei Liu @ 2018-04-19  8:17 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:19PM +0100, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

I guess DEBUG was broken because in three cases a gc was not in scope
and in one case an argument was missing?

With the commit message updated:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/libxl_event.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 484f9bab4d..0370b6acdd 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -248,6 +248,7 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
>  short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events) {
>      struct pollfd check;
>      int r;
> +    EGC_GC;
>  
>      for (;;) {
>          check.fd = fd;
> @@ -336,7 +337,7 @@ static void time_done_debug(libxl__gc *gc, const char *func,
>                              libxl__ev_time *ev, int rc)
>  {
>  #ifdef DEBUG
> -    libxl__log(CTX, XTL_DEBUG, -1,__FILE__,0,func,
> +    libxl__log(CTX, XTL_DEBUG, -1, __FILE__, 0, func, INVALID_DOMID,
>                 "ev_time=%p done rc=%d .func=%p infinite=%d abs=%lu.%06lu",
>                 ev, rc, ev->func, ev->infinite,
>                 (unsigned long)ev->abs.tv_sec, (unsigned long)ev->abs.tv_usec);
> @@ -445,6 +446,8 @@ void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
>  
>  static void time_occurs(libxl__egc *egc, libxl__ev_time *etime, int rc)
>  {
> +    EGC_GC;
> +
>      DBG("ev_time=%p occurs abs=%lu.%06lu",
>          etime, (unsigned long)etime->abs.tv_sec,
>          (unsigned long)etime->abs.tv_usec);
> @@ -1192,6 +1195,7 @@ static int afterpoll_check_fd(libxl__poller *poller,
>  static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents_ign)
>  {
>      short revents_current = libxl__fd_poll_recheck(egc, efd->fd, efd->events);
> +    EGC_GC;
>  
>      DBG("ev_fd=%p occurs fd=%d events=%x revents_ign=%x revents_current=%x",
>          efd, efd->fd, efd->events, revents_ign, revents_current);
> @@ -2117,6 +2121,8 @@ int libxl_ao_abort(libxl_ctx *ctx, const libxl_asyncop_how *how)
>  int libxl__ao_aborting(libxl__ao *ao)
>  {
>      libxl__ao *root = ao_nested_root(ao);
> +    AO_GC;
> +
>      if (root->aborting) {
>          DBG("ao=%p: aborting at explicit check (root=%p)", ao, root);
>          return ERROR_ABORTED;
> -- 
> Anthony PERARD
> 

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client
  2018-04-16 17:32 ` [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client Anthony PERARD
@ 2018-04-19  8:19   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-19  8:19 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:20PM +0100, Anthony PERARD wrote:
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED
  2018-04-16 17:32 ` [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED Anthony PERARD
@ 2018-04-19  8:22   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-19  8:22 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:21PM +0100, Anthony PERARD wrote:
> This patch fix complilation error with #define DEBUG_RECEIVED of the
> macro DEBUG_REPORT_RECEIVED.
> 
>   error: field precision specifier ‘.*’ expects argument of type ‘int’, but argument 9 has type ‘ssize_t {aka long int}’
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

There is no risk of truncation here because prior checks have made sure
rd can't be larger than 4K+1.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 1/9] libxl_event: Fix DEBUG prints
  2018-04-19  8:17   ` Wei Liu
@ 2018-04-19 11:01     ` Anthony PERARD
  0 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2018-04-19 11:01 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Ian Jackson

On Thu, Apr 19, 2018 at 09:17:04AM +0100, Wei Liu wrote:
> On Mon, Apr 16, 2018 at 06:32:19PM +0100, Anthony PERARD wrote:
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> I guess DEBUG was broken because in three cases a gc was not in scope
> and in one case an argument was missing?

Yes

> With the commit message updated:
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

I think the commit message can be:

The libxl__log() call was missing the domid.

The macro DBG is using LIBXL__LOG which rely on a "gc". Add a GC where
needed.

> > ---
> >  tools/libxl/libxl_event.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> > index 484f9bab4d..0370b6acdd 100644
> > --- a/tools/libxl/libxl_event.c
> > +++ b/tools/libxl/libxl_event.c
> > @@ -248,6 +248,7 @@ void libxl__ev_fd_deregister(libxl__gc *gc, libxl__ev_fd *ev)
> >  short libxl__fd_poll_recheck(libxl__egc *egc, int fd, short events) {
> >      struct pollfd check;
> >      int r;
> > +    EGC_GC;
> >  
> >      for (;;) {
> >          check.fd = fd;
> > @@ -336,7 +337,7 @@ static void time_done_debug(libxl__gc *gc, const char *func,
> >                              libxl__ev_time *ev, int rc)
> >  {
> >  #ifdef DEBUG
> > -    libxl__log(CTX, XTL_DEBUG, -1,__FILE__,0,func,
> > +    libxl__log(CTX, XTL_DEBUG, -1, __FILE__, 0, func, INVALID_DOMID,
> >                 "ev_time=%p done rc=%d .func=%p infinite=%d abs=%lu.%06lu",
> >                 ev, rc, ev->func, ev->infinite,
> >                 (unsigned long)ev->abs.tv_sec, (unsigned long)ev->abs.tv_usec);
> > @@ -445,6 +446,8 @@ void libxl__ev_time_deregister(libxl__gc *gc, libxl__ev_time *ev)
> >  
> >  static void time_occurs(libxl__egc *egc, libxl__ev_time *etime, int rc)
> >  {
> > +    EGC_GC;
> > +
> >      DBG("ev_time=%p occurs abs=%lu.%06lu",
> >          etime, (unsigned long)etime->abs.tv_sec,
> >          (unsigned long)etime->abs.tv_usec);
> > @@ -1192,6 +1195,7 @@ static int afterpoll_check_fd(libxl__poller *poller,
> >  static void fd_occurs(libxl__egc *egc, libxl__ev_fd *efd, short revents_ign)
> >  {
> >      short revents_current = libxl__fd_poll_recheck(egc, efd->fd, efd->events);
> > +    EGC_GC;
> >  
> >      DBG("ev_fd=%p occurs fd=%d events=%x revents_ign=%x revents_current=%x",
> >          efd, efd->fd, efd->events, revents_ign, revents_current);
> > @@ -2117,6 +2121,8 @@ int libxl_ao_abort(libxl_ctx *ctx, const libxl_asyncop_how *how)
> >  int libxl__ao_aborting(libxl__ao *ao)
> >  {
> >      libxl__ao *root = ao_nested_root(ao);
> > +    AO_GC;
> > +
> >      if (root->aborting) {
> >          DBG("ao=%p: aborting at explicit check (root=%p)", ao, root);
> >          return ERROR_ABORTED;

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
  2018-04-16 18:09   ` Anthony PERARD
  2018-04-17  9:18   ` Anthony PERARD
@ 2018-04-20 18:37   ` Ian Jackson
  2018-04-23 16:54     ` Anthony PERARD
  2 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2018-04-20 18:37 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu

Anthony PERARD writes ("[RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> When QEMU is restricted, the qemu on the receiving side cann't write
> anything to xenstore once the migration is started. So it cann't tell
> libxl that it is ready to continue running the guest.
...
> This patch creates a pipe, give the write-end to qemu, and wait for
> something to be written to it. (We could check if it is actually the QMP
> greeting message.)

This is indeed the kind of thing I had in mind in our IRL
conversation.

> QEMU is asked to setup a QMP server on this pipe, but even if it is a
> one-way only, qemu will write the QMP greeting message to the pipe.
> This is done with:
> -add-fd, to create a fdset which is use later.
> -chardev 'file,path=/dev/fdset/1,append=true', this open a char device
> on the write-end of the pipe, tell qemu that the FD is write-only, and
> not to run truncate on it.
> -mon, just start the QMP server on this new chardev.

Have you considered socketpair() ?  That avoids the oddnes of passing
qemu a write-only fd.

Anyway, I'm not sure why this approach justifies HACK.  Are all the
things you are asking qemu to do not fully supported ?

> This patch copies most of "xswait" and call it "qmpwait". This is
> probably not the best way forward due to duplication.

Ah.  Indeed :-).

I haven't reviewed the code yet, only your description.

Ian.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
  2018-04-16 17:32 ` [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read Anthony PERARD
@ 2018-04-23  9:03   ` Wei Liu
  2018-04-23 14:50     ` Anthony PERARD
  0 siblings, 1 reply; 28+ messages in thread
From: Wei Liu @ 2018-04-23  9:03 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote:
> In qmp_next(), the inner loop should only try to parse messages from
> QMP, if there is more than one.
> 
> The handling of the receive buffer ('incomplete'), should be done at the
> same scope level as read(). It doesn't need to be handle more that once
> after a read.
> 

In general I agree this is a better idea than the current code.

> Before this patch, when on message what handled, the inner loop would

Sorry, I failed to parse "when on message what handled".

> restart by adding the 'buffer' into 'incomplete' (after reallocation).
> Since 'rd' was not reset, the buffer would be strcat a second time.
> After that, the stream from the QMP server would have syntax error, and
> the parsor would throw errors.
> 
> This is unlikely to happen as the receive buffer is very large. And
> receiving two messages in a row is unlikely. In the current case, this
> could be an event and a response to a command.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_qmp.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 4d207c3842..a25f445fb6 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
>  
>          DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
>  
> +        if (incomplete) {
> +            size_t current_pos = s - incomplete;
> +            incomplete = libxl__realloc(gc, incomplete,
> +                                        incomplete_size + rd + 1);
> +            strncat(incomplete + incomplete_size, qmp->buffer, rd);
> +            s = incomplete + current_pos;

This can be dropped, because s is not changed. It is just the reversal
of what is a few lines above.

> +            incomplete_size += rd;
> +            s_end = incomplete + incomplete_size;
> +        } else {
> +            incomplete = libxl__strndup(gc, qmp->buffer, rd);
> +            incomplete_size = rd;
> +            s = incomplete;
> +            s_end = s + rd;
> +            rd = 0;

This can be dropped.

And I think we should take this change to change "incomplete" to
something more meaningful, like "qmp_msg_buf".

> +        }
> +
>          do {
>              char *end = NULL;
> -            if (incomplete) {
> -                size_t current_pos = s - incomplete;
> -                incomplete = libxl__realloc(gc, incomplete,
> -                                            incomplete_size + rd + 1);
> -                strncat(incomplete + incomplete_size, qmp->buffer, rd);
> -                s = incomplete + current_pos;
> -                incomplete_size += rd;
> -                s_end = incomplete + incomplete_size;
> -            } else {
> -                incomplete = libxl__strndup(gc, qmp->buffer, rd);
> -                incomplete_size = rd;
> -                s = incomplete;
> -                s_end = s + rd;
> -                rd = 0;
> -            }
>  
>              end = strstr(s, "\r\n");
>              if (end) {
> -- 
> Anthony PERARD
> 

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU
  2018-04-16 17:32 ` [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU Anthony PERARD
@ 2018-04-23  9:04   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-23  9:04 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:23PM +0100, Anthony PERARD wrote:
> Adding the ability to send a file descriptor from libxl to QEMU via the
> QMP interface. This will be use with the "add-fd" QMP command.
> 
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor
  2018-04-16 17:32 ` [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor Anthony PERARD
@ 2018-04-23  9:20   ` Wei Liu
  2018-04-23 15:45     ` Anthony PERARD
  2018-04-24  9:46   ` Wei Liu
  1 sibling, 1 reply; 28+ messages in thread
From: Wei Liu @ 2018-04-23  9:20 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:24PM +0100, Anthony PERARD wrote:
> In case QEMU have restricted access to the system, open the file for it,
> and QEMU will save its state to this file descritor.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

I think this is going to break FreeBSD?

And what is /dev/fdset exactly? I don't seem to have it on my Debian
workstation (with Linux 4.15). So this change can potentially be broken
on Linux as well?

Wei.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 7/9] libxl_qmp: Implement query-status command
  2018-04-16 17:32 ` [RFC v2 7/9] libxl_qmp: Implement query-status command Anthony PERARD
@ 2018-04-23  9:24   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-23  9:24 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:25PM +0100, Anthony PERARD wrote:
> It check via QMP if QEMU as reach the intended status.

Some typos.

"... checks ... if QEMU has reached ..."

> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  tools/libxl/libxl_internal.h |  3 +++
>  tools/libxl/libxl_qmp.c      | 39 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6352380644..3764d26463 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1857,6 +1857,9 @@ _hidden int libxl__qmp_nbd_server_stop(libxl__gc *gc, int domid);
>  _hidden int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid,
>                                           const char *parant,
>                                           const char *child, const char *node);
> +_hidden int libxl__qmp_query_status(libxl__gc *gc,
> +                                    int domid,
> +                                    const char *intended_status);
>  /* run a hmp command in qmp mode */
>  _hidden int libxl__qmp_hmp(libxl__gc *gc, int domid, const char *command_line,
>                             char **out);
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 3bb0f28bea..83bd804219 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1246,6 +1246,45 @@ int libxl__qmp_x_blockdev_change(libxl__gc *gc, int domid, const char *parent,
>      return qmp_run_command(gc, domid, "x-blockdev-change", args, NULL, NULL);
>  }
>  
> +static int qmp_check_status(libxl__qmp_handler *qmp,
> +                            const libxl__json_object *response,
> +                            void *opaque)
> +{
> +    char **status = opaque;
> +    GC_INIT(qmp->ctx);
> +    const libxl__json_object *o;
> +
> +    o = libxl__json_map_get("status", response, JSON_STRING);
> +    if (!o)
> +        return 1;
> +    *status = libxl__strdup(gc, libxl__json_object_get_string(o));

Missing GC_FREE in the exit paths.

The rest looks OK.

Wei.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
  2018-04-23  9:03   ` Wei Liu
@ 2018-04-23 14:50     ` Anthony PERARD
  2018-04-23 15:01       ` Wei Liu
  0 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-23 14:50 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Ian Jackson

On Mon, Apr 23, 2018 at 10:03:39AM +0100, Wei Liu wrote:
> On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote:
> > In qmp_next(), the inner loop should only try to parse messages from
> > QMP, if there is more than one.
> > 
> > The handling of the receive buffer ('incomplete'), should be done at the
> > same scope level as read(). It doesn't need to be handle more that once
> > after a read.
> > 
> 
> In general I agree this is a better idea than the current code.
> 
> > Before this patch, when on message what handled, the inner loop would
> 
> Sorry, I failed to parse "when on message what handled".

I probably wanted to write: "when one message was handled". Is this
better?

> > restart by adding the 'buffer' into 'incomplete' (after reallocation).
> > Since 'rd' was not reset, the buffer would be strcat a second time.
> > After that, the stream from the QMP server would have syntax error, and
> > the parsor would throw errors.
> > 
> > This is unlikely to happen as the receive buffer is very large. And
> > receiving two messages in a row is unlikely. In the current case, this
> > could be an event and a response to a command.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> >  tools/libxl/libxl_qmp.c | 31 ++++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index 4d207c3842..a25f445fb6 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> >  
> >          DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
> >  
> > +        if (incomplete) {
> > +            size_t current_pos = s - incomplete;
> > +            incomplete = libxl__realloc(gc, incomplete,
> > +                                        incomplete_size + rd + 1);
> > +            strncat(incomplete + incomplete_size, qmp->buffer, rd);
> > +            s = incomplete + current_pos;
> 
> This can be dropped, because s is not changed. It is just the reversal
> of what is a few lines above.

No, realloc may change the location of the allocated memory. So, between
"current_pos = s - incomplete" and "s = incomplete + current_pos", the
value of 'incomplete' may have changed.

> > +            incomplete_size += rd;
> > +            s_end = incomplete + incomplete_size;
> > +        } else {
> > +            incomplete = libxl__strndup(gc, qmp->buffer, rd);
> > +            incomplete_size = rd;
> > +            s = incomplete;
> > +            s_end = s + rd;
> > +            rd = 0;
> 
> This can be dropped.
> 
> And I think we should take this change to change "incomplete" to
> something more meaningful, like "qmp_msg_buf".

I think that can be done.


-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read
  2018-04-23 14:50     ` Anthony PERARD
@ 2018-04-23 15:01       ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-23 15:01 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 23, 2018 at 03:50:27PM +0100, Anthony PERARD wrote:
> On Mon, Apr 23, 2018 at 10:03:39AM +0100, Wei Liu wrote:
> > On Mon, Apr 16, 2018 at 06:32:22PM +0100, Anthony PERARD wrote:
> > > In qmp_next(), the inner loop should only try to parse messages from
> > > QMP, if there is more than one.
> > > 
> > > The handling of the receive buffer ('incomplete'), should be done at the
> > > same scope level as read(). It doesn't need to be handle more that once
> > > after a read.
> > > 
> > 
> > In general I agree this is a better idea than the current code.
> > 
> > > Before this patch, when on message what handled, the inner loop would
> > 
> > Sorry, I failed to parse "when on message what handled".
> 
> I probably wanted to write: "when one message was handled". Is this
> better?

Yes.

> 
> > > restart by adding the 'buffer' into 'incomplete' (after reallocation).
> > > Since 'rd' was not reset, the buffer would be strcat a second time.
> > > After that, the stream from the QMP server would have syntax error, and
> > > the parsor would throw errors.
> > > 
> > > This is unlikely to happen as the receive buffer is very large. And
> > > receiving two messages in a row is unlikely. In the current case, this
> > > could be an event and a response to a command.
> > > 
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > >  tools/libxl/libxl_qmp.c | 31 ++++++++++++++++---------------
> > >  1 file changed, 16 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > > index 4d207c3842..a25f445fb6 100644
> > > --- a/tools/libxl/libxl_qmp.c
> > > +++ b/tools/libxl/libxl_qmp.c
> > > @@ -524,23 +524,24 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> > >  
> > >          DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
> > >  
> > > +        if (incomplete) {
> > > +            size_t current_pos = s - incomplete;
> > > +            incomplete = libxl__realloc(gc, incomplete,
> > > +                                        incomplete_size + rd + 1);
> > > +            strncat(incomplete + incomplete_size, qmp->buffer, rd);
> > > +            s = incomplete + current_pos;
> > 
> > This can be dropped, because s is not changed. It is just the reversal
> > of what is a few lines above.
> 
> No, realloc may change the location of the allocated memory. So, between
> "current_pos = s - incomplete" and "s = incomplete + current_pos", the
> value of 'incomplete' may have changed.

Indeed, I missed that somehow.

Wei.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor
  2018-04-23  9:20   ` Wei Liu
@ 2018-04-23 15:45     ` Anthony PERARD
  2018-04-24  9:37       ` Wei Liu
  0 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-23 15:45 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, Ian Jackson

On Mon, Apr 23, 2018 at 10:20:42AM +0100, Wei Liu wrote:
> On Mon, Apr 16, 2018 at 06:32:24PM +0100, Anthony PERARD wrote:
> > In case QEMU have restricted access to the system, open the file for it,
> > and QEMU will save its state to this file descritor.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> I think this is going to break FreeBSD?

If the previous patch works on FreeBSD (Learned to send FD through QMP
to QEMU), this patch shouldn't break anything. Looking at how
libxl__sendmsg_fds() is implemented, and how the receive side is
implemented in QEMU, there doesn't seems to be any difference on
FreeBSD, so I guess it's going to work fine.

> And what is /dev/fdset exactly? I don't seem to have it on my Debian
> workstation (with Linux 4.15). So this change can potentially be broken
> on Linux as well?

/dev/fdset is a QEMU internal API. You can add a file descriptor to a
fd-set via QMP or via the command line. And later, you can give a path
to something in QEMU (maybe a block driver) which refer to a fd-set
created earlier. The path would be "/dev/fdset/$fdset_number". There can
be several fd in a fd-set, qemu will just look for the first one which
have the right permission, like read or write permission.

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-20 18:37   ` Ian Jackson
@ 2018-04-23 16:54     ` Anthony PERARD
  2018-04-23 16:56       ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2018-04-23 16:54 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Wei Liu

On Fri, Apr 20, 2018 at 07:37:17PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> > When QEMU is restricted, the qemu on the receiving side cann't write
> > anything to xenstore once the migration is started. So it cann't tell
> > libxl that it is ready to continue running the guest.
> ...
> > This patch creates a pipe, give the write-end to qemu, and wait for
> > something to be written to it. (We could check if it is actually the QMP
> > greeting message.)
> 
> This is indeed the kind of thing I had in mind in our IRL
> conversation.

Good, where are heading in the right direction.

> > QEMU is asked to setup a QMP server on this pipe, but even if it is a
> > one-way only, qemu will write the QMP greeting message to the pipe.
> > This is done with:
> > -add-fd, to create a fdset which is use later.
> > -chardev 'file,path=/dev/fdset/1,append=true', this open a char device
> > on the write-end of the pipe, tell qemu that the FD is write-only, and
> > not to run truncate on it.
> > -mon, just start the QMP server on this new chardev.
> 
> Have you considered socketpair() ?  That avoids the oddnes of passing
> qemu a write-only fd.

I did considered to pass an open socket to QEMU, I didn't know that
socketpair() existed. But I don't think that would help with the current
version of QEMU (2.11 and older). There are only two ways to give a
file descriptor to qemu, these are:
  -chardev pipe,..
  -chardev file,...
And in both case, QEMU is only going to write to the fd anyway.

But, with the to be release QEMU 2.12, there is a new interface that
allow to pre-open a socket:
  -chardev socket,fd=?
See https://git.qemu.org/?p=qemu.git;a=commit;h=0935700f8544033ebbd41e1f13cd528f8a58d24d

With that, we could open our usual "/var/run/xen/qmp-libxl-$domid"
socket for QEMU. That would work when the retricted fonctionnality is
also available. But that cann't be use unconditionnaly.

-- 
Anthony PERARD

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore
  2018-04-23 16:54     ` Anthony PERARD
@ 2018-04-23 16:56       ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2018-04-23 16:56 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu

Anthony PERARD writes ("Re: [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore"):
> But, with the to be release QEMU 2.12, there is a new interface that
> allow to pre-open a socket:
>   -chardev socket,fd=?
> See https://git.qemu.org/?p=qemu.git;a=commit;h=0935700f8544033ebbd41e1f13cd528f8a58d24d
> 
> With that, we could open our usual "/var/run/xen/qmp-libxl-$domid"
> socket for QEMU. That would work when the retricted fonctionnality is
> also available. But that cann't be use unconditionnaly.

Right.

It's a bit ugly having two methods but that's the price of
compatibility.

Ian.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor
  2018-04-23 15:45     ` Anthony PERARD
@ 2018-04-24  9:37       ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-24  9:37 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 23, 2018 at 04:45:28PM +0100, Anthony PERARD wrote:
> On Mon, Apr 23, 2018 at 10:20:42AM +0100, Wei Liu wrote:
> > On Mon, Apr 16, 2018 at 06:32:24PM +0100, Anthony PERARD wrote:
> > > In case QEMU have restricted access to the system, open the file for it,
> > > and QEMU will save its state to this file descritor.
> > > 
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > I think this is going to break FreeBSD?
> 
> If the previous patch works on FreeBSD (Learned to send FD through QMP
> to QEMU), this patch shouldn't break anything. Looking at how
> libxl__sendmsg_fds() is implemented, and how the receive side is
> implemented in QEMU, there doesn't seems to be any difference on
> FreeBSD, so I guess it's going to work fine.
> 
> > And what is /dev/fdset exactly? I don't seem to have it on my Debian
> > workstation (with Linux 4.15). So this change can potentially be broken
> > on Linux as well?
> 
> /dev/fdset is a QEMU internal API. You can add a file descriptor to a
> fd-set via QMP or via the command line. And later, you can give a path
> to something in QEMU (maybe a block driver) which refer to a fd-set
> created earlier. The path would be "/dev/fdset/$fdset_number". There can
> be several fd in a fd-set, qemu will just look for the first one which
> have the right permission, like read or write permission.

Oh, I was thinking /dev/fdset was some sort of Linux specific thing. It
is interesting that QEMU actually creates /dev/fdset. That means it
should work fine (at least in theory) for both FreeBSD and Linux.

Wei.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor
  2018-04-16 17:32 ` [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor Anthony PERARD
  2018-04-23  9:20   ` Wei Liu
@ 2018-04-24  9:46   ` Wei Liu
  1 sibling, 0 replies; 28+ messages in thread
From: Wei Liu @ 2018-04-24  9:46 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: xen-devel, Wei Liu, Ian Jackson

On Mon, Apr 16, 2018 at 06:32:24PM +0100, Anthony PERARD wrote:
> In case QEMU have restricted access to the system, open the file for it,
> and QEMU will save its state to this file descritor.
> 
> 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2018-04-24  9:46 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16 17:32 [RFC v2 0/9] libxl: Enable save/restore/migration of a restricted QEMU Anthony PERARD
2018-04-16 17:32 ` [RFC v2 1/9] libxl_event: Fix DEBUG prints Anthony PERARD
2018-04-19  8:17   ` Wei Liu
2018-04-19 11:01     ` Anthony PERARD
2018-04-16 17:32 ` [RFC v2 2/9] libxl_qmp: Documentation of the logic of the QMP client Anthony PERARD
2018-04-19  8:19   ` Wei Liu
2018-04-16 17:32 ` [RFC v2 3/9] libxl_qmp: Fix use of DEBUG_RECEIVED Anthony PERARD
2018-04-19  8:22   ` Wei Liu
2018-04-16 17:32 ` [RFC v2 4/9] libxl_qmp: Move the buffer realloc to the same scope level as read Anthony PERARD
2018-04-23  9:03   ` Wei Liu
2018-04-23 14:50     ` Anthony PERARD
2018-04-23 15:01       ` Wei Liu
2018-04-16 17:32 ` [RFC v2 5/9] libxl: Learned to send FD through QMP to QEMU Anthony PERARD
2018-04-23  9:04   ` Wei Liu
2018-04-16 17:32 ` [RFC v2 6/9] libxl: Have QEMU save its state to a file descriptor Anthony PERARD
2018-04-23  9:20   ` Wei Liu
2018-04-23 15:45     ` Anthony PERARD
2018-04-24  9:37       ` Wei Liu
2018-04-24  9:46   ` Wei Liu
2018-04-16 17:32 ` [RFC v2 7/9] libxl_qmp: Implement query-status command Anthony PERARD
2018-04-23  9:24   ` Wei Liu
2018-04-16 17:32 ` [RFC v2 8/9] HACK libxl_exec: Check QEMU status via QMP instead of xenstore Anthony PERARD
2018-04-16 18:09   ` Anthony PERARD
2018-04-17  9:18   ` Anthony PERARD
2018-04-20 18:37   ` Ian Jackson
2018-04-23 16:54     ` Anthony PERARD
2018-04-23 16:56       ` Ian Jackson
2018-04-16 17:32 ` [RFC v2 9/9] libxl_qmp: Add a warning to not trust QEMU Anthony PERARD

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.