[PATCH v3 00/31] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*

[PATCH v3 00/31] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*

[Anthony PERARD]

The real meat in this patch series start with patch
"libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"
which implement libxl__ev_qmp_* functions to turn the QMP client into
asynchronous mode.

This comes with two examples on how to use it:
* "libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp"
  with patches:
  - "libxl_qmp: Implement libxl__qmp_insert_cdrom_ev"
  - "libxl_disk: Cut libxl_cdrom_insert into step"
* "libxl: QEMU startup sync based on QMP"
  which can use QMP to find out when QEMU as started.
  this requires: "libxl_dm: Pre-open QMP socket for QEMU"
  But that only works with dm_restrict=1 as explain in the patch.

The first few patches do some cleanup and fixes of the current qmp client
implementation, mostly because it bothered me as I think we should remove the
current implementation. There is also two patches to allow to save a restricted
QEMU, but that would need to be converted over to libxl__ev_qmp_*.

There is still one bug that I haven't fix yet. When creating a guest with
dm_restrict=1, the call to libxl__qmp_initializations() is going to fail
because libxl is still connected to the QMP socket. But libxl doesn't care
about failure, and that just mean that `xl console` will not work and vnc will
not have any password. save/restore of the same guest will works fine because
libxl__ev_qmp_* will have an oportunity to disconnect from the socket before
libxl__qmp_initializations() is called.

Cheers,

Anthony PERARD (31):
  libxl_event: Fix DEBUG prints
  libxl_qmp: Documentation of the logic of the QMP client
  libxl_qmp: Fix use of DEBUG_RECEIVED
  libxl_json: fix build with DEBUG_ANSWER
  libxl_qmp: Move the buffer realloc to the same scope level as read
  libxl_qmp: Add a warning to not trust QEMU
  libxl_qmp: Learned to send FD through QMP to QEMU
  libxl_qmp: Have QEMU save its state to a file descriptor
  libxl_qmp: Move struct sockaddr_un variable to qmp_open()
  libxl_qmp: Move buffers to the stack of qmp_next.
  libxl_qmp: Remove unused yajl_ctx form handler
  libxl_json: constify libxl__json_object_to_yajl_gen arguments
  libxl_qmp: Separate QMP message generation from qmp_send_prepare
  libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP
  libxl_qmp_ev: Implement fd callback and read data
  libxl_json: Allow partial parsing
  libxl_json: Enable yajl_allow_trailing_garbage
  libxl_json: libxl__json_object_to_json
  libxl_qmp_ev: Parse JSON input from QMP
  libxl_qmp: Introduce libxl__ev_qmp functions
  libxl_qmp_ev: Handle write to socket
  libxl_qmp: Simplify qmp_response_type() prototype
  libxl_qmp_ev: Handle messages from QEMU
  libxl_qmp_ev: Respond to QMP greeting
  libxl_qmp_ev: Disconnect QMP when no more events
  libxl_qmp: Disable beautify for QMP generated cmd
  libxl_qmp: Implement libxl__qmp_insert_cdrom_ev
  libxl_disk: Cut libxl_cdrom_insert into step
  libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp
  libxl_dm: Pre-open QMP socket for QEMU
  libxl: QEMU startup sync based on QMP

 tools/libxl/libxl.c                  |    4 +
 tools/libxl/libxl_disk.c             |  242 ++++--
 tools/libxl/libxl_dm.c               |   80 +-
 tools/libxl/libxl_event.c            |    8 +-
 tools/libxl/libxl_exec.c             |   44 ++
 tools/libxl/libxl_internal.h         |   67 +-
 tools/libxl/libxl_json.c             |  142 +++-
 tools/libxl/libxl_json.h             |    5 +-
 tools/libxl/libxl_qmp.c              | 1024 ++++++++++++++++++++++++--
 tools/libxl/libxl_types_internal.idl |   14 +
 10 files changed, 1478 insertions(+), 152 deletions(-)

-- 
Anthony PERARD


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

[PATCH v3 01/31] libxl_event: Fix DEBUG prints

[Anthony PERARD]

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.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---

Notes:
    v3:
    - Add a commit message.
    
    New in RFC v2

 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

Re: [PATCH v3 01/31] libxl_event: Fix DEBUG prints

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 01/31] libxl_event: Fix DEBUG prints"):
> 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.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

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

[PATCH v3 02/31] libxl_qmp: Documentation of the logic of the QMP client

[Anthony PERARD]

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---

Notes:
    v3:
    - Add documentation of the qmp_callback_t type.
    
    New in RFC v2

 tools/libxl/libxl_qmp.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index be1fda18ba..36b183c8c4 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 response 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>
@@ -43,6 +79,12 @@
 #define QMP_RECEIVE_BUFFER_SIZE 4096
 #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
 
+/*
+ * qmp_callback_t is call whenever a message from QMP contain the "id"
+ * associated with the callback.
+ * "tree" contain the JSON tree that is in "return" of a QMP message. If QMP
+ * sent an error message, "tree" will be NULL.
+ */
 typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
                               const libxl__json_object *tree,
                               void *opaque);
-- 
Anthony PERARD


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

Re: [PATCH v3 02/31] libxl_qmp: Documentation of the logic of the QMP client

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 02/31] libxl_qmp: Documentation of the logic of the QMP client"):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

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

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

[PATCH v3 03/31] libxl_qmp: Fix use of DEBUG_RECEIVED

[Anthony PERARD]

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>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---

Notes:
    New in RFC v2

 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 36b183c8c4..c42e2bf4b8 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -528,7 +528,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

Re: [PATCH v3 03/31] libxl_qmp: Fix use of DEBUG_RECEIVED

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 03/31] libxl_qmp: Fix use of DEBUG_RECEIVED"):
> This patch fix complilation error with #define DEBUG_RECEIVED of the
> macro DEBUG_REPORT_RECEIVED.

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

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

[PATCH v3 04/31] libxl_json: fix build with DEBUG_ANSWER

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 0823b8cfd2..dc93a88ef1 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -59,8 +59,8 @@ struct libxl__yajl_ctx {
         const unsigned char *buf = NULL; \
         size_t len = 0; \
         yajl_gen_get_buf((yajl_ctx)->g, &buf, &len); \
-        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), LIBXL__LOG_DEBUG,
-		   "response:\n", buf); \
+        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), XTL_DEBUG, \
+		   "response: %s\n", buf); \
         yajl_gen_free((yajl_ctx)->g); \
         (yajl_ctx)->g = NULL; \
     } while (0)
-- 
Anthony PERARD


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

Re: [PATCH v3 04/31] libxl_json: fix build with DEBUG_ANSWER

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 04/31] libxl_json: fix build with DEBUG_ANSWER"):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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

Although,

>          yajl_gen_get_buf((yajl_ctx)->g, &buf, &len); \
> -        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), LIBXL__LOG_DEBUG,
> -		   "response:\n", buf); \
> +        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), XTL_DEBUG, \
> +		   "response: %s\n", buf); \

I'm not sure why you changed LIBXL__LOG_DEBUG to XTL_DEBUG.  It would
be nice to mention it in the commit message.  Personally I would
prefer it because (i) it's shorter (ii) we're not likely to want to
decouple the libxl log levels from the XTL ones (iii) if we do, in the
future, it will be an easy search-and-replace.

Ian.

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

Re: [PATCH v3 04/31] libxl_json: fix build with DEBUG_ANSWER

[Anthony PERARD]

On Wed, Jun 27, 2018 at 03:22:56PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 04/31] libxl_json: fix build with DEBUG_ANSWER"):
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Although,
> 
> >          yajl_gen_get_buf((yajl_ctx)->g, &buf, &len); \
> > -        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), LIBXL__LOG_DEBUG,
> > -		   "response:\n", buf); \
> > +        LIBXL__LOG(libxl__gc_owner((yajl_ctx)->gc), XTL_DEBUG, \
> > +		   "response: %s\n", buf); \
> 
> I'm not sure why you changed LIBXL__LOG_DEBUG to XTL_DEBUG.  It would

I'm not sure either.

> be nice to mention it in the commit message.  Personally I would
> prefer it because (i) it's shorter (ii) we're not likely to want to
> decouple the libxl log levels from the XTL ones (iii) if we do, in the
> future, it will be an easy search-and-replace.

Ok, I'll keep the change, and add to the commit message:
  Also replace LIBXL__LOG_DEBUG by XTL_DEBUG, because it's shorter and
  more often used in libxl.

-- 
Anthony PERARD

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

[PATCH v3 05/31] libxl_qmp: Move the buffer realloc to the same scope level as read

[Anthony PERARD]

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

Notes:
    New in RFC v2

 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 c42e2bf4b8..58ecd4baf3 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -530,23 +530,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

Re: [PATCH v3 05/31] libxl_qmp: Move the buffer realloc to the same scope level as read

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 05/31] libxl_qmp: Move the buffer realloc to the same scope level as read"):
> 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.

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

However, I have not reviewed the buffer handling in detail for
off-by-one errors etc.

I think it would be best for me to do a proper security-focused review
of the whole qmp arrangement after your series.

Thanks,
Ian.

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

[PATCH v3 06/31] libxl_qmp: Add a warning to not trust QEMU

[Anthony PERARD]

... 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 58ecd4baf3..8b3ed94868 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

Re: [PATCH v3 06/31] libxl_qmp: Add a warning to not trust QEMU

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 06/31] libxl_qmp: Add a warning to not trust QEMU"):
> ... even if it is not the case for the current code.

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

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

[PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU

[Anthony PERARD]

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>
---
 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 8b3ed94868..e1fcce2291 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -125,6 +125,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,
@@ -423,6 +426,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;
 }
 
@@ -648,9 +653,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

Re: [PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU"):
> 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.

Do you know which byte of the message the fd should be attached to ?

What if qemu reads a partial message, discarding the fd, and then
reads the rest of the message, finding the fd missing ?  Or something.

Ian.

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

Re: [PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU

[Anthony PERARD]

On Wed, Jun 27, 2018 at 03:26:51PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU"):
> > 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.
> 
> Do you know which byte of the message the fd should be attached to ?

Yes, anywhere before the last byte of the command that is going to use
the fd. QEMU is going to store any fd received until a command is using
it.

We should be able to the the fd, then sent several qmp command, then the
add-fd command, and I think that will work fine.

> What if qemu reads a partial message, discarding the fd, and then
> reads the rest of the message, finding the fd missing ?  Or something.

I don't think QEMU discards fds until a command is using it, or maybe
until a new fd comes in (I did not check this second thought).

-- 
Anthony PERARD

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

Re: [PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v3 07/31] libxl_qmp: Learned to send FD through QMP to QEMU"):
> Yes, anywhere before the last byte of the command that is going to use
> the fd. QEMU is going to store any fd received until a command is using
> it.

Great.  Can you maybe add a comment about that ?

> We should be able to the the fd, then sent several qmp command, then the
> add-fd command, and I think that will work fine.

Let's not, though :-).

> I don't think QEMU discards fds until a command is using it, or maybe
> until a new fd comes in (I did not check this second thought).

If you call read() (or recvmsg() without suitable ancillary data
parameters), the kernel will discard any fds that might have
accompanied the data you read.

Ian.

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

[PATCH v3 08/31] libxl_qmp: Have QEMU save its state to a file descriptor

[Anthony PERARD]

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 e1fcce2291..c71c3cbca4 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -998,25 +998,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

Re: [PATCH v3 08/31] libxl_qmp: Have QEMU save its state to a file descriptor

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 08/31] libxl_qmp: Have QEMU save its state to a file descriptor"):
> In case QEMU have restricted access to the system, open the file for it,
> and QEMU will save its state to this file descritor.

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

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

[PATCH v3 09/31] libxl_qmp: Move struct sockaddr_un variable to qmp_open()

[Anthony PERARD]

This variable is only used once, no need to keep it in the handler.

Also fix coding style (remove space after sizeof).
And allow strncpy to use all the space in sun_path.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index c71c3cbca4..251840a155 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -105,7 +105,6 @@ typedef struct callback_id_pair {
 } callback_id_pair;
 
 struct libxl__qmp_handler {
-    struct sockaddr_un addr;
     int qmp_fd;
     bool connected;
     time_t timeout;
@@ -436,6 +435,7 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
 {
     int ret = -1;
     int i = 0;
+    struct sockaddr_un addr;
 
     qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0);
     if (qmp->qmp_fd < 0) {
@@ -452,18 +452,16 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
         goto out;
     }
 
-    if (sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) {
+    if (sizeof(addr.sun_path) <= strlen(qmp_socket_path)) {
         ret = -1;
         goto out;
     }
-    memset(&qmp->addr, 0, sizeof (qmp->addr));
-    qmp->addr.sun_family = AF_UNIX;
-    strncpy(qmp->addr.sun_path, qmp_socket_path,
-            sizeof (qmp->addr.sun_path)-1);
+    memset(&addr, 0, sizeof(addr));
+    addr.sun_family = AF_UNIX;
+    strncpy(addr.sun_path, qmp_socket_path, sizeof(addr.sun_path) - 1);
 
     do {
-        ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
-                      sizeof (qmp->addr));
+        ret = connect(qmp->qmp_fd, (struct sockaddr *) &addr, sizeof(addr));
         if (ret == 0)
             break;
         if (errno == ENOENT || errno == ECONNREFUSED) {
-- 
Anthony PERARD


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

Re: [PATCH v3 09/31] libxl_qmp: Move struct sockaddr_un variable to qmp_open()

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 09/31] libxl_qmp: Move struct sockaddr_un variable to qmp_open()"):
...
> And allow strncpy to use all the space in sun_path.

I wasn't able to see in the diff what this entry in the commit message
refers to.

Thanks,
Ian.

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

Re: [PATCH v3 09/31] libxl_qmp: Move struct sockaddr_un variable to qmp_open()

[Anthony PERARD]

On Wed, Jun 27, 2018 at 03:31:21PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 09/31] libxl_qmp: Move struct sockaddr_un variable to qmp_open()"):
> ...
> > And allow strncpy to use all the space in sun_path.
> 
> I wasn't able to see in the diff what this entry in the commit message
> refers to.

I probably forgot to remove the -1 in the strncpy call or was thinking
about another patch.

-- 
Anthony PERARD

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

[PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next.

[Anthony PERARD]

That buffer is only used locally, and never reuse accross different call
of qmp_next. So remove it form the handler.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 251840a155..4da84dcf16 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -111,7 +111,6 @@ struct libxl__qmp_handler {
     /* wait_for_id will be used by the synchronous send function */
     int wait_for_id;
 
-    char buffer[QMP_RECEIVE_BUFFER_SIZE + 1];
     libxl__yajl_ctx *yajl_ctx;
 
     libxl_ctx *ctx;
@@ -501,6 +500,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
     char *incomplete = NULL;
     size_t incomplete_size = 0;
     int rc = 0;
+    char buffer[QMP_RECEIVE_BUFFER_SIZE + 1];
 
     do {
         fd_set rfds;
@@ -524,7 +524,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
             return -1;
         }
 
-        rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
+        rd = read(qmp->qmp_fd, buffer, QMP_RECEIVE_BUFFER_SIZE);
         if (rd == 0) {
             LOGD(ERROR, qmp->domid, "Unexpected end of socket");
             return -1;
@@ -532,20 +532,20 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
             LOGED(ERROR, qmp->domid, "Socket read error");
             return rd;
         }
-        qmp->buffer[rd] = '\0';
+        buffer[rd] = '\0';
 
-        DEBUG_REPORT_RECEIVED(qmp->domid, qmp->buffer, (int)rd);
+        DEBUG_REPORT_RECEIVED(qmp->domid, 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);
+            strncat(incomplete + incomplete_size, buffer, rd);
             s = incomplete + current_pos;
             incomplete_size += rd;
             s_end = incomplete + incomplete_size;
         } else {
-            incomplete = libxl__strndup(gc, qmp->buffer, rd);
+            incomplete = libxl__strndup(gc, buffer, rd);
             incomplete_size = rd;
             s = incomplete;
             s_end = s + rd;
-- 
Anthony PERARD


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

Re: [PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next.

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next."):
> That buffer is only used locally, and never reuse accross different call
> of qmp_next. So remove it form the handler.

How big is this buffer ?  I think you're moving it from the heap to
the stack ?  Do we need to worry about stack size ?

Thanks,
Ian.

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

Re: [PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next.

[Anthony PERARD]

On Wed, Jun 27, 2018 at 03:32:32PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next."):
> > That buffer is only used locally, and never reuse accross different call
> > of qmp_next. So remove it form the handler.
> 
> How big is this buffer ?

It's 4k

> I think you're moving it from the heap to
> the stack ?  Do we need to worry about stack size ?

We can probably remove this patch from the series. It is not important.

-- 
Anthony PERARD

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

Re: [PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next.

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next."):
> On Wed, Jun 27, 2018 at 03:32:32PM +0100, Ian Jackson wrote:
> > Anthony PERARD writes ("[PATCH v3 10/31] libxl_qmp: Move buffers to the stack of qmp_next."):
> > > That buffer is only used locally, and never reuse accross different call
> > > of qmp_next. So remove it form the handler.
> > 
> > How big is this buffer ?
> 
> It's 4k

I think that is big enough that it would be better on the heap,
although libxl is quite flabby in its stack use.

Thanks,
Ian.

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

[PATCH v3 11/31] libxl_qmp: Remove unused yajl_ctx form handler

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 4da84dcf16..1184ca823f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -111,8 +111,6 @@ struct libxl__qmp_handler {
     /* wait_for_id will be used by the synchronous send function */
     int wait_for_id;
 
-    libxl__yajl_ctx *yajl_ctx;
-
     libxl_ctx *ctx;
     uint32_t domid;
 
-- 
Anthony PERARD


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

Re: [PATCH v3 11/31] libxl_qmp: Remove unused yajl_ctx form handler

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 11/31] libxl_qmp: Remove unused yajl_ctx form handler"):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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

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

[PATCH v3 12/31] libxl_json: constify libxl__json_object_to_yajl_gen arguments

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c582894589..12bbfe4a63 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2030,7 +2030,7 @@ _hidden const libxl__json_object *libxl__json_map_get(const char *key,
                                           libxl__json_node_type expected_type);
 _hidden yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc_opt,
                                                    yajl_gen hand,
-                                                   libxl__json_object *param);
+                                                   const libxl__json_object *param);
 _hidden void libxl__json_object_free(libxl__gc *gc_opt,
                                      libxl__json_object *obj);
 
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index dc93a88ef1..b7f9077f0d 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -612,7 +612,7 @@ const libxl__json_object *libxl__json_map_get(const char *key,
 
 yajl_status libxl__json_object_to_yajl_gen(libxl__gc *gc,
                                            yajl_gen hand,
-                                           libxl__json_object *obj)
+                                           const libxl__json_object *obj)
 {
     int idx = 0;
     yajl_status rc;
-- 
Anthony PERARD


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

Re: [PATCH v3 12/31] libxl_json: constify libxl__json_object_to_yajl_gen arguments

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 12/31] libxl_json: constify libxl__json_object_to_yajl_gen arguments"):
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

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

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

[PATCH v3 13/31] libxl_qmp: Separate QMP message generation from qmp_send_prepare

[Anthony PERARD]

This new function qmp_prepare_qmp_cmd() can be reuse later when
introducing a different way to communicate with a QMP server,
libxl__ev_qmp.

Also, add the QMP end of command '\r\n' into the generated string.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 1184ca823f..9f4c3f5c20 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -578,17 +578,17 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
     return rc;
 }
 
-static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
-                              const char *cmd, libxl__json_object *args,
-                              qmp_callback_t callback, void *opaque,
-                              qmp_request_context *context)
+static char *qmp_prepare_qmp_cmd(libxl__gc *gc,
+                                 const char *cmd,
+                                 const libxl__json_object *args,
+                                 int id,
+                                 size_t *len_r)
 {
-    const unsigned char *buf = NULL;
-    char *ret = NULL;
-    libxl_yajl_length len = 0;
+    const unsigned char *buf;
+    libxl_yajl_length len;
     yajl_gen_status s;
     yajl_gen hand;
-    callback_id_pair *elm = NULL;
+    char *ret = NULL;
 
     hand = libxl_yajl_gen_alloc(NULL);
 
@@ -600,7 +600,7 @@ static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
     libxl__yajl_gen_asciiz(hand, "execute");
     libxl__yajl_gen_asciiz(hand, cmd);
     libxl__yajl_gen_asciiz(hand, "id");
-    yajl_gen_integer(hand, ++qmp->last_id_used);
+    yajl_gen_integer(hand, id);
     if (args) {
         libxl__yajl_gen_asciiz(hand, "arguments");
         libxl__json_object_to_yajl_gen(gc, hand, args);
@@ -610,6 +610,36 @@ static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
     s = yajl_gen_get_buf(hand, &buf, &len);
 
     if (s) {
+        goto out;
+    }
+
+    ret = libxl__malloc(NOGC, len + 3);
+    strncpy(ret, (const char *)buf, len + 3);
+    strncpy(ret + len, "\r\n", 3);
+    len += 2;
+
+    if (len_r)
+        *len_r = len;
+
+out:
+    yajl_gen_free(hand);
+    return ret;
+}
+
+static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
+                              const char *cmd, libxl__json_object *args,
+                              qmp_callback_t callback, void *opaque,
+                              qmp_request_context *context,
+                              size_t *len_r)
+{
+    char *buf;
+    callback_id_pair *elm;
+
+    buf = qmp_prepare_qmp_cmd(gc,
+                              cmd, args, ++qmp->last_id_used,
+                              NULL);
+
+    if (!buf) {
         LOGD(ERROR, qmp->domid, "Failed to generate a qmp command");
         goto out;
     }
@@ -625,13 +655,10 @@ static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
     elm->context = context;
     LIBXL_STAILQ_INSERT_TAIL(&qmp->callback_list, elm, next);
 
-    ret = libxl__strndup(gc, (const char*)buf, len);
-
     LOGD(DEBUG, qmp->domid, "next qmp command: '%s'", buf);
 
 out:
-    yajl_gen_free(hand);
-    return ret;
+    return buf;
 }
 
 static int qmp_send(libxl__qmp_handler *qmp,
@@ -643,7 +670,8 @@ static int qmp_send(libxl__qmp_handler *qmp,
     int rc = -1;
     GC_INIT(qmp->ctx);
 
-    buf = qmp_send_prepare(gc, qmp, cmd, args, callback, opaque, context);
+    buf = qmp_send_prepare(gc, qmp, cmd, args, callback, opaque, context,
+                           NULL);
 
     if (buf == NULL) {
         goto out;
@@ -659,12 +687,10 @@ static int qmp_send(libxl__qmp_handler *qmp,
                                 "QMP command", "QMP socket"))
             goto out;
     }
-    if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
-                            "CRLF", "QMP socket"))
-        goto out;
 
     rc = qmp->last_id_used;
 out:
+    free(buf);
     GC_FREE;
     return rc;
 }
-- 
Anthony PERARD


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

Re: [PATCH v3 13/31] libxl_qmp: Separate QMP message generation from qmp_send_prepare

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 13/31] libxl_qmp: Separate QMP message generation from qmp_send_prepare"):
> This new function qmp_prepare_qmp_cmd() can be reuse later when
> introducing a different way to communicate with a QMP server,
> libxl__ev_qmp.
> 
> Also, add the QMP end of command '\r\n' into the generated string.

Would it be terribly inconvenient if this function

> +static char *qmp_prepare_qmp_cmd(libxl__gc *gc,
> +                                 const char *cmd,
> +                                 const libxl__json_object *args,
> +                                 int id,
> +                                 size_t *len_r)
...
> +    ret = libxl__malloc(NOGC, len + 3);

actually used its incoming gc ?  Perhaps it needs a 2nd gc argument ?

I think for future we should be using some appropriate ao gc, or
something, for these buffers.  But, anyway, that is a future
improvement, and not a blocker for this patch.

So:

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

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

Re: [PATCH v3 13/31] libxl_qmp: Separate QMP message generation from qmp_send_prepare

[Anthony PERARD]

On Wed, Jun 27, 2018 at 03:45:33PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 13/31] libxl_qmp: Separate QMP message generation from qmp_send_prepare"):
> > This new function qmp_prepare_qmp_cmd() can be reuse later when
> > introducing a different way to communicate with a QMP server,
> > libxl__ev_qmp.
> > 
> > Also, add the QMP end of command '\r\n' into the generated string.
> 
> Would it be terribly inconvenient if this function
> 
> > +static char *qmp_prepare_qmp_cmd(libxl__gc *gc,
> > +                                 const char *cmd,
> > +                                 const libxl__json_object *args,
> > +                                 int id,
> > +                                 size_t *len_r)
> ...
> > +    ret = libxl__malloc(NOGC, len + 3);
> 
> actually used its incoming gc ?

Yes, because I wanted to keep the allocated buffers until it is actually
sent. But maybe we could provide a different gc, but I'm not sure which
one or how to get it.

That function is going to be called by:
> libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev, ....)
introduce in the patch "libxl_qmp: Introduce libxl__ev_qmp functions"

> Perhaps it needs a 2nd gc argument ?
> 
> I think for future we should be using some appropriate ao gc, or
> something, for these buffers.  But, anyway, that is a future
> improvement, and not a blocker for this patch.

Sure.

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

Thanks,

-- 
Anthony PERARD

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

[PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Anthony PERARD]

This is a first patch to implement libxl__ev_qmp, it only connect to the
QMP socket of QEMU and register a callback that does nothing.

Callback functions will be implemented in following patches.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
TODO:
This would probably needs to have a list in CTX, with state for
different domid.
---
 tools/libxl/libxl.c          |   4 ++
 tools/libxl/libxl_internal.h |   8 +++
 tools/libxl/libxl_qmp.c      | 106 +++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b41ade9fda..b3fb8c1f8b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -162,6 +162,10 @@ int libxl_ctx_free(libxl_ctx *ctx)
     assert(!libxl__ev_fd_isregistered(&ctx->evtchn_efd));
     assert(!libxl__ev_fd_isregistered(&ctx->sigchld_selfpipe_efd));
 
+    libxl__ev_qmp_stop(gc, ctx->qmp_ev);
+    free(ctx->qmp_ev);
+    ctx->qmp_ev = NULL;
+
     /* Now there should be no more events requested from the application: */
 
     assert(LIBXL_LIST_EMPTY(&ctx->efds));
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 12bbfe4a63..d9eebfd98b 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__ev_qmp_state libxl__ev_qmp_state;
 
 typedef struct libxl__domain_create_state libxl__domain_create_state;
 typedef void libxl__domain_create_cb(struct libxl__egc *egc,
@@ -503,6 +504,9 @@ struct libxl__ctx {
     LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
 
     libxl_version_info version_info;
+
+    // FIXME: May need a list, with on state for each domid
+    libxl__ev_qmp_state *qmp_ev;
 };
 
 /*
@@ -4418,6 +4422,10 @@ static inline bool libxl__string_is_default(char **s)
 {
     return *s == NULL;
 }
+
+_hidden libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid);
+/* Allow to disconnect from a QMP server and free ressources */
+_hidden void libxl__ev_qmp_stop(libxl__gc *, libxl__ev_qmp_state *qmp);
 #endif
 
 /*
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9f4c3f5c20..077cac9c8a 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1351,6 +1351,112 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
     return ret;
 }
 
+/* ------------ Implementation of libxl__ev_qmp ---------------- */
+
+struct libxl__ev_qmp_state {
+    libxl__carefd *cfd;
+    libxl__ev_fd efd;
+    uint32_t domid;
+};
+
+static void ev_qmp_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
+                               int fd, short events, short revents)
+{
+}
+
+static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
+{
+    qmp->domid = INVALID_DOMID;
+    qmp->cfd = NULL;
+    libxl__ev_fd_init(&qmp->efd);
+}
+
+libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid)
+{
+    int rc, r;
+    struct sockaddr_un un;
+    const char *qmp_socket_path;
+    libxl__ev_qmp_state *qmp;
+
+    CTX_LOCK;
+
+    if (!CTX->qmp_ev) {
+        qmp = libxl__malloc(NOGC, sizeof(*qmp));
+        CTX->qmp_ev = qmp;
+        libxl__ev_qmp_state_init(qmp);
+    } else {
+        qmp = CTX->qmp_ev;
+    }
+
+    if (libxl__ev_fd_isregistered(&qmp->efd)) {
+        LOG(DEBUG, "reusing connection: %d == %d", domid, qmp->domid);
+        assert(domid == qmp->domid);
+        rc = 0;
+        goto out;
+    }
+
+    qmp->domid = domid;
+
+    qmp_socket_path = GCSPRINTF("%s/qmp-libxl-%d",
+                                libxl__run_dir_path(), domid);
+
+    LOGD(DEBUG, domid, "Starting new QMP event handler");
+    libxl__carefd_begin();
+    qmp->cfd = libxl__carefd_opened(CTX, socket(AF_UNIX, SOCK_STREAM, 0));
+    if (!qmp->cfd) {
+        LOGED(ERROR, domid, "socket() failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    rc = libxl_fd_set_nonblock(CTX, libxl__carefd_fd(qmp->cfd), 1);
+    if (rc)
+        goto out;
+
+    if (sizeof(un.sun_path) <= strlen(qmp_socket_path)) {
+        rc = -1;
+        goto out;
+    }
+    memset(&un, 0, sizeof(un));
+    un.sun_family = AF_UNIX;
+    assert(strlen(qmp_socket_path) <= sizeof(un.sun_path));
+    strncpy(un.sun_path, qmp_socket_path, sizeof(un.sun_path));
+
+    r = connect(libxl__carefd_fd(qmp->cfd),
+                (struct sockaddr *) &un, sizeof(un));
+    if (r) {
+        LOGED(ERROR, domid, "Failed to connect to QMP socket %s",
+              qmp_socket_path);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = libxl__ev_fd_register(gc, &qmp->efd,
+                               ev_qmp_fd_callback,
+                               libxl__carefd_fd(qmp->cfd),
+                               POLLIN);
+
+out:
+    if (rc)
+        libxl__ev_qmp_stop(gc, qmp);
+    CTX_UNLOCK;
+    return qmp;
+}
+
+void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
+{
+    if (!qmp)
+        return;
+
+    CTX_LOCK;
+
+    LOGD(DEBUG, qmp->domid, "Stopping QMP handler");
+
+    libxl__ev_fd_deregister(gc, &qmp->efd);
+    libxl__carefd_close(qmp->cfd);
+    qmp->cfd = NULL;
+
+    CTX_UNLOCK;
+}
 /*
  * Local variables:
  * mode: C
-- 
Anthony PERARD


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

Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"):
> This is a first patch to implement libxl__ev_qmp, it only connect to the
> QMP socket of QEMU and register a callback that does nothing.
...
> @@ -503,6 +504,9 @@ struct libxl__ctx {
>      LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
>  
>      libxl_version_info version_info;
> +
> +    // FIXME: May need a list, with on state for each domid
> +    libxl__ev_qmp_state *qmp_ev;

My thought is that the lifetime of this thing should probably be in
each relevant ao.

Is it too inconvenient to reconnect to qmp for every libxl operation ?
If it is then this needs to be a cache, a bit like libxl__poller but
different.  But that can be handled inside what you are calling
_ev_qmp_start.

Also, I think if you are going to have a libxl__ev_qmp it needs to be
just like all the other libxl__ev_ things.  It's not clear to me that
QMP is similar enough.

Do you actually need an explicit "start" or "connect" operation ?
I think in any case the "send a qmp command" operations should
probably connect automatically.

So something like this:

   /* libxl__qmp_state has the following states:
    *   Undefined
    *   Disconnected
    *   Connected
    */

   void libxl__qmp_init(ibxl__qmp_state *qmp); /* U -> D */

   int libxl__qmp_connect(libxl__gc *gc, uint32_t domid,
                          libxl__qmp_state *qmp_upd); /* [UC] -> C */

   int libxl__qmp_dispose(libxl__qmp_state *qmp_upd); /* [DC] -> D */

   int libxl__qmp_command( lots of parameters incl callback ); /* [DC] */

> +_hidden libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid);

Line length.

Also, this interface does not support returning a proper error status.

> +libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid)
> +{
> +    int rc, r;
> +    struct sockaddr_un un;
> +    const char *qmp_socket_path;
> +    libxl__ev_qmp_state *qmp;
...
> +out:
> +    if (rc)
> +        libxl__ev_qmp_stop(gc, qmp);
> +    CTX_UNLOCK;
> +    return qmp;

I think the error handling is messed up here.  If this fails, you will
stop the qmp and then return it anyway.

Ian.

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

Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Anthony PERARD]

On Wed, Jun 27, 2018 at 04:07:52PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"):
> > This is a first patch to implement libxl__ev_qmp, it only connect to the
> > QMP socket of QEMU and register a callback that does nothing.
> ...
> > @@ -503,6 +504,9 @@ struct libxl__ctx {
> >      LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
> >  
> >      libxl_version_info version_info;
> > +
> > +    // FIXME: May need a list, with on state for each domid
> > +    libxl__ev_qmp_state *qmp_ev;
> 
> My thought is that the lifetime of this thing should probably be in
> each relevant ao.

That sound plausible, I can try to move this state to libxl__ao. I just
need to find out what's happen on connect(3) when something else is
allready connected to QEMU.

> Is it too inconvenient to reconnect to qmp for every libxl operation ?
> If it is then this needs to be a cache, a bit like libxl__poller but
> different.  But that can be handled inside what you are calling
> _ev_qmp_start.
> 
> Also, I think if you are going to have a libxl__ev_qmp it needs to be
> just like all the other libxl__ev_ things.  It's not clear to me that
> QMP is similar enough.
> 
> Do you actually need an explicit "start" or "connect" operation ?
> I think in any case the "send a qmp command" operations should
> probably connect automatically.

That is what I have done. The libxl__ev_qmp_start() should be static, it
is only use from within libxl_qmp.c. But I've put it in libxl_interal.h
just to allow this patch to build.

The _stop is normally not needed, to be called. It is automatically
called as soon as there is no more QMP command in flight (all command
have receveid a response).

> So something like this:
> 
>    /* libxl__qmp_state has the following states:
>     *   Undefined
>     *   Disconnected
>     *   Connected
>     */
> 
>    void libxl__qmp_init(ibxl__qmp_state *qmp); /* U -> D */
> 
>    int libxl__qmp_connect(libxl__gc *gc, uint32_t domid,
>                           libxl__qmp_state *qmp_upd); /* [UC] -> C */
> 
>    int libxl__qmp_dispose(libxl__qmp_state *qmp_upd); /* [DC] -> D */
> 
>    int libxl__qmp_command( lots of parameters incl callback ); /* [DC] */

So what the interface looks like at the end of the series is:

void libxl__ev_qmp_init(libxl__ev_qmp *ev);
int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
                           libxl__ev_qmp_callback *,
                           uint32_t domid,
                           const char *cmd, libxl__json_object *args);
void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev);
int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev);

The libxl__qmp_state is not exposed. The _register() will attempt to
find the current _state and use it, or create a new one (connect).

The libxl__ev_qmp_stop() call in libxl_ctx_free() is only there in case
something went wrong, it should not be needed.


I'll try to write better documentation about the possible states of both
libxl__ev_qmp_state and libxl__ev_qmp, and how they relate to each
other.

BTW, I think qmp is kind of similair to libxl__ev_xswatch, which would
_ev_fd_register(xenstore_fd) on the first path to watch, and _deregister
once there is no more watches. The one more thing that qmp need to do is
open the socket and close it.

-- 
Anthony PERARD

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

Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"):
> So what the interface looks like at the end of the series is:
> 
> void libxl__ev_qmp_init(libxl__ev_qmp *ev);
> int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
>                            libxl__ev_qmp_callback *,
>                            uint32_t domid,
>                            const char *cmd, libxl__json_object *args);
> void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev);
> int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev);
> 
> The libxl__qmp_state is not exposed. The _register() will attempt to
> find the current _state and use it, or create a new one (connect).

Ah, I see.

Err, the global (ctx) state is definitely wrong then.  Multiple
operations might be in flight at once with the same ctx[1], so libxl
might need to be connected to multiple qemus (or connected to the same
qemu multiple times, if you don't want to deal with demultiplexing).

If you intend for each _ev to handle only one command at a time, and
map commands 1:1 to connections, then the connection state needs to be
in your ev structure.

TBH the API you quote about does seem very similar to the other
libxl_ev_FOO but it is very counterintuitive that "register" is the
function to send a qmp command.  I think some better names would help,
even if it means they are less regular.

[1] The ctx is not shared between threads, but it is shared between
independent aos etc.

> I'll try to write better documentation about the possible states of both
> libxl__ev_qmp_state and libxl__ev_qmp, and how they relate to each
> other.

That would really help, although I think what I am saying above
may imply that they need to be unified.

> BTW, I think qmp is kind of similair to libxl__ev_xswatch, which would
> _ev_fd_register(xenstore_fd) on the first path to watch, and _deregister
> once there is no more watches. The one more thing that qmp need to do is
> open the socket and close it.

If you are sharing the qmp connection between multiple concurrent qmp
commands then I think you need a data structure in the ctx which can
find an existing connection given a domid.

Ian.

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

Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP

[Anthony PERARD]

On Thu, Jun 28, 2018 at 12:44:28PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v3 14/31] libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"):
> > So what the interface looks like at the end of the series is:
> > 
> > void libxl__ev_qmp_init(libxl__ev_qmp *ev);
> > int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
> >                            libxl__ev_qmp_callback *,
> >                            uint32_t domid,
> >                            const char *cmd, libxl__json_object *args);
> > void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev);
> > int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev);
> > 
> > The libxl__qmp_state is not exposed. The _register() will attempt to
> > find the current _state and use it, or create a new one (connect).
> 
> Ah, I see.
> 
> Err, the global (ctx) state is definitely wrong then.  Multiple
> operations might be in flight at once with the same ctx[1], so libxl
> might need to be connected to multiple qemus (or connected to the same
> qemu multiple times, if you don't want to deal with demultiplexing).

Demultiplexing is already been dealt with, even in the current code, so
that's not an issue.

I haven't dealt with the ability to connect to multiple qemu (of
different domain) but I know that was an issue, there a TODO and a FIXME
for it.

But, the ability to connect to the same qemu multiple times is an issue,
qemu only accept one connection at a time (per socket).

> If you intend for each _ev to handle only one command at a time, and
> map commands 1:1 to connections, then the connection state needs to be
> in your ev structure.

I did intend to have one _ev handle one command, but we need to be able
to send multiple command within the same connection. This is because of
the way qemu handle received fds.

For example, to open a new cdrom, this action needs to happen:
-> connect, to qmp
-> send "add-fd"
<- receive back a fdset id.
-> send "blockdev-change-medium" file=$fdset (insert a new cdrom)
-> disconnect

That will work. But if we disconnect after "add-fd" (and before
change-medium), then qemu will close the fd, because it will be unused.


> TBH the API you quote about does seem very similar to the other
> libxl_ev_FOO but it is very counterintuitive that "register" is the
> function to send a qmp command.  I think some better names would help,
> even if it means they are less regular.

Maybe libxl__ev_qmp_execute() instead. It is true that my _register
behave a bit differently to other _registers, the callback will only
ever be called only once, and should call _deregister.

> [1] The ctx is not shared between threads, but it is shared between
> independent aos etc.
> 
> > I'll try to write better documentation about the possible states of both
> > libxl__ev_qmp_state and libxl__ev_qmp, and how they relate to each
> > other.
> 
> That would really help, although I think what I am saying above
> may imply that they need to be unified.
> 
> > BTW, I think qmp is kind of similair to libxl__ev_xswatch, which would
> > _ev_fd_register(xenstore_fd) on the first path to watch, and _deregister
> > once there is no more watches. The one more thing that qmp need to do is
> > open the socket and close it.
> 
> If you are sharing the qmp connection between multiple concurrent qmp
> commands then I think you need a data structure in the ctx which can
> find an existing connection given a domid.
> 
> Ian.

-- 
Anthony PERARD

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

[PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data

[Anthony PERARD]

First step into taking care of the input from QEMU's QMP socket. For
now, we read data and store them in buffers.

Parsing of the data will be done in the following patches.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 077cac9c8a..48dc376307 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -75,6 +75,12 @@
 #  define DEBUG_REPORT_RECEIVED(dom, buf, len) ((void)0)
 #endif
 
+#ifdef DEBUG_QMP_CLIENT
+#  define LOG_QMP(f, ...) LOGD(DEBUG, qmp->domid, f, ##__VA_ARGS__)
+#else
+#  define LOG_QMP(f, ...)
+#endif
+
 /*
  * QMP types & constant
  */
@@ -1353,15 +1359,115 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
 
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
+typedef struct libxl__qmp_rx_buf libxl__qmp_rx_buf;
+struct libxl__qmp_rx_buf {
+    LIBXL_TAILQ_ENTRY(libxl__qmp_rx_buf) entry;
+    /* How much data there is in buf */
+    int used;
+    /* How much have been parsed */
+    size_t consumed;
+    char buf[QMP_RECEIVE_BUFFER_SIZE];
+};
+
 struct libxl__ev_qmp_state {
     libxl__carefd *cfd;
     libxl__ev_fd efd;
     uint32_t domid;
+
+    LIBXL_TAILQ_HEAD(libxl__qmp_bufs, libxl__qmp_rx_buf) bufs;
 };
 
+
+static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
+                                    int fd)
+{
+    EGC_GC;
+    ssize_t r;
+    libxl__qmp_rx_buf *buf;
+
+    /* Check if last buffer still have space, or alloc a new one */
+    buf = LIBXL_TAILQ_LAST(&qmp->bufs, libxl__qmp_bufs);
+    if (buf) {
+        /* The -1 is because there is always space for a NUL character */
+        if (buf->used == sizeof(buf->buf) - 1) {
+            buf = NULL;
+        }
+    }
+    if (!buf) {
+        buf = libxl__malloc(NOGC, sizeof(*buf));
+        buf->used = 0;
+        buf->consumed = 0;
+        LIBXL_TAILQ_INSERT_TAIL(&qmp->bufs, buf, entry);
+    }
+
+    for (;;) {
+        /* The -1 is because there is always space for a NUL character */
+        r = read(fd, buf->buf + buf->used, sizeof(buf->buf) - buf->used - 1);
+        if (r < 0) {
+            if (errno == EINTR) continue;
+            assert(errno);
+            if (errno == EWOULDBLOCK) {
+                return 0;
+            }
+            LOGED(ERROR, qmp->domid, "error reading QMP socket");
+            return ERROR_FAIL;
+        }
+        break;
+    }
+
+    if (r == 0) {
+        LOGD(ERROR, qmp->domid, "No data read on QMP socket");
+        return 0;
+    }
+
+    LOG_QMP("received %ldB: '%.*s'", r, (int)r, buf->buf + buf->used);
+
+    buf->used += r;
+    assert(buf->used < sizeof(buf->buf));
+
+    return 0;
+}
+
+/* When the QMP client reach the conclusion that the QMP connection doesn't
+ * work anymore, this function can be called to propagate the error to every
+ * callback registered. And stop the client. */
+static void ev_qmp_callback_error(libxl__egc *egc, libxl__ev_qmp_state *qmp)
+{
+    EGC_GC;
+
+    LOGD(ERROR, qmp->domid, "Error happend with the QMP connection to QEMU");
+    libxl__ev_qmp_stop(gc, qmp);
+}
+
 static void ev_qmp_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
                                int fd, short events, short revents)
 {
+    EGC_GC;
+    int rc;
+
+    libxl__ev_qmp_state *qmp = CONTAINER_OF(ev_fd, *qmp, efd);
+
+    if (revents & (POLLHUP)) {
+        LOGD(DEBUG, qmp->domid, "received POLLHUP from QMP socket");
+        ev_qmp_callback_error(egc, qmp);
+        return;
+    }
+    if (revents & ~(POLLIN|POLLOUT)) {
+        LOGD(ERROR, qmp->domid,
+             "unexpected poll event 0x%x on QMP socket (expected POLLIN "
+             "and/or POLLOUT)",
+            revents);
+        ev_qmp_callback_error(egc, qmp);
+        return;
+    }
+
+    if (revents & POLLIN) {
+        rc = ev_qmp_callback_readable(egc, qmp, fd);
+        if (rc) {
+            ev_qmp_callback_error(egc, qmp);
+            return;
+        }
+    }
 }
 
 static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
@@ -1369,6 +1475,7 @@ static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
     qmp->domid = INVALID_DOMID;
     qmp->cfd = NULL;
     libxl__ev_fd_init(&qmp->efd);
+    LIBXL_TAILQ_INIT(&qmp->bufs);
 }
 
 libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid)
@@ -1444,6 +1551,8 @@ out:
 
 void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 {
+    libxl__qmp_rx_buf *buf, *tbuf;
+
     if (!qmp)
         return;
 
@@ -1451,6 +1560,10 @@ void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 
     LOGD(DEBUG, qmp->domid, "Stopping QMP handler");
 
+    LIBXL_TAILQ_FOREACH_SAFE(buf, &qmp->bufs, entry, tbuf)
+        free(buf);
+    LIBXL_TAILQ_INIT(&qmp->bufs);
+
     libxl__ev_fd_deregister(gc, &qmp->efd);
     libxl__carefd_close(qmp->cfd);
     qmp->cfd = NULL;
-- 
Anthony PERARD


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

Re: [PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data"):
> First step into taking care of the input from QEMU's QMP socket. For
> now, we read data and store them in buffers.

How big is this data ?  Is all this business with a linked list of
buffers really necessary ?

(Maybe I should be reading this as a final tree state rather than as
individual patches.)

Ian.

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

Re: [PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data

[Anthony PERARD]

On Wed, Jun 27, 2018 at 04:10:18PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data"):
> > First step into taking care of the input from QEMU's QMP socket. For
> > now, we read data and store them in buffers.
> 
> How big is this data ?  Is all this business with a linked list of
> buffers really necessary ?

The alternative is realloc. I don't know how big the data is going to
be. I don't want to speculate.

But right now, QMP_RECEIVE_BUFFER_SIZE is set to 4k, I don't think we
would need more than that, but I think the code need to be prepared to
receive more.

-- 
Anthony PERARD

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

Re: [PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v3 15/31] libxl_qmp_ev: Implement fd callback and read data"):
> On Wed, Jun 27, 2018 at 04:10:18PM +0100, Ian Jackson wrote:
> > How big is this data ?  Is all this business with a linked list of
> > buffers really necessary ?
> 
> The alternative is realloc. I don't know how big the data is going to
> be. I don't want to speculate.

Yes, but there is not anything very wrong with realloc.

> But right now, QMP_RECEIVE_BUFFER_SIZE is set to 4k, I don't think we
> would need more than that, but I think the code need to be prepared to
> receive more.

All of this buffer queue complication is a performance optimisation to
avoid hypothetical reallocs, then :-) ?

Ian.

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

[PATCH v3 16/31] libxl_json: Allow partial parsing

[Anthony PERARD]

This set of function allow to parse a JSON string that is spread accross
different memory location.

This is usefull when a JSON string is received from a remote process,
and in order to avoid using realloc, the message is recorded in multiple
buffers.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |   6 +++
 tools/libxl/libxl_json.c     | 100 +++++++++++++++++++++++++++--------
 2 files changed, 84 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d9eebfd98b..c87712c83a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2039,6 +2039,12 @@ _hidden void libxl__json_object_free(libxl__gc *gc_opt,
                                      libxl__json_object *obj);
 
 _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
+/* allow to parse a json string store in multiple buffers */
+_hidden libxl__yajl_ctx *libxl__json_parse_alloc(libxl__gc *gc);
+_hidden int libxl__json_parse_partial(libxl__gc *gc, libxl__yajl_ctx *yajl_ctx,
+                                      const char *s, size_t len);
+_hidden libxl__json_object *libxl__json_complete_parse(libxl__gc *gc,
+                                                   libxl__yajl_ctx *yajl_ctx);
 
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index b7f9077f0d..3727af34d8 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -912,47 +912,103 @@ static void yajl_ctx_free(libxl__yajl_ctx *yajl_ctx)
     DEBUG_GEN_FREE(yajl_ctx);
 }
 
-libxl__json_object *libxl__json_parse(libxl__gc *gc, const char *s)
+/*
+ * Only to use with:
+ * libxl__json_parse_partial
+ * libxl__json_complete_parse
+ *
+ * gc should be the same accross all functions.
+ * NOGC is not allowed unless called from libxl__json_parse()
+ */
+libxl__yajl_ctx *libxl__json_parse_alloc(libxl__gc *gc)
 {
-    yajl_status status;
-    libxl__yajl_ctx yajl_ctx;
-    libxl__json_object *o = NULL;
-    unsigned char *str = NULL;
+    libxl__yajl_ctx *yajl_ctx;
 
-    memset(&yajl_ctx, 0, sizeof (yajl_ctx));
-    yajl_ctx.gc = gc;
+    GCNEW(yajl_ctx);
 
-    DEBUG_GEN_ALLOC(&yajl_ctx);
+    yajl_ctx->gc = gc;
 
-    if (yajl_ctx.hand == NULL) {
-        yajl_ctx.hand = libxl__yajl_alloc(&callbacks, NULL, &yajl_ctx);
-    }
-    status = yajl_parse(yajl_ctx.hand, (const unsigned char *)s, strlen(s));
+    DEBUG_GEN_ALLOC(yajl_ctx);
+
+    yajl_ctx->hand = libxl__yajl_alloc(&callbacks, NULL, yajl_ctx);
+
+    return yajl_ctx;
+}
+
+static void json_parse_error(libxl__gc *gc, libxl__yajl_ctx *yajl_ctx,
+                             const char *s, size_t len)
+{
+    unsigned char *str;
+    str = yajl_get_error(yajl_ctx->hand, 1, (const unsigned char*)s, len);
+    LOGE(ERROR, "yajl error: %s", str);
+    yajl_free_error(yajl_ctx->hand, str);
+    yajl_ctx_free(yajl_ctx);
+}
+
+int libxl__json_parse_partial(libxl__gc *gc, libxl__yajl_ctx *yajl_ctx,
+                              const char *s, size_t len)
+{
+    yajl_status status;
+
+    assert(gc == yajl_ctx->gc);
+
+    status = yajl_parse(yajl_ctx->hand, (const unsigned char *)s, strlen(s));
     if (status != yajl_status_ok)
         goto out;
 
-    status = yajl_complete_parse(yajl_ctx.hand);
+    return 0;
+
+out:
+    json_parse_error(gc, yajl_ctx, s, len);
+    return ERROR_FAIL;
+}
+
+libxl__json_object *libxl__json_complete_parse(libxl__gc *gc,
+                                               libxl__yajl_ctx *yajl_ctx)
+{
+    yajl_status status;
+    libxl__json_object *o;
+
+    assert(gc == yajl_ctx->gc);
+
+    status = yajl_complete_parse(yajl_ctx->hand);
     if (status != yajl_status_ok)
         goto out;
 
-    o = yajl_ctx.head;
+    o = yajl_ctx->head;
 
-    DEBUG_GEN_REPORT(&yajl_ctx);
+    DEBUG_GEN_REPORT(yajl_ctx);
 
-    yajl_ctx.head = NULL;
+    yajl_ctx->head = NULL;
 
-    yajl_ctx_free(&yajl_ctx);
+    yajl_ctx_free(yajl_ctx);
     return o;
 
 out:
-    str = yajl_get_error(yajl_ctx.hand, 1, (const unsigned char*)s, strlen(s));
-
-    LIBXL__LOG(libxl__gc_owner(gc), LIBXL__LOG_ERROR, "yajl error: %s", str);
-    yajl_free_error(yajl_ctx.hand, str);
-    yajl_ctx_free(&yajl_ctx);
+    json_parse_error(gc, yajl_ctx, NULL, 0);
     return NULL;
 }
 
+libxl__json_object *libxl__json_parse(libxl__gc *gc, const char *s)
+{
+    libxl__yajl_ctx *yajl_ctx;
+    int rc;
+    libxl__json_object *o = NULL;
+
+    yajl_ctx = libxl__json_parse_alloc(gc);
+
+    rc = libxl__json_parse_partial(gc, yajl_ctx, s, strlen(s));
+    if (rc)
+        goto out;
+
+    o = libxl__json_complete_parse(gc, yajl_ctx);
+
+out:
+    if (!libxl__gc_is_real(gc))
+        free(yajl_ctx);
+    return o;
+}
+
 static const char *yajl_gen_status_to_string(yajl_gen_status s)
 {
         switch (s) {
-- 
Anthony PERARD


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

[PATCH v3 17/31] libxl_json: Enable yajl_allow_trailing_garbage

[Anthony PERARD]

This allow to parse a string that is not NUL-terminated. With that
options disabled, YAJL v2 would look ahead on completion to find out if
there is more to parse.

YAJL v1 doesn't have this behavior.

Any function function that allocate a yajl_handle via this function
either parse a NUL-terminated string, or do provide proper length. So
change the default and allow garbage (like a different JSON document)
after the end of the data to parse.

This is importand for the QMP client, as there could be more than one
message to parse, and YAJL would consider the next message to be
garbage and throw an error.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_json.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_json.h b/tools/libxl/libxl_json.h
index af26e7885d..260783bfde 100644
--- a/tools/libxl/libxl_json.h
+++ b/tools/libxl/libxl_json.h
@@ -50,7 +50,10 @@ static inline yajl_handle libxl__yajl_alloc(const yajl_callbacks *callbacks,
                                             yajl_alloc_funcs *allocFuncs,
                                             void *ctx)
 {
-    return yajl_alloc(callbacks, allocFuncs, ctx);
+    yajl_handle hand = yajl_alloc(callbacks, allocFuncs, ctx);
+    if (hand)
+        yajl_config(hand, yajl_allow_trailing_garbage, 1);
+    return hand;
 }
 
 static inline yajl_gen libxl_yajl_gen_alloc(const yajl_alloc_funcs *allocFuncs)
-- 
Anthony PERARD


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

[PATCH v3 18/31] libxl_json: libxl__json_object_to_json

[Anthony PERARD]

Allow to generate a JSON string from a libxl__json_object,
usefull for debugging.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c87712c83a..9271701246 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2046,6 +2046,9 @@ _hidden int libxl__json_parse_partial(libxl__gc *gc, libxl__yajl_ctx *yajl_ctx,
 _hidden libxl__json_object *libxl__json_complete_parse(libxl__gc *gc,
                                                    libxl__yajl_ctx *yajl_ctx);
 
+_hidden char *libxl__json_object_to_json(libxl__gc *gc,
+                                         const libxl__json_object *args);
+
   /* Based on /local/domain/$domid/dm-version xenstore key
    * default is qemu xen traditional */
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index 3727af34d8..3322ea12b5 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -1073,6 +1073,42 @@ out:
     return ret;
 }
 
+char *libxl__json_object_to_json(libxl__gc *gc,
+                                 const libxl__json_object *args)
+{
+    const unsigned char *buf;
+    libxl_yajl_length len;
+    yajl_gen_status s;
+    yajl_gen hand;
+    char *ret = NULL;
+    int rc;
+
+    if (!args)
+        return NULL;
+
+    hand = libxl_yajl_gen_alloc(NULL);
+
+    if (!hand) {
+        return NULL;
+    }
+
+    rc = libxl__json_object_to_yajl_gen(gc, hand, args);
+    if (rc)
+        goto out;
+
+    s = yajl_gen_get_buf(hand, &buf, &len);
+
+    if (s) {
+        goto out;
+    }
+
+    ret = libxl__strndup(gc, (const char *)buf, len);
+
+out:
+    yajl_gen_free(hand);
+    return ret;
+}
+
 yajl_gen_status libxl__uint64_gen_json(yajl_gen hand, uint64_t val)
 {
     char *num;
-- 
Anthony PERARD


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

Re: [PATCH v3 18/31] libxl_json: libxl__json_object_to_json

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 18/31] libxl_json: libxl__json_object_to_json"):
> Allow to generate a JSON string from a libxl__json_object,
> usefull for debugging.

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

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

[PATCH v3 19/31] libxl_qmp_ev: Parse JSON input from QMP

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 48dc376307..e4441f76f4 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1383,7 +1383,9 @@ static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
 {
     EGC_GC;
     ssize_t r;
+    char *end = NULL;
     libxl__qmp_rx_buf *buf;
+    int rc;
 
     /* Check if last buffer still have space, or alloc a new one */
     buf = LIBXL_TAILQ_LAST(&qmp->bufs, libxl__qmp_bufs);
@@ -1425,6 +1427,102 @@ static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
     buf->used += r;
     assert(buf->used < sizeof(buf->buf));
 
+    /* workaround strstr limitation */
+    buf->buf[buf->used] = '\0';
+
+    /*
+     * Search for the end of a QMP message: "\r\n".
+     * - First check if those two chr were received accross 2 read.
+     * - Then, look for them within the newly read data.
+     *
+     * end: This point to the address right after \r\n
+     */
+    if (buf->buf[buf->used - r] == '\n') {
+        /* First new chr read is \n, look if the previous one is \r. */
+
+        if (buf->used - r == 0) {
+            libxl__qmp_rx_buf *prev_buf;
+
+            prev_buf = LIBXL_TAILQ_PREV(buf, libxl__qmp_bufs, entry);
+            if (prev_buf &&
+                prev_buf->buf[prev_buf->used - 1] == '\r') {
+                end = buf->buf + 1;
+            }
+        } else if (buf->buf[buf->used - r - 1] == '\r') {
+            end = buf->buf + buf->used - r - 1;
+            end += 2;
+        }
+    }
+    if (!end) {
+        end = strstr(buf->buf + buf->used - r, "\r\n");
+        if (end)
+            end += 2;
+    }
+
+    while (end) {
+        libxl__json_object *o;
+        libxl__yajl_ctx *yajl_ctx;
+        libxl__qmp_rx_buf *parse_buf, *tbuf;
+
+        yajl_ctx = libxl__json_parse_alloc(gc);
+
+        LIBXL_TAILQ_FOREACH_SAFE(parse_buf, &qmp->bufs, entry, tbuf) {
+            size_t len;
+            char *s;
+
+            /* Start parsing from s */
+            s = parse_buf->buf + parse_buf->consumed;
+            /* Findout how much can be parsed */
+            if (buf == parse_buf) {
+                /* this is the last buffer to parse, stop at end */
+                len = end - parse_buf->buf - parse_buf->consumed;
+            } else {
+                /* parse all the buffer */
+                len = parse_buf->used - parse_buf->consumed;
+            }
+
+            LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
+
+            rc = libxl__json_parse_partial(gc, yajl_ctx, s, len);
+            if (rc)
+                break;
+
+            parse_buf->consumed += len;
+
+            if (parse_buf->consumed >= parse_buf->used) {
+                LIBXL_TAILQ_REMOVE(&qmp->bufs, parse_buf, entry);
+                free(parse_buf);
+                if (buf == parse_buf) {
+                    buf = NULL;
+                    break;
+                }
+            }
+
+            /* Last buffer to parse, will call complete */
+            if (buf == parse_buf)
+                break;
+        }
+
+        if (rc)
+            return rc;
+
+        o = libxl__json_complete_parse(gc, yajl_ctx);
+
+        if (!o) {
+            LOGD(ERROR, qmp->domid, "Parse error");
+            return ERROR_FAIL;
+        }
+
+        LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, o));
+
+        /* check if there is another message received at the same time */
+        if (buf) {
+            end = strstr(buf->buf + buf->consumed, "\r\n");
+            if (end)
+                end += 2;
+        } else
+            end = NULL;
+    }
     return 0;
 }
 
-- 
Anthony PERARD


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

[PATCH v3 20/31] libxl_qmp: Introduce libxl__ev_qmp functions

[Anthony PERARD]

Calling libxl__ev_qmp_register() will prepare a command to be sent to
QEMU and stash it in a queue to be sent later.

The actual sent will be done in a separate patch.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9271701246..16533f651e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -202,6 +202,8 @@ 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__ev_qmp_state libxl__ev_qmp_state;
+typedef struct libxl__json_object libxl__json_object;
+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,
@@ -357,6 +359,39 @@ struct libxl__ev_child {
     LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
 };
 
+/*
+ * libxl__ev_qmp
+ */
+
+typedef struct libxl__ev_qmp libxl__ev_qmp;
+/* response: QMP response on success, or NULL on error.
+ * error_class: NONE on success, otherwise QMP error class or libxl error */
+typedef void libxl__ev_qmp_callback(libxl__egc *egc, libxl__ev_qmp *ev,
+                                    const libxl__json_object *response,
+                                    libxl__qmp_error_class error_class);
+struct libxl__ev_qmp {
+    /* read-only once registered */
+    uint32_t domid;
+    libxl__ev_qmp_callback *callback;
+    /* If !NULL, this file descriptor will be sent to the QMP server,
+     * and closed once sent. */
+    libxl__carefd *efd;
+
+    /* private */
+
+    /* id == -1: initial state or response already received and callback called.
+     * id > 0: id used to send a command to qemu. */
+    int id;
+    LIBXL_TAILQ_ENTRY(libxl__ev_qmp) entry;
+};
+
+_hidden void libxl__ev_qmp_init(libxl__ev_qmp *ev);
+_hidden int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
+                                   libxl__ev_qmp_callback *,
+                                   uint32_t domid,
+                                   const char *cmd, libxl__json_object *args);
+_hidden void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev);
+_hidden int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev);
 
 /*
  * evgen structures, which are the state we use for generating
@@ -1905,7 +1940,7 @@ typedef enum {
     JSON_ANY     = 255 /* this is a mask of all values above, adjust as needed */
 } libxl__json_node_type;
 
-typedef struct libxl__json_object {
+struct libxl__json_object {
     libxl__json_node_type type;
     union {
         bool b;
@@ -1918,7 +1953,7 @@ typedef struct libxl__json_object {
         flexarray_t *map;
     } u;
     struct libxl__json_object *parent;
-} libxl__json_object;
+};
 
 typedef int (*libxl__json_parse_callback)(libxl__gc *gc,
                                           libxl__json_object *o,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index e4441f76f4..9b5eb8fd35 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1369,15 +1369,57 @@ struct libxl__qmp_rx_buf {
     char buf[QMP_RECEIVE_BUFFER_SIZE];
 };
 
+typedef struct libxl__qmp_tx_buf libxl__qmp_tx_buf;
+struct libxl__qmp_tx_buf {
+    LIBXL_TAILQ_ENTRY(libxl__qmp_tx_buf) entry;
+    size_t len;
+    char *buf;
+    /* File descriptor to send along the command */
+    libxl__carefd *efd;
+};
+
 struct libxl__ev_qmp_state {
     libxl__carefd *cfd;
     libxl__ev_fd efd;
     uint32_t domid;
 
     LIBXL_TAILQ_HEAD(libxl__qmp_bufs, libxl__qmp_rx_buf) bufs;
+
+    unsigned int last_id_used;
+    /* Indicate that QEMU is ready to respond to command. */
+    bool ready;
+    LIBXL_TAILQ_HEAD(libxl__qmp_tx_bufs, libxl__qmp_tx_buf) tx_buf;
+
+    LIBXL_TAILQ_HEAD(libxl__ev_qmps, libxl__ev_qmp) qmp_events;
 };
 
 
+/* Prepare a QMP command to be sent */
+static int ev_qmp_queue_command(libxl__gc *gc,
+                                libxl__ev_qmp_state *qmp,
+                                const char *cmd,
+                                const libxl__json_object *args,
+                                libxl__carefd *efd)
+{
+    char *buf = NULL;
+    size_t len;
+    libxl__qmp_tx_buf *out;
+
+    buf = qmp_prepare_qmp_cmd(gc,
+                              cmd, args, ++qmp->last_id_used,
+                              &len);
+    if (!buf)
+        return ERROR_FAIL;
+
+    out = libxl__malloc(NOGC, sizeof(*out));
+    out->buf = buf;
+    out->len = len;
+    out->efd = efd;
+    LIBXL_TAILQ_INSERT_TAIL(&qmp->tx_buf, out, entry);
+
+    return 0;
+}
+
 static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
                                     int fd)
 {
@@ -1532,8 +1574,17 @@ static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
 static void ev_qmp_callback_error(libxl__egc *egc, libxl__ev_qmp_state *qmp)
 {
     EGC_GC;
+    libxl__ev_qmp *ev, *tev;
 
     LOGD(ERROR, qmp->domid, "Error happend with the QMP connection to QEMU");
+
+    LIBXL_TAILQ_FOREACH_SAFE(ev, &qmp->qmp_events, entry, tev) {
+        if (ev->id == -1)
+            continue;
+        /* Call every callback with error state. */
+        ev->id = -1;
+        ev->callback(egc, ev, NULL, LIBXL__QMP_ERROR_CLASS_LIBXL_ERROR);
+    }
     libxl__ev_qmp_stop(gc, qmp);
 }
 
@@ -1574,6 +1625,10 @@ static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
     qmp->cfd = NULL;
     libxl__ev_fd_init(&qmp->efd);
     LIBXL_TAILQ_INIT(&qmp->bufs);
+    qmp->last_id_used = 0;
+    qmp->ready = false;
+    LIBXL_TAILQ_INIT(&qmp->tx_buf);
+    LIBXL_TAILQ_INIT(&qmp->qmp_events);
 }
 
 libxl__ev_qmp_state *libxl__ev_qmp_start(libxl__gc *gc, uint32_t domid)
@@ -1650,6 +1705,8 @@ out:
 void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 {
     libxl__qmp_rx_buf *buf, *tbuf;
+    libxl__qmp_tx_buf *tx_buf, *tx_tbuf;
+    libxl__ev_qmp *qmp_ev, *tqmp_ev;
 
     if (!qmp)
         return;
@@ -1661,6 +1718,19 @@ void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
     LIBXL_TAILQ_FOREACH_SAFE(buf, &qmp->bufs, entry, tbuf)
         free(buf);
     LIBXL_TAILQ_INIT(&qmp->bufs);
+    LIBXL_TAILQ_FOREACH_SAFE(tx_buf, &qmp->tx_buf, entry, tx_tbuf) {
+        free(tx_buf->buf);
+        free(tx_buf);
+    }
+    LIBXL_TAILQ_INIT(&qmp->tx_buf);
+
+    LIBXL_TAILQ_FOREACH_SAFE(qmp_ev, &qmp->qmp_events, entry, tqmp_ev) {
+        LOGD(ERROR, qmp->domid, "Event left %p", qmp_ev);
+        libxl__ev_qmp_deregister(gc, qmp_ev);
+    }
+    LIBXL_TAILQ_INIT(&qmp->qmp_events);
+
+    qmp->ready = false;
 
     libxl__ev_fd_deregister(gc, &qmp->efd);
     libxl__carefd_close(qmp->cfd);
@@ -1668,6 +1738,75 @@ void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 
     CTX_UNLOCK;
 }
+
+/*
+ * Actual libxl__ev_qmp
+ */
+
+void libxl__ev_qmp_init(libxl__ev_qmp *ev)
+{
+    ev->domid = INVALID_DOMID;
+    ev->callback = NULL;
+    ev->efd = NULL;
+    ev->id = -1;
+}
+
+int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
+                           libxl__ev_qmp_callback *callback,
+                           uint32_t domid,
+                           const char *cmd, libxl__json_object *args)
+{
+    libxl__ev_qmp_state *qmp;
+    int rc;
+
+    CTX_LOCK;
+
+    LOGD(DEBUG, domid, " ev %p, cmd '%s'", ev, cmd);
+
+    /* Connect to QEMU if not already connected */
+    qmp = libxl__ev_qmp_start(gc, domid);
+    if (!qmp) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = ev_qmp_queue_command(gc, qmp, cmd, args, ev->efd);
+    if (rc)
+        goto out;
+
+    ev->domid = domid;
+    ev->callback = callback;
+    ev->id = qmp->last_id_used;
+    /* fd is in the queue, and should be closed once sent */
+    ev->efd = NULL;
+
+    LIBXL_TAILQ_INSERT_TAIL(&qmp->qmp_events, ev, entry);
+
+    rc = 0;
+
+out:
+    CTX_UNLOCK;
+    return rc;
+}
+
+void libxl__ev_qmp_deregister(libxl__gc *gc, libxl__ev_qmp *ev)
+{
+    LOGD(DEBUG, ev->domid, " ev %p", ev);
+
+    CTX_LOCK;
+
+    if (libxl__ev_qmp_isregistered(ev)) {
+        LIBXL_TAILQ_REMOVE(&CTX->qmp_ev->qmp_events, ev, entry);
+    }
+    libxl__ev_qmp_init(ev);
+
+    CTX_UNLOCK;
+}
+
+int libxl__ev_qmp_isregistered(const libxl__ev_qmp *ev)
+{
+    return ev->domid != INVALID_DOMID;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index f2ff01718d..ada97615d5 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -13,6 +13,20 @@ libxl__qmp_message_type = Enumeration("qmp_message_type", [
     (5, "invalid"),
     ])
 
+libxl__qmp_error_class = Enumeration("qmp_error_class", [
+    # No error
+    (0, "NONE"),
+    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
+    (1, "libxl_error"),
+    # QMP error classes described in QEMU sources code (QapiErrorClass)
+    (2, "GenericError"),
+    (3, "CommandNotFound"),
+    (4, "DeviceNotActive"),
+    (5, "DeviceNotFound"),
+    # Unrecognized QMP error class
+    (6, "Unknown"),
+    ])
+
 libxl__device_kind = Enumeration("device_kind", [
     (0, "NONE"),
     (1, "VIF"),
-- 
Anthony PERARD


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

[PATCH v3 21/31] libxl_qmp_ev: Handle write to socket

[Anthony PERARD]

The libxl__ev_qmp_* will now send commands to QEMU when the socket is
ready for writes. Also stop pulling for POLLOUT events once the send
queue is empty.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9b5eb8fd35..02eae1f5ce 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1416,6 +1416,7 @@ static int ev_qmp_queue_command(libxl__gc *gc,
     out->len = len;
     out->efd = efd;
     LIBXL_TAILQ_INSERT_TAIL(&qmp->tx_buf, out, entry);
+    libxl__ev_fd_modify(gc, &qmp->efd, qmp->efd.events | POLLOUT);
 
     return 0;
 }
@@ -1568,6 +1569,46 @@ static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
     return 0;
 }
 
+static int ev_qmp_callback_writable(libxl__egc *egc,
+                                    libxl__ev_qmp_state *qmp,
+                                    int fd)
+{
+    EGC_GC;
+    libxl__qmp_tx_buf *buf;
+    int rc;
+
+    if (!qmp->ready) {
+        return 0;
+    }
+
+    if (LIBXL_TAILQ_EMPTY(&qmp->tx_buf))
+        return 0;
+
+    buf = LIBXL_TAILQ_FIRST(&qmp->tx_buf);
+
+    LOG_QMP("sending: '%.*s'", (int)buf->len, buf->buf);
+
+    if (buf->efd) {
+        int buf_fd = libxl__carefd_fd(buf->efd);
+        rc = libxl__sendmsg_fds(gc, fd, buf->buf, buf->len,
+                                1, &buf_fd, "QMP socket");
+        libxl__carefd_close(buf->efd);
+    } else {
+        rc = libxl_write_exactly(CTX, fd, buf->buf, buf->len,
+                                 "QMP command", "QMP socket");
+    }
+
+    if (rc)
+        goto out;
+
+    LIBXL_TAILQ_REMOVE(&qmp->tx_buf, buf, entry);
+    free(buf->buf);
+    free(buf);
+
+out:
+    return 1;
+}
+
 /* When the QMP client reach the conclusion that the QMP connection doesn't
  * work anymore, this function can be called to propagate the error to every
  * callback registered. And stop the client. */
@@ -1617,6 +1658,13 @@ static void ev_qmp_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
             return;
         }
     }
+    if (revents & POLLOUT) {
+        int ret = ev_qmp_callback_writable(egc, qmp, fd);
+        if (ret == 0) {
+            /* nothing to write, disable it. */
+            libxl__ev_fd_modify(gc, &qmp->efd, events & ~POLLOUT);
+        }
+    }
 }
 
 static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
-- 
Anthony PERARD


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

[PATCH v3 22/31] libxl_qmp: Simplify qmp_response_type() prototype

[Anthony PERARD]

Remove the libxl__qmp_handler* argument so the function can be reused
later in a different context.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 02eae1f5ce..0e7ec54b9f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -280,8 +280,7 @@ static int enable_qmp_capabilities(libxl__qmp_handler *qmp)
  * Helpers
  */
 
-static libxl__qmp_message_type qmp_response_type(libxl__qmp_handler *qmp,
-                                                 const libxl__json_object *o)
+static libxl__qmp_message_type qmp_response_type(const libxl__json_object *o)
 {
     libxl__qmp_message_type type;
     libxl__json_map_node *node = NULL;
@@ -347,7 +346,7 @@ static int qmp_handle_response(libxl__gc *gc, libxl__qmp_handler *qmp,
 {
     libxl__qmp_message_type type = LIBXL__QMP_MESSAGE_TYPE_INVALID;
 
-    type = qmp_response_type(qmp, resp);
+    type = qmp_response_type(resp);
     LOGD(DEBUG, qmp->domid, "message type: %s", libxl__qmp_message_type_to_string(type));
 
     switch (type) {
-- 
Anthony PERARD


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

Re: [PATCH v3 22/31] libxl_qmp: Simplify qmp_response_type() prototype

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 22/31] libxl_qmp: Simplify qmp_response_type() prototype"):
> Remove the libxl__qmp_handler* argument so the function can be reused
> later in a different context.

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

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

[PATCH v3 23/31] libxl_qmp_ev: Handle messages from QEMU

[Anthony PERARD]

This will handle messages received, and call callbacks registered via
libxl__ev_qmp_register().

This also print error messages from QMP on behalf of the callback.

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

---
Should we let callbacks print error messages themself? They already have
the error class, which is often GenericError, a human readable
error message.
---
 tools/libxl/libxl_qmp.c | 100 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 0e7ec54b9f..db07c1822a 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1420,6 +1420,101 @@ static int ev_qmp_queue_command(libxl__gc *gc,
     return 0;
 }
 
+/*
+ * Handle messages received from QMP server
+ */
+
+static void ev_qmp_handle_return(libxl__egc *egc,
+                                 libxl__ev_qmp_state *qmp,
+                                 const libxl__json_object *resp,
+                                 libxl__qmp_message_type type)
+{
+    EGC_GC;
+    const libxl__json_object *o;
+    int id;
+    libxl__ev_qmp *ev;
+    const libxl__json_object *ret;
+    libxl__qmp_error_class error_class = 0;
+
+    o = libxl__json_map_get("id", resp, JSON_INTEGER);
+    if (!o) {
+        const char *error_desc;
+
+        /* unexpected message, attempt to find an error description */
+        o = libxl__json_map_get("error", resp, JSON_MAP);
+        o = libxl__json_map_get("desc", o, JSON_STRING);
+        error_desc = libxl__json_object_get_string(o);
+        if (error_desc)
+            LOGD(ERROR, qmp->domid, "%s", error_desc);
+        else
+            LOGD(ERROR, qmp->domid, "Received unexpected message: %s",
+                 libxl__json_object_to_json(gc, resp));
+        return;
+    }
+
+    id = libxl__json_object_get_integer(o);
+    LIBXL_TAILQ_FOREACH(ev, &qmp->qmp_events, entry) {
+        if (ev->id == id)
+            break;
+    }
+    if (!ev)
+        /* callback not found */
+        return;
+
+    switch (type) {
+    case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
+        ret = libxl__json_map_get("return", resp, JSON_ANY);
+        break;
+    }
+    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
+        const char *s;
+        const libxl__json_object *err;
+
+        error_class = LIBXL__QMP_ERROR_CLASS_UNKNOWN;
+        err = libxl__json_map_get("error", resp, JSON_MAP);
+        o = libxl__json_map_get("class", err, JSON_STRING);
+        s = libxl__json_object_get_string(o);
+        if (s)
+            libxl__qmp_error_class_from_string(s, &error_class);
+
+        o = libxl__json_map_get("desc", err, JSON_STRING);
+        s = libxl__json_object_get_string(o);
+        if (s)
+            LOGD(ERROR, qmp->domid, "%s", s);
+
+        ret = NULL;
+        break;
+    }
+    default:
+        abort();
+    }
+    ev->id = -1;
+    ev->callback(egc, ev, ret, error_class);
+}
+
+static void ev_qmp_handle_message(libxl__egc *egc,
+                                  libxl__ev_qmp_state *qmp,
+                                  const libxl__json_object *resp)
+{
+    EGC_GC;
+    libxl__qmp_message_type type = qmp_response_type(resp);
+
+    switch (type) {
+    case LIBXL__QMP_MESSAGE_TYPE_QMP:
+        /* greeting message */
+        return;
+    case LIBXL__QMP_MESSAGE_TYPE_RETURN:
+    case LIBXL__QMP_MESSAGE_TYPE_ERROR:
+        ev_qmp_handle_return(egc, qmp, resp, type);
+        return;
+    case LIBXL__QMP_MESSAGE_TYPE_EVENT:
+        /* Event are ignored */
+        return;
+    case LIBXL__QMP_MESSAGE_TYPE_INVALID:
+        return;
+    }
+}
+
 static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
                                     int fd)
 {
@@ -1557,6 +1652,8 @@ static int ev_qmp_callback_readable(libxl__egc *egc, libxl__ev_qmp_state *qmp,
 
         LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, o));
 
+        ev_qmp_handle_message(egc, qmp, o);
+
         /* check if there is another message received at the same time */
         if (buf) {
             end = strstr(buf->buf + buf->consumed, "\r\n");
@@ -1762,6 +1859,9 @@ void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 
     LOGD(DEBUG, qmp->domid, "Stopping QMP handler");
 
+    /* There should be no more events requested */
+    assert(LIBXL_TAILQ_EMPTY(&qmp->qmp_events));
+
     LIBXL_TAILQ_FOREACH_SAFE(buf, &qmp->bufs, entry, tbuf)
         free(buf);
     LIBXL_TAILQ_INIT(&qmp->bufs);
-- 
Anthony PERARD


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

[PATCH v3 24/31] libxl_qmp_ev: Respond to QMP greeting

[Anthony PERARD]

Slight change in the infrastructure to allow to send a buffer before any
command that would already been prepared.

qmp_capabilities needs to be the first command sent.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index db07c1822a..adf466e4c4 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1387,6 +1387,8 @@ struct libxl__ev_qmp_state {
     unsigned int last_id_used;
     /* Indicate that QEMU is ready to respond to command. */
     bool ready;
+    /* Allow to commands while !ready, to respond to QMP greeting. */
+    int priority_send;
     LIBXL_TAILQ_HEAD(libxl__qmp_tx_bufs, libxl__qmp_tx_buf) tx_buf;
 
     LIBXL_TAILQ_HEAD(libxl__ev_qmps, libxl__ev_qmp) qmp_events;
@@ -1398,7 +1400,8 @@ static int ev_qmp_queue_command(libxl__gc *gc,
                                 libxl__ev_qmp_state *qmp,
                                 const char *cmd,
                                 const libxl__json_object *args,
-                                libxl__carefd *efd)
+                                libxl__carefd *efd,
+                                bool high_priority)
 {
     char *buf = NULL;
     size_t len;
@@ -1414,12 +1417,78 @@ static int ev_qmp_queue_command(libxl__gc *gc,
     out->buf = buf;
     out->len = len;
     out->efd = efd;
-    LIBXL_TAILQ_INSERT_TAIL(&qmp->tx_buf, out, entry);
+    if (high_priority) {
+        /* qmp_capabilities cmd need to be send out first */
+        LIBXL_TAILQ_INSERT_HEAD(&qmp->tx_buf, out, entry);
+        qmp->priority_send += 1;
+    } else {
+        LIBXL_TAILQ_INSERT_TAIL(&qmp->tx_buf, out, entry);
+    }
     libxl__ev_fd_modify(gc, &qmp->efd, qmp->efd.events | POLLOUT);
 
     return 0;
 }
 
+/* QMP greeting response */
+typedef struct {
+    libxl__ev_qmp ev;
+    libxl__ev_qmp_state *qmp;
+} ev_qmp_greeting_state;
+static void ev_qmp_callback_capabilities(libxl__egc *egc,
+                                        libxl__ev_qmp *ev,
+                                        const libxl__json_object *o,
+                                        libxl__qmp_error_class error)
+{
+    EGC_GC;
+    ev_qmp_greeting_state *qegs = CONTAINER_OF(ev, *qegs, ev);
+    libxl__ev_qmp_state *qmp = qegs->qmp;
+
+    CTX_LOCK;
+    qmp->ready = true;
+
+    /* Allow potential command to be sent */
+    libxl__ev_fd_modify(gc, &qmp->efd, qmp->efd.events | POLLOUT);
+
+    CTX_UNLOCK;
+
+    libxl__ev_qmp_deregister(gc, ev);
+
+    free(qegs);
+}
+
+static int ev_qmp_callback_greeting(libxl__egc *egc,
+                                    libxl__ev_qmp_state *qmp)
+{
+    EGC_GC;
+    ev_qmp_greeting_state *qegs;
+    int rc;
+
+    /* Will be freed in callback */
+    qegs = libxl__malloc(NOGC, sizeof(*qegs));
+
+    libxl__ev_qmp_init(&qegs->ev);
+
+    /* Do part of libxl__ev_qmp_register as this is the only time where the
+     * argument high_priority is true. */
+
+    qegs->qmp = qmp;
+    qegs->ev.domid = qmp->domid;
+    qegs->ev.callback = ev_qmp_callback_capabilities;
+    rc = ev_qmp_queue_command(gc, qmp, "qmp_capabilities", NULL, NULL, true);
+    if (rc)
+        goto out;
+    qegs->ev.id = qmp->last_id_used;
+
+    CTX_LOCK;
+    LIBXL_TAILQ_INSERT_TAIL(&qmp->qmp_events, &qegs->ev, entry);
+    CTX_UNLOCK;
+
+out:
+    if (rc)
+        free(qegs);
+    return rc;
+}
+
 /*
  * Handle messages received from QMP server
  */
@@ -1501,7 +1570,8 @@ static void ev_qmp_handle_message(libxl__egc *egc,
 
     switch (type) {
     case LIBXL__QMP_MESSAGE_TYPE_QMP:
-        /* greeting message */
+        /* On the greeting message from the server, call qmp_capabilities */
+        ev_qmp_callback_greeting(egc, qmp);
         return;
     case LIBXL__QMP_MESSAGE_TYPE_RETURN:
     case LIBXL__QMP_MESSAGE_TYPE_ERROR:
@@ -1673,7 +1743,9 @@ static int ev_qmp_callback_writable(libxl__egc *egc,
     libxl__qmp_tx_buf *buf;
     int rc;
 
-    if (!qmp->ready) {
+    /* Don't send anything until connected, unless there is the response to the
+     * greeting message */
+    if (!qmp->ready && !qmp->priority_send) {
         return 0;
     }
 
@@ -1701,6 +1773,9 @@ static int ev_qmp_callback_writable(libxl__egc *egc,
     free(buf->buf);
     free(buf);
 
+    if (!qmp->ready && qmp->priority_send)
+        qmp->priority_send -= 1;
+
 out:
     return 1;
 }
@@ -1771,6 +1846,7 @@ static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
     LIBXL_TAILQ_INIT(&qmp->bufs);
     qmp->last_id_used = 0;
     qmp->ready = false;
+    qmp->priority_send = 0;
     LIBXL_TAILQ_INIT(&qmp->tx_buf);
     LIBXL_TAILQ_INIT(&qmp->qmp_events);
 }
@@ -1878,6 +1954,7 @@ void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
     LIBXL_TAILQ_INIT(&qmp->qmp_events);
 
     qmp->ready = false;
+    qmp->priority_send = 0;
 
     libxl__ev_fd_deregister(gc, &qmp->efd);
     libxl__carefd_close(qmp->cfd);
@@ -1917,7 +1994,7 @@ int libxl__ev_qmp_register(libxl__gc *gc, libxl__ev_qmp *ev,
         goto out;
     }
 
-    rc = ev_qmp_queue_command(gc, qmp, cmd, args, ev->efd);
+    rc = ev_qmp_queue_command(gc, qmp, cmd, args, ev->efd, false);
     if (rc)
         goto out;
 
-- 
Anthony PERARD


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

Re: [PATCH v3 24/31] libxl_qmp_ev: Respond to QMP greeting

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 24/31] libxl_qmp_ev: Respond to QMP greeting"):
> Slight change in the infrastructure to allow to send a buffer before any
> command that would already been prepared.

I'm inclined to think that this would be better done as part of the
"connect to qmp" state machine.

Ian.

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

[PATCH v3 25/31] libxl_qmp_ev: Disconnect QMP when no more events

[Anthony PERARD]

This will check if there is more libxl_ev_qmp in flight, and if not,
just disconnect from the QMP socket.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index adf466e4c4..302178c5f5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1800,6 +1800,7 @@ static void ev_qmp_callback_error(libxl__egc *egc, libxl__ev_qmp_state *qmp)
     libxl__ev_qmp_stop(gc, qmp);
 }
 
+static void libxl__ev_qmp_checkstate(libxl__gc *gc, libxl__ev_qmp_state *qmp);
 static void ev_qmp_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
                                int fd, short events, short revents)
 {
@@ -1836,6 +1837,8 @@ static void ev_qmp_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
             libxl__ev_fd_modify(gc, &qmp->efd, events & ~POLLOUT);
         }
     }
+    /* Check if there is still something to do or just disconnect. */
+    libxl__ev_qmp_checkstate(gc, qmp);
 }
 
 static void libxl__ev_qmp_state_init(libxl__ev_qmp_state *qmp)
@@ -1922,6 +1925,16 @@ out:
     return qmp;
 }
 
+/* Check if it should stay around or just close the socket to let other use use
+ * it. */
+static void libxl__ev_qmp_checkstate(libxl__gc *gc, libxl__ev_qmp_state *qmp)
+{
+    if (LIBXL_TAILQ_EMPTY(&qmp->qmp_events)) {
+        LOGD(DEBUG, qmp->domid, "Nothing left to do, stop QMP ev handler");
+        libxl__ev_qmp_stop(gc, qmp);
+    }
+}
+
 void libxl__ev_qmp_stop(libxl__gc *gc, libxl__ev_qmp_state *qmp)
 {
     libxl__qmp_rx_buf *buf, *tbuf;
-- 
Anthony PERARD


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

[PATCH v3 26/31] libxl_qmp: Disable beautify for QMP generated cmd

[Anthony PERARD]

There is no need for it.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 302178c5f5..f44b313a5e 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -601,6 +601,11 @@ static char *qmp_prepare_qmp_cmd(libxl__gc *gc,
         return NULL;
     }
 
+#if HAVE_YAJL_V2
+    /* Disable beautify for data sent to QEMU */
+    yajl_gen_config(hand, yajl_gen_beautify, 0);
+#endif
+
     yajl_gen_map_open(hand);
     libxl__yajl_gen_asciiz(hand, "execute");
     libxl__yajl_gen_asciiz(hand, cmd);
-- 
Anthony PERARD


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

Re: [PATCH v3 26/31] libxl_qmp: Disable beautify for QMP generated cmd

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 26/31] libxl_qmp: Disable beautify for QMP generated cmd"):
> There is no need for it.

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

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

[PATCH v3 27/31] libxl_qmp: Implement libxl__qmp_insert_cdrom_ev

[Anthony PERARD]

This function is a reimplementation of libxl__qmp_insert_cdrom() but to be
use with libxl__ev_qmp.

It also open the cdrom in libxl and send the FD via QMP, so QEMU doesn't
need access permition on the cdrom file.

libxl_cdrom_insert() will need to be reorganize to be able to use that
new function, this is done in the next few patches.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 16533f651e..371b27e866 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1870,6 +1870,10 @@ _hidden int libxl__qmp_restore(libxl__gc *gc, int domid, const char *filename);
 /* Set dirty bitmap logging status */
 _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable);
 _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk);
+int libxl__qmp_insert_cdrom_ev(libxl__gc *gc, int domid,
+                               libxl__ev_qmp *ev,
+                               libxl__ev_qmp_callback *callback,
+                               const libxl_device_disk *disk);
 /* Add a virtual CPU */
 _hidden int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int index);
 /* Query the bitmap of CPUs */
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f44b313a5e..6728e5ad9e 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1361,6 +1361,108 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
     return ret;
 }
 
+/*
+ * Function using libxl__ev_qmp
+ */
+
+/* libxl__qmp_insert_cdrom_ev */
+
+struct cdrom_insert_ev_callback {
+    libxl__ev_qmp ev;
+    libxl__ev_qmp *callback_ev;
+    libxl__ev_qmp_callback *callback;
+    const libxl_device_disk *disk;
+};
+
+static void cdrom_insert_fd_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                               const libxl__json_object *response,
+                               libxl__qmp_error_class error)
+{
+    EGC_GC;
+    libxl__json_object *args = NULL;
+    const libxl__json_object *o;
+    struct cdrom_insert_ev_callback *cb = CONTAINER_OF(ev, *cb, ev);
+    const libxl_device_disk *disk = cb->disk;
+    int dev_number = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+    int fdset;
+    int rc;
+
+    o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
+    if (!o) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    fdset = libxl__json_object_get_integer(o);
+
+    QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", dev_number);
+    QMP_PARAMETERS_SPRINTF(&args, "target", "/dev/fdset/%d", fdset);
+    qmp_parameters_add_string(gc, &args, "arg",
+                              libxl__qemu_disk_format_string(disk->format));
+    rc = libxl__ev_qmp_register(gc, cb->callback_ev,
+                                cb->callback,
+                                ev->domid,
+                                "change", args);
+    if (rc)
+        goto out;
+out:
+    libxl__ev_qmp_deregister(gc, ev);
+    if (!o || rc) {
+        cb->callback(egc, cb->callback_ev, NULL,
+                     LIBXL__QMP_ERROR_CLASS_LIBXL_ERROR);
+    }
+    free(cb);
+}
+
+
+int libxl__qmp_insert_cdrom_ev(libxl__gc *gc, int domid, libxl__ev_qmp *ev,
+                               libxl__ev_qmp_callback *callback,
+                               const libxl_device_disk *disk)
+{
+    libxl__json_object *args = NULL;
+    int dev_number = libxl__device_disk_dev_number(disk->vdev, NULL, NULL);
+    int rc;
+    libxl__carefd *cdrom_efd = NULL;
+    struct cdrom_insert_ev_callback *cb = NULL;
+
+    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
+        QMP_PARAMETERS_SPRINTF(&args, "device", "ide-%i", dev_number);
+        rc = libxl__ev_qmp_register(gc, ev, callback, domid,
+                                    "eject", args);
+    } else {
+        libxl__carefd_begin();
+        cdrom_efd = libxl__carefd_opened(CTX, open(disk->pdev_path, O_RDONLY));
+        if (!cdrom_efd) {
+            LOGED(ERROR, domid,
+                  "Failed to open cdrom file %s", disk->pdev_path);
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        cb = libxl__malloc(NOGC, sizeof (*cb));
+        libxl__ev_qmp_init(&cb->ev);
+        cb->callback = callback;
+        cb->callback_ev = ev;
+        cb->disk = disk;
+
+        /* This free form parameter is not use by QEMU or libxl. */
+        QMP_PARAMETERS_SPRINTF(&args, "opaque", "%s:%s",
+                               libxl_disk_format_to_string(disk->format),
+                               disk->pdev_path);
+        cb->ev.efd = cdrom_efd;
+        rc = libxl__ev_qmp_register(gc, &cb->ev, cdrom_insert_fd_cb, domid,
+                                    "add-fd", args);
+        if (rc)
+            goto out;
+    }
+out:
+    if (rc) {
+        free(cb);
+        libxl__carefd_close(cdrom_efd);
+    }
+    return rc;
+}
+
+
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
 typedef struct libxl__qmp_rx_buf libxl__qmp_rx_buf;
-- 
Anthony PERARD


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

Re: [PATCH v3 27/31] libxl_qmp: Implement libxl__qmp_insert_cdrom_ev

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 27/31] libxl_qmp: Implement libxl__qmp_insert_cdrom_ev"):
> This function is a reimplementation of libxl__qmp_insert_cdrom() but to be
> use with libxl__ev_qmp.

Overall, I think what I am missing in much of this is a highly-formal
description of the states of these asynchronous things, and the
permitted call orders, etc.  Like for libxl_ev_FOO, which you are
trying to follow, but doesn't really fit.

I appreciate that I may be asking for a lot of rework.  Sorry.

I think before you do a full resend, it would be worth writing out the
internal interfaces in a form that addresses this aspect of my review
(ie, the part in libxl_internal.h).

Also my comments about whether the buffer queue is really needed.

Regards,
Ian.

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

[PATCH v3 28/31] libxl_disk: Cut libxl_cdrom_insert into step

[Anthony PERARD]

This is to prepare libxl_cdrom_insert to be able to send commands to
QEMU via the libxl__ev_qmp. The next patch is going to make use of it.

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

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index e9eceb65e3..a3bf974fe3 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -661,31 +661,56 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
     return rc;
 }
 
+typedef struct {
+    libxl__ao *ao;
+    libxl_domain_config d_config;
+    const char *be_path;
+    const char *libxl_path;
+    libxl_device_disk *disk;
+    libxl_device_disk disk_empty;
+    libxl_device_disk disk_saved;
+    int dm_ver;
+    int domid;
+    libxl__domain_userdata_lock *lock;
+} libxl__cdrom_insert_state;
+static void cdrom_insert_ejected(libxl__egc *egc,
+                                 libxl__cdrom_insert_state *cis);
+static void cdrom_insert_inserted(libxl__egc *egc,
+                                  libxl__cdrom_insert_state *cis);
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc);
+
 int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
                        const libxl_asyncop_how *ao_how)
 {
     AO_CREATE(ctx, domid, ao_how);
     int num = 0, i;
-    libxl_device_disk *disks = NULL, disk_saved, disk_empty;
-    libxl_domain_config d_config;
-    int rc, dm_ver;
+    libxl_device_disk *disks = NULL, *disk_saved, *disk_empty;
+    int rc;
     libxl__device device;
-    const char *be_path, *libxl_path;
-    char * tmp;
-    libxl__domain_userdata_lock *lock = NULL;
-    xs_transaction_t t = XBT_NULL;
-    flexarray_t *insert = NULL, *empty = NULL;
+    libxl__cdrom_insert_state *cis;
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_disk_init(&disk_empty);
-    libxl_device_disk_init(&disk_saved);
-    libxl_device_disk_copy(ctx, &disk_saved, disk);
+    GCNEW(cis);
+    cis->ao = ao;
+    cis->domid = domid;
+    // XXX: can I do that?  is disk going to exist until the AO is over?
+    cis->disk = disk;
 
-    disk_empty.format = LIBXL_DISK_FORMAT_EMPTY;
-    disk_empty.vdev = libxl__strdup(NOGC, disk->vdev);
-    disk_empty.pdev_path = libxl__strdup(NOGC, "");
-    disk_empty.is_cdrom = 1;
-    libxl__device_disk_setdefault(gc, domid, &disk_empty, false);
+    disk_empty = &cis->disk_empty;
+    disk_saved = &cis->disk_saved;
+
+
+    libxl_domain_config_init(&cis->d_config);
+    libxl_device_disk_init(disk_empty);
+    libxl_device_disk_init(disk_saved);
+    libxl_device_disk_copy(ctx, disk_saved, disk);
+
+    disk_empty->format = LIBXL_DISK_FORMAT_EMPTY;
+    disk_empty->vdev = libxl__strdup(NOGC, disk->vdev);
+    disk_empty->pdev_path = libxl__strdup(NOGC, "");
+    disk_empty->is_cdrom = 1;
+    libxl__device_disk_setdefault(gc, domid, disk_empty, false);
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
     if (type == LIBXL_DOMAIN_TYPE_INVALID) {
@@ -704,8 +729,8 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         goto out;
     }
 
-    dm_ver = libxl__device_model_version_running(gc, domid);
-    if (dm_ver == -1) {
+    cis->dm_ver = libxl__device_model_version_running(gc, domid);
+    if (cis->dm_ver == -1) {
         LOGD(ERROR, domid, "Cannot determine device model version");
         rc = ERROR_FAIL;
         goto out;
@@ -737,31 +762,14 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     rc = libxl__device_from_disk(gc, domid, disk, &device);
     if (rc) goto out;
 
-    be_path = libxl__device_backend_path(gc, &device);
-    libxl_path = libxl__device_libxl_path(gc, &device);
-
-    insert = flexarray_make(gc, 4, 1);
-
-    flexarray_append_pair(insert, "type",
-                          libxl__device_disk_string_of_backend(disk->backend));
-    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
-        flexarray_append_pair(insert, "params",
-                        GCSPRINTF("%s:%s",
-                            libxl__device_disk_string_of_format(disk->format),
-                            disk->pdev_path));
-    else
-        flexarray_append_pair(insert, "params", "");
-
-    empty = flexarray_make(gc, 4, 1);
-    flexarray_append_pair(empty, "type",
-                          libxl__device_disk_string_of_backend(disk->backend));
-    flexarray_append_pair(empty, "params", "");
+    cis->be_path = libxl__device_backend_path(gc, &device);
+    cis->libxl_path = libxl__device_libxl_path(gc, &device);
 
     /* Note: CTX lock is already held at this point so lock hierarchy
      * is maintained.
      */
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
+    cis->lock = libxl__lock_domain_userdata(gc, domid);
+    if (!cis->lock) {
         rc = ERROR_LOCK_FAIL;
         goto out;
     }
@@ -769,11 +777,46 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     /* We need to eject the original image first. This is implemented
      * by inserting empty media. JSON is not updated.
      */
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        rc = libxl__qmp_insert_cdrom(gc, domid, &disk_empty);
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_insert_cdrom(gc, domid, disk_empty);
         if (rc) goto out;
     }
 
+    cdrom_insert_ejected(egc, cis);
+
+    return AO_INPROGRESS;
+
+out:
+    libxl__device_list_free(&libxl__disk_devtype, disks, num);
+    cdrom_insert_done(egc, cis, rc);
+
+    if (rc) return AO_CREATE_FAIL(rc);
+    return AO_INPROGRESS;
+}
+
+static void cdrom_insert_ejected(libxl__egc *egc,
+                                 libxl__cdrom_insert_state *cis)
+{
+    STATE_AO_GC(cis->ao);
+    uint32_t domid = cis->domid;
+    int rc;
+    const char *be_path, *libxl_path;
+    char * tmp;
+    xs_transaction_t t = XBT_NULL;
+    flexarray_t *empty;
+
+    libxl_domain_config *d_config = &cis->d_config;
+    libxl_device_disk *disk = cis->disk;
+    libxl_device_disk *disk_saved = &cis->disk_saved;
+
+    be_path = cis->be_path;
+    libxl_path = cis->libxl_path;
+
+    empty = flexarray_make(gc, 4, 1);
+    flexarray_append_pair(empty, "type",
+                          libxl__device_disk_string_of_backend(disk->backend));
+    flexarray_append_pair(empty, "params", "");
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -800,19 +843,55 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc < 0) goto out;
     }
 
-    rc = libxl__get_domain_configuration(gc, domid, &d_config);
+    rc = libxl__get_domain_configuration(gc, domid, d_config);
     if (rc) goto out;
 
-    device_add_domain_config(gc, &d_config, &libxl__disk_devtype, &disk_saved);
+    device_add_domain_config(gc, d_config, &libxl__disk_devtype, disk_saved);
 
-    rc = libxl__dm_check_start(gc, &d_config, domid);
+    rc = libxl__dm_check_start(gc, d_config, domid);
     if (rc) goto out;
 
-    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         rc = libxl__qmp_insert_cdrom(gc, domid, disk);
         if (rc) goto out;
     }
 
+    cdrom_insert_inserted(egc, cis);
+
+    return;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+    cdrom_insert_done(egc, cis, rc);
+}
+
+static void cdrom_insert_inserted(libxl__egc *egc,
+                                  libxl__cdrom_insert_state *cis)
+{
+    STATE_AO_GC(cis->ao);
+    uint32_t domid = cis->domid;
+    int rc;
+    const char *be_path, *libxl_path;
+    char * tmp;
+    xs_transaction_t t = XBT_NULL;
+    flexarray_t *insert;
+
+    libxl_device_disk *disk = cis->disk;
+    libxl_domain_config *d_config = &cis->d_config;
+    be_path = cis->be_path;
+    libxl_path = cis->libxl_path;
+
+    insert = flexarray_make(gc, 4, 1);
+    flexarray_append_pair(insert, "type",
+                          libxl__device_disk_string_of_backend(disk->backend));
+    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
+        flexarray_append_pair(insert, "params",
+                        GCSPRINTF("%s:%s",
+                            libxl__device_disk_string_of_format(disk->format),
+                            disk->pdev_path));
+    else
+        flexarray_append_pair(insert, "params", "");
+
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
@@ -826,7 +905,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
             goto out;
         }
 
-        rc = libxl__set_domain_configuration(gc, domid, &d_config);
+        rc = libxl__set_domain_configuration(gc, domid, d_config);
         if (rc) goto out;
 
         char **kvs = libxl__xs_kvs_of_flexarray(gc, insert);
@@ -842,22 +921,24 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         if (rc < 0) goto out;
     }
 
-    /* success, no actual async */
-    libxl__ao_complete(egc, ao, 0);
-
     rc = 0;
 
 out:
     libxl__xs_transaction_abort(gc, &t);
-    libxl__device_list_free(&libxl__disk_devtype, disks, num);
-    libxl_device_disk_dispose(&disk_empty);
-    libxl_device_disk_dispose(&disk_saved);
-    libxl_domain_config_dispose(&d_config);
+    cdrom_insert_done(egc, cis, rc);
+}
 
-    if (lock) libxl__unlock_domain_userdata(lock);
+static void cdrom_insert_done(libxl__egc *egc,
+                              libxl__cdrom_insert_state *cis,
+                              int rc)
+{
+    STATE_AO_GC(cis->ao);
 
-    if (rc) return AO_CREATE_FAIL(rc);
-    return AO_INPROGRESS;
+    libxl_domain_config_dispose(&cis->d_config);
+    libxl_device_disk_dispose(&cis->disk_empty);
+    libxl_device_disk_dispose(&cis->disk_saved);
+    if (cis->lock) libxl__unlock_domain_userdata(cis->lock);
+    libxl__ao_complete(egc, ao, rc);
 }
 
 /* libxl__alloc_vdev only works on the local domain, that is the domain
-- 
Anthony PERARD


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

[PATCH v3 29/31] libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp

[Anthony PERARD]

So when QEMU is involve, the operation will be asynchrone and will
finish later.

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

diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
index a3bf974fe3..9808a53c1b 100644
--- a/tools/libxl/libxl_disk.c
+++ b/tools/libxl/libxl_disk.c
@@ -663,6 +663,7 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t domid,
 
 typedef struct {
     libxl__ao *ao;
+    libxl__ev_qmp ev;
     libxl_domain_config d_config;
     const char *be_path;
     const char *libxl_path;
@@ -675,8 +676,14 @@ typedef struct {
 } libxl__cdrom_insert_state;
 static void cdrom_insert_ejected(libxl__egc *egc,
                                  libxl__cdrom_insert_state *cis);
+static void cdrom_insert_ejected_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                        const libxl__json_object *response,
+                                        libxl__qmp_error_class error);
 static void cdrom_insert_inserted(libxl__egc *egc,
                                   libxl__cdrom_insert_state *cis);
+static void cdrom_insert_inserted_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                         const libxl__json_object *response,
+                                         libxl__qmp_error_class error);
 static void cdrom_insert_done(libxl__egc *egc,
                               libxl__cdrom_insert_state *cis,
                               int rc);
@@ -694,6 +701,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
     GCNEW(cis);
     cis->ao = ao;
     cis->domid = domid;
+    libxl__ev_qmp_init(&cis->ev);
     // XXX: can I do that?  is disk going to exist until the AO is over?
     cis->disk = disk;
 
@@ -778,12 +786,14 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
      * by inserting empty media. JSON is not updated.
      */
     if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        rc = libxl__qmp_insert_cdrom(gc, domid, disk_empty);
+        rc = libxl__qmp_insert_cdrom_ev(gc, domid, &cis->ev,
+                                        cdrom_insert_ejected_qmp_cb,
+                                        disk_empty);
         if (rc) goto out;
+    } else {
+        cdrom_insert_ejected(egc, cis);
     }
 
-    cdrom_insert_ejected(egc, cis);
-
     return AO_INPROGRESS;
 
 out:
@@ -794,6 +804,21 @@ out:
     return AO_INPROGRESS;
 }
 
+static void cdrom_insert_ejected_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                        const libxl__json_object *response,
+                                        libxl__qmp_error_class error)
+{
+    EGC_GC;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(ev, *cis, ev);
+
+    if (error) {
+        cdrom_insert_done(egc, cis, ERROR_FAIL);
+    } else {
+        libxl__ev_qmp_deregister(gc, ev);
+        cdrom_insert_ejected(egc, cis);
+    }
+}
+
 static void cdrom_insert_ejected(libxl__egc *egc,
                                  libxl__cdrom_insert_state *cis)
 {
@@ -852,12 +877,14 @@ static void cdrom_insert_ejected(libxl__egc *egc,
     if (rc) goto out;
 
     if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
+        rc = libxl__qmp_insert_cdrom_ev(gc, domid, &cis->ev,
+                                        cdrom_insert_inserted_qmp_cb,
+                                        disk);
         if (rc) goto out;
+    } else {
+        cdrom_insert_inserted(egc, cis);
     }
 
-    cdrom_insert_inserted(egc, cis);
-
     return;
 
 out:
@@ -865,6 +892,21 @@ out:
     cdrom_insert_done(egc, cis, rc);
 }
 
+static void cdrom_insert_inserted_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                         const libxl__json_object *response,
+                                         libxl__qmp_error_class error)
+{
+    EGC_GC;
+    libxl__cdrom_insert_state *cis = CONTAINER_OF(ev, *cis, ev);
+
+    if (error) {
+        cdrom_insert_done(egc, cis, ERROR_FAIL);
+    } else {
+        libxl__ev_qmp_deregister(gc, ev);
+        cdrom_insert_inserted(egc, cis);
+    }
+}
+
 static void cdrom_insert_inserted(libxl__egc *egc,
                                   libxl__cdrom_insert_state *cis)
 {
@@ -934,6 +976,7 @@ static void cdrom_insert_done(libxl__egc *egc,
 {
     STATE_AO_GC(cis->ao);
 
+    libxl__ev_qmp_deregister(gc, &cis->ev);
     libxl_domain_config_dispose(&cis->d_config);
     libxl_device_disk_dispose(&cis->disk_empty);
     libxl_device_disk_dispose(&cis->disk_saved);
-- 
Anthony PERARD


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

Re: [PATCH v3 29/31] libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 29/31] libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp"):
> So when QEMU is involve, the operation will be asynchrone and will
> finish later.

This looks roughly plausible, in the sense that if you address my
internal API concerns, and make this part fit again, it will probably
be good.

Ian.

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

[PATCH v3 30/31] libxl_dm: Pre-open QMP socket for QEMU

[Anthony PERARD]

When starting QEMU with dm_restrict=1, pre-open the QMP socket before
exec QEMU. That socket will be usefull to findout if QEMU is ready, and
pre-opening it means that libxl can connect to it without waiting for
QEMU to create it.

The pre-openning is conditionnal, based on the use of dm_restrict
because it is using a new command line option of QEMU, and dm_restrict
support in QEMU is newer.

-chardev socket,fd=X is available with QEMU 2.12, since commit:
> char: allow passing pre-opened socket file descriptor at startup
> 0935700f8544033ebbd41e1f13cd528f8a58d24d

dm_restrict will be available in QEMU 3.0.

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

Notes:
    We may want to set cloexec on the socket up to the forking point. Because if
    the socket stay open in a random process, libxl will not detected right away if
    QEMU as failed to start.  On the other hand, we can rely on timeouts.

 tools/libxl/libxl_dm.c | 73 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 18ada69e8b..10b35d822a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -24,6 +24,8 @@
 #include <sys/types.h>
 #include <pwd.h>
 #include <grp.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
 static const char *libxl_tapif_script(libxl__gc *gc)
 {
@@ -913,7 +915,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_monitor_fd)
 {
     const libxl_domain_create_info *c_info = &guest_config->c_info;
     const libxl_domain_build_info *b_info = &guest_config->b_info;
@@ -942,10 +944,58 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                       GCSPRINTF("%d", guest_domid), NULL);
 
     flexarray_append(dm_args, "-chardev");
-    flexarray_append(dm_args,
-                     GCSPRINTF("socket,id=libxl-cmd,"
-                                    "path=%s/qmp-libxl-%d,server,nowait",
-                                    libxl__run_dir_path(), guest_domid));
+    /* If we have to use dm_restrict, QEMU need to be new enough and will have
+     * the new interface where we can pre-open the QMP socket. */
+    if (libxl_defbool_val(b_info->dm_restrict))
+    {
+        int socket_fd = -1;
+        struct sockaddr_un un;
+        const char *socket_path;
+
+        socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);
+        *dm_monitor_fd = socket_fd;
+        if (socket_fd < 0) {
+            LOGED(ERROR, guest_domid, "Failed to create UNIX socket");
+            return ERROR_FAIL;
+        }
+
+        socket_path = GCSPRINTF("%s/qmp-libxl-%d",
+                                libxl__run_dir_path(), guest_domid);
+        if (strlen(socket_path) > sizeof(un.sun_path)) {
+            LOGD(ERROR, guest_domid, "UNIX socket path '%s' is too long",
+                 socket_path);
+            LOGD(DEBUG, guest_domid, "Path must be less than %zu bytes",
+                 sizeof(un.sun_path));
+            return ERROR_FAIL;
+        }
+
+        if (unlink(socket_path) < 0 && errno != ENOENT) {
+            LOGED(ERROR, guest_domid, "Failed to unlink socket %s", socket_path);
+            return ERROR_FAIL;
+        }
+
+        memset(&un, 0, sizeof(un));
+        un.sun_family = AF_UNIX;
+        strncpy(un.sun_path, socket_path, sizeof(un.sun_path));
+        if (bind(socket_fd, (struct sockaddr*) &un, sizeof(un)) < 0) {
+            LOGED(ERROR, guest_domid, "Failed to bind socket to %s", socket_path);
+            return ERROR_FAIL;
+        }
+
+        if (listen(socket_fd, 1) < 0) {
+            LOGED(ERROR, guest_domid, "Failed to listen on socket");
+            return ERROR_FAIL;
+        }
+
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
+                                   socket_fd));
+    } else {
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxl-cmd,"
+                                   "path=%s/qmp-libxl-%d,server,nowait",
+                                   libxl__run_dir_path(), guest_domid));
+    }
 
     flexarray_append(dm_args, "-no-shutdown");
     flexarray_append(dm_args, "-mon");
@@ -1722,7 +1772,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_monitor_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. */
 {
@@ -1735,10 +1786,11 @@ static int libxl__build_device_model_args(libxl__gc *gc,
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
         assert(dm_state_fd != NULL);
         assert(*dm_state_fd < 0);
+        assert(dm_monitor_fd != NULL);
         return libxl__build_device_model_args_new(gc, dm,
                                                   guest_domid, guest_config,
                                                   args, envs,
-                                                  state, dm_state_fd);
+                                                  state, dm_state_fd, dm_monitor_fd);
     default:
         LOGED(ERROR, guest_domid, "unknown device model version %d",
               guest_config->b_info.device_model_version);
@@ -1958,7 +2010,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, NULL);
     if (ret) {
         ret = ERROR_FAIL;
         goto out;
@@ -2244,6 +2296,7 @@ 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_monitor_fd = -1;
 
     if (libxl_defbool_val(b_info->device_model_stubdomain)) {
         abort();
@@ -2261,7 +2314,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     }
     rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
                                           &args, &envs, state,
-                                          &dm_state_fd);
+                                          &dm_state_fd,
+                                          &dm_monitor_fd);
     if (rc)
         goto out;
 
@@ -2359,6 +2413,7 @@ out_close:
     if (logfile_w >= 0) close(logfile_w);
 out:
     if (dm_state_fd >= 0) close(dm_state_fd);
+    if (dm_monitor_fd >= 0) close(dm_monitor_fd);
     if (rc)
         device_model_spawn_outcome(egc, dmss, rc);
 }
-- 
Anthony PERARD


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

Re: [PATCH v3 30/31] libxl_dm: Pre-open QMP socket for QEMU

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 30/31] libxl_dm: Pre-open QMP socket for QEMU"):
> When starting QEMU with dm_restrict=1, pre-open the QMP socket before
> exec QEMU. That socket will be usefull to findout if QEMU is ready, and
> pre-opening it means that libxl can connect to it without waiting for
> QEMU to create it.
...
> +        socket_fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +        *dm_monitor_fd = socket_fd;
> +        if (socket_fd < 0) {
> +            LOGED(ERROR, guest_domid, "Failed to create UNIX socket");
> +            return ERROR_FAIL;
> +        }

I can't help thinking that this is a lot of lines of code.  Is none of
this available anywhere else for reuse ?

> +        socket_path = GCSPRINTF("%s/qmp-libxl-%d",
> +                                libxl__run_dir_path(), guest_domid);
> +        if (strlen(socket_path) > sizeof(un.sun_path)) {
> +            LOGD(ERROR, guest_domid, "UNIX socket path '%s' is too long",
> +                 socket_path);
> +            LOGD(DEBUG, guest_domid, "Path must be less than %zu bytes",
> +                 sizeof(un.sun_path));
> +            return ERROR_FAIL;
> +        }

Eg this part maybe ?

Thanks,
Ian.

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

[PATCH v3 31/31] libxl: QEMU startup sync based on QMP

[Anthony PERARD]

This is only activated when dm_restrict=1, as explained in the previous
patch "libxl_dm: Pre-open QMP socket for QEMU"

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_dm.c       |  7 ++++++
 tools/libxl/libxl_exec.c     | 44 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  5 ++++
 3 files changed, 56 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 10b35d822a..4e89e09fc8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2398,6 +2398,13 @@ retry_transaction:
     spawn->failure_cb = device_model_startup_failed;
     spawn->detached_cb = device_model_detached;
 
+    spawn->qmp_domid = INVALID_DOMID;
+    if (dm_monitor_fd >= 0) {
+        /* There is a valid QMP socket available now, have libxl__spawn_spawn
+         * use it to find out when QEMU is ready */
+        spawn->qmp_domid = domid;
+    }
+
     rc = libxl__spawn_spawn(egc, spawn);
     if (rc < 0)
         goto out_close;
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 02e6c917f0..e61297ed2f 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -258,6 +258,9 @@ err:
 /* Event callbacks. */
 static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
                               int rc, const char *xsdata);
+static void spawn_qmp_callback(libxl__egc *egc, libxl__ev_qmp *ev,
+                               const libxl__json_object *response,
+                               libxl__qmp_error_class error);
 static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
                                pid_t pid, int status);
 
@@ -272,6 +275,7 @@ void libxl__spawn_init(libxl__spawn_state *ss)
 {
     libxl__ev_child_init(&ss->mid);
     libxl__xswait_init(&ss->xswait);
+    libxl__ev_qmp_init(&ss->ev_qmp);
 }
 
 int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
@@ -291,6 +295,11 @@ int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *ss)
     ss->xswait.callback = spawn_watch_event;
     rc = libxl__xswait_start(gc, &ss->xswait);
     if (rc) goto out_err;
+    if (ss->qmp_domid != INVALID_DOMID) {
+        rc = libxl__ev_qmp_register(gc, &ss->ev_qmp, spawn_qmp_callback,
+                                    ss->qmp_domid, "query-status", NULL);
+        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 +356,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__ev_qmp_deregister(gc, &ss->ev_qmp);
 }
 
 static void spawn_detach(libxl__gc *gc, libxl__spawn_state *ss)
@@ -359,6 +369,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__ev_qmp_deregister(gc, &ss->ev_qmp);
 
     pid_t child = ss->mid.pid;
     r = kill(child, SIGKILL);
@@ -399,6 +410,39 @@ static void spawn_watch_event(libxl__egc *egc, libxl__xswait_state *xswa,
     ss->confirm_cb(egc, ss, p); /* must be last */
 }
 
+static void spawn_qmp_callback(libxl__egc *egc, libxl__ev_qmp *ev,
+                               const libxl__json_object *response,
+                               libxl__qmp_error_class error)
+{
+    EGC_GC;
+    libxl__spawn_state *ss = CONTAINER_OF(ev, *ss, ev_qmp);
+    const libxl__json_object *o;
+    const char *status;
+
+    if (error) {
+        goto failed;
+    }
+    o = libxl__json_map_get("status", response, JSON_STRING);
+    if (!o) {
+        LOGD(DEBUG, ev->domid, "QMP unexpected response");
+        goto failed;
+    }
+    status = libxl__json_object_get_string(o);
+    if (!strcmp(status, "running")) {
+        /* success */
+    } else {
+        LOGD(DEBUG, ev->domid, "Unexpected QEMU status: %s", status);
+        goto failed;
+    }
+
+    ss->confirm_cb(egc, ss, status); /* must be last */
+    return;
+
+failed:
+    LOGD(ERROR, ev->domid, "QEMU did not start properly");
+    spawn_fail(egc, ss, ERROR_FAIL); /* must be last */
+}
+
 static void spawn_middle_death(libxl__egc *egc, libxl__ev_child *childw,
                                pid_t pid, int status)
     /* On entry, is Attached or Detaching */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 371b27e866..61cc8c9f0c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1603,11 +1603,16 @@ struct libxl__spawn_state {
     libxl__spawn_confirm_cb *confirm_cb;
     libxl__spawn_detached_cb *detached_cb;
 
+    /* If qmp_domid != INVALID_DOMID, then libxl__spawn_spawn will also use QMP
+     * to find out when the process is started */
+    uint32_t qmp_domid;
+
     /* remaining fields are private to libxl_spawn_... */
     int detaching; /* we are in Detaching */
     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__ev_qmp ev_qmp;
 };
 
 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

Re: [PATCH v3 31/31] libxl: QEMU startup sync based on QMP

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3 31/31] libxl: QEMU startup sync based on QMP"):
> This is only activated when dm_restrict=1, as explained in the previous
> patch "libxl_dm: Pre-open QMP socket for QEMU"
...
> @@ -1603,11 +1603,16 @@ struct libxl__spawn_state {
>      libxl__spawn_confirm_cb *confirm_cb;
>      libxl__spawn_detached_cb *detached_cb;
>  
> +    /* If qmp_domid != INVALID_DOMID, then libxl__spawn_spawn will also use QMP
> +     * to find out when the process is started */
> +    uint32_t qmp_domid;
> +

I think this is a layering violation.  libxl__spawn_* is a thing for
double forking and shouldn't know about qmp.  I think you need to
handle this the way the xenstore readiness is handled.

Thanks,
Ian.

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

Re: [PATCH v3 31/31] libxl: QEMU startup sync based on QMP

[Anthony PERARD]

On Wed, Jun 27, 2018 at 05:16:07PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3 31/31] libxl: QEMU startup sync based on QMP"):
> > This is only activated when dm_restrict=1, as explained in the previous
> > patch "libxl_dm: Pre-open QMP socket for QEMU"
> ...
> > @@ -1603,11 +1603,16 @@ struct libxl__spawn_state {
> >      libxl__spawn_confirm_cb *confirm_cb;
> >      libxl__spawn_detached_cb *detached_cb;
> >  
> > +    /* If qmp_domid != INVALID_DOMID, then libxl__spawn_spawn will also use QMP
> > +     * to find out when the process is started */
> > +    uint32_t qmp_domid;
> > +
> 
> I think this is a layering violation.  libxl__spawn_* is a thing for
> double forking and shouldn't know about qmp.

Yes, I think I agree with that now, and I think I can move the QMP
calls to libxl__dm_spawn_*.

> I think you need to
> handle this the way the xenstore readiness is handled.

That is what I actually tried to do... but the way it is done is weird,
both libxl__dm_spawn_* and libxl__spawn_* do some setup of xenstore, the
first one setup some parameters, but the second one does start the
watch for event.

Anyway, I've managed to move the QMP stuff to libxl__dm_spawn_*. Result
in v4.

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH v3 00/31] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*

[Anthony PERARD]

On Fri, Jun 01, 2018 at 03:36:49PM +0100, Anthony PERARD wrote:
> The real meat in this patch series start with patch
> "libxl_qmp_ev: Introduce libxl__ev_qmp_start() to connect to QMP"
> which implement libxl__ev_qmp_* functions to turn the QMP client into
> asynchronous mode.
> 
> This comes with two examples on how to use it:
> * "libxl_disk: Have libxl_cdrom_insert use libxl__ev_qmp"
>   with patches:
>   - "libxl_qmp: Implement libxl__qmp_insert_cdrom_ev"
>   - "libxl_disk: Cut libxl_cdrom_insert into step"
> * "libxl: QEMU startup sync based on QMP"
>   which can use QMP to find out when QEMU as started.
>   this requires: "libxl_dm: Pre-open QMP socket for QEMU"
>   But that only works with dm_restrict=1 as explain in the patch.
> 
> The first few patches do some cleanup and fixes of the current qmp client
> implementation, mostly because it bothered me as I think we should remove the
> current implementation. There is also two patches to allow to save a restricted
> QEMU, but that would need to be converted over to libxl__ev_qmp_*.
> 
> There is still one bug that I haven't fix yet. When creating a guest with
> dm_restrict=1, the call to libxl__qmp_initializations() is going to fail
> because libxl is still connected to the QMP socket. But libxl doesn't care
> about failure, and that just mean that `xl console` will not work and vnc will
> not have any password. save/restore of the same guest will works fine because
> libxl__ev_qmp_* will have an oportunity to disconnect from the socket before
> libxl__qmp_initializations() is called.
> 
> Cheers,

Patches series available in a git tag:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git libxl-migration-fdset-v3

-- 
Anthony PERARD

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

[PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

All the functions will be implemented in later patches.

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

---

What do you think of this design? This is the same as in my patch series
with new names (to avoid confusion with libxl___ev_*) and documentation.

I'll write something as well for the internal of the engine (the QMP
client itself).
---
 tools/libxl/libxl_internal.h         | 98 ++++++++++++++++++++++++++++
 tools/libxl/libxl_types_internal.idl | 14 ++++
 2 files changed, 112 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c582894589..ac5a8a21f2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -357,6 +357,104 @@ struct libxl__ev_child {
 };
 
 
+/*
+ * QMP asynchronous calls
+ */
+
+/*
+ * This struct is used to register one command to send to QEMU with an
+ * associated callback.
+ *
+ * Possible states:
+ *  Undefined
+ *    Might contain anything.
+ *  Idle
+ *    Struct contents are defined enough to pass to any
+ *    libxl__qmp_cmd_* functions but is not registered and callback
+ *    will not be called. The struct does not contain references to
+ *    any allocated resources so can be thrown away.
+ *  Active
+ *    Currently waiting for a response from QEMU, and callback can be
+ *    called. _dispose must be called to reclaim resources.
+ */
+typedef struct libxl__qmp_cmd_state libxl__qmp_cmd_state;
+
+/*
+ * cmd_state:   In Idle state (with value domid available).
+ * response:    QMP response on success, or NULL on error.
+ * error_class: NONE on success, otherwise QMP error class or libxl error.
+ */
+typedef libxl__qmp_cmd_callback(libxl__gc *gc,
+                                libxl__qmp_cmd_state *cmd_state,
+                                const libxl__json_object *response,
+                                libxl__qmp_error_class error_class);
+
+/*
+ * Initialize libxl__qmp_cmd_state.
+ *    Which must be in Undefined or Idle state.
+ *    On return it is Idle.
+ */
+_hidden void libxl__qmp_cmd_init(libxl__qmp_cmd_state *cmd_state);
+
+/*
+ * Register a command to be issued to QEMU.
+ *   On entry cmd_state must be Idle.
+ *   Returns a libxl error code; on error return cmd_state is Idle.
+ *   On successful return cmd_state is Active and callback will be
+ *   called in the future.
+ *   Callback will not be called from within the call to this
+ *   function.
+ *
+ * This function will attempt to connect to the QMP socket if not
+ * already connected. No other communication will be done before the
+ * function returns.
+ *
+ * The callback will be called with the same cmd_state, but the
+ * cmd_state will be Idle.
+ *
+ * When called from within a callback, the same QMP connection will be
+ * reused to execute the new command. This is important in the case
+ * where the first command is "add-fd" and the second command use the
+ * fdset created by QEMU.
+ */
+_hidden int libxl__qmp_cmd_exec(libxl__gc *gc,
+                                libxl__qmp_cmd_state *cmd_state,
+                                libxl__qmp_cmd_callback *callback,
+                                uint32_t domid,
+                                const char *cmd, libxl__json_object *args);
+
+/*
+ * On entry, cmd_state must be in state Active or Idle.
+ * On return it is Idle.
+ */
+_hidden void libxl__qmp_cmd_dispose(libxl__gc *gc,
+                                    libxl__qmp_cmd_state *cmd_state);
+
+struct libxl__qmp_cmd_state {
+    /* read-only once Active and from within the callback */
+    uint32_t domid;
+    libxl__qmp_cmd_callback *callback;
+
+    /* private */
+
+    /*
+     * id == 0: initial state or response already received and callback called.
+     *          State is Idle.
+     * id > 0:  id used to send a command to qemu.
+     *          State is Active.
+     */
+    int id;
+    LIBXL_TAILQ_ENTRY(libxl__ev_qmp) entry;
+
+    /*
+     * This value can be initialise before calling _qmp_cmd_exec. The
+     * file descriptor will sent to QEMU along with the command, then
+     * the fd will be closed.
+     */
+    libxl__carefd *efd;
+};
+
+
 /*
  * evgen structures, which are the state we use for generating
  * events for the caller.
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index f2ff01718d..ada97615d5 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -13,6 +13,20 @@ libxl__qmp_message_type = Enumeration("qmp_message_type", [
     (5, "invalid"),
     ])
 
+libxl__qmp_error_class = Enumeration("qmp_error_class", [
+    # No error
+    (0, "NONE"),
+    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
+    (1, "libxl_error"),
+    # QMP error classes described in QEMU sources code (QapiErrorClass)
+    (2, "GenericError"),
+    (3, "CommandNotFound"),
+    (4, "DeviceNotActive"),
+    (5, "DeviceNotFound"),
+    # Unrecognized QMP error class
+    (6, "Unknown"),
+    ])
+
 libxl__device_kind = Enumeration("device_kind", [
     (0, "NONE"),
     (1, "VIF"),
-- 
Anthony PERARD


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

Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU"):
> All the functions will be implemented in later patches.

Thanks, this really makes things clearer for me.

> What do you think of this design? This is the same as in my patch series
> with new names (to avoid confusion with libxl___ev_*) and documentation.
> 
> I'll write something as well for the internal of the engine (the QMP
> client itself).
...

> +/*
> + * This struct is used to register one command to send to QEMU with an
> + * associated callback.

You still use the word `register' which I don't think is right.  It
makes it sound as if there is a separate `registration' and `sending'.

How about

   This facility allows a command to be sent to qemu, and the response
   to be handed to a callback function.  Each libxl__qmp_cmd_state
   handles zero or one outstanding command; if multiple commands are
   to be sent concurrently, multiple libxl__qmp_cmd_state's must be
   used.

or some such ?

> + * Possible states:
> + *  Undefined
> + *    Might contain anything.
> + *  Idle
> + *    Struct contents are defined enough to pass to any
> + *    libxl__qmp_cmd_* functions but is not registered and callback
> + *    will not be called. The struct does not contain references to
> + *    any allocated resources so can be thrown away.
> + *  Active
> + *    Currently waiting for a response from QEMU, and callback can be
> + *    called. _dispose must be called to reclaim resources.

I don't think this set of states is accurate.  In particular, your API
description (about connection management) implies that there are at
least these states:
  (i) undefined
  (ii) there is no qmp connection open
  (iii) we have sent a command and are expecting a callback
  (iv) there is a qmp connection open, but no command is outstanding

(iv) does not fit into any of the categories above.

> +/*
> + * Initialize libxl__qmp_cmd_state.
> + *    Which must be in Undefined or Idle state.
> + *    On return it is Idle.

You might want to abbreviate this state notation, eg as is done for
libxl__xs_transaction_... .  So here you could just write
       Undefined/Idle -> Idle

> +/*
> + * Register a command to be issued to QEMU.

Again, "register" has been inherited from libxl_ev_*.  I think it
would be clearer to say that this function

     Sends a command to QEMU.

Looking through libxl_internal.h again, I see that there is
libxl__ev_child, which provides another model for handling a thing
which is sort of, but not exactly, like a libxl__ev_FOO.  There, the
struct is called libxl_ev_*, but the actual function names are quite
different.  There is no libxl__ev_child_register, only
libxl__ev_child_fork.  And libxl__ev_child_deregister is entirely
absent.

So you could call your think libxl__ev_qmp but have functions
libxl__ev_qmp_send and libxl__ev_qmp_dispose/destroy.
(and _init of course).

If you do this you need to do like libxl__ev_child_fork does, and
write commentary describing the ways in which a libxl__ev_qmp is not
like most libxl__ev_FOO.

I think I mind less what the struct is called than what the functions
are called.  Your send function should probably be _send or _exec.
The dispose function can be _dispose or _destroy, or similar.

> +struct libxl__qmp_cmd_state {
> +    /* read-only once Active and from within the callback */
> +    uint32_t domid;
> +    libxl__qmp_cmd_callback *callback;

You copied this pattern from libxl__ev_FOO.  I don't object to it.

But in general, I have sometimes found it more convenient to put these
parameters in the struct and expect callers to fill them in.  You are
doing that with the carefd.  Maybe you want to do it with all of
them ?  See libxl__datacopier_start for an example.

Up to you.  I don't object to mixing the two styles within the same
facility provided the internal docs are clear.

> +    /* private */

You should say "private for libxl__qmp_cmd_...".

> +    /*
> +     * This value can be initialise before calling _qmp_cmd_exec. The
> +     * file descriptor will sent to QEMU along with the command, then
> +     * the fd will be closed.
> +     */
> +    libxl__carefd *efd;

Why not
       libxl__carefd efd;
?

Also, I don't think this description of the semantics is right.  The
caller must always somehow arrange to initialise this value.
Presumably _init clears it ?  Certainly this as a parameter to the
operation, this should be up with domid and callback.

Maybe you want comments like the ones in libxl__datacopier_state etc.,
which say /* caller must fill these in */.

And, you probably want to make it clear that the fd remains open in
the libxl process.  (I assume it does.)

> +libxl__qmp_error_class = Enumeration("qmp_error_class", [
> +    # No error
> +    (0, "NONE"),
> +    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
> +    (1, "libxl_error"),
> +    # QMP error classes described in QEMU sources code (QapiErrorClass)
> +    (2, "GenericError"),
> +    (3, "CommandNotFound"),
> +    (4, "DeviceNotActive"),
> +    (5, "DeviceNotFound"),
> +    # Unrecognized QMP error class
> +    (6, "Unknown"),

Are these numbers from qmp ?  Why not assign a bunch of libxl error
values instead ?

I hope this review is helpful.

Thanks,
Ian.

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

Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

On Tue, Jul 03, 2018 at 03:48:22PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU"):
> > All the functions will be implemented in later patches.
> 
> Thanks, this really makes things clearer for me.
> 
> > What do you think of this design? This is the same as in my patch series
> > with new names (to avoid confusion with libxl___ev_*) and documentation.
> > 
> > I'll write something as well for the internal of the engine (the QMP
> > client itself).

Before replying to this email, here is the description of what state a
QMP connection can be. This states are only internal to libxl_qmp.c, the
rest of libxl doesn't need to know.

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1235,6 +1235,97 @@
     return ret;
 }
 
+/* ------------ Implementation of asynchronous QMP calls ------------ */
+
+/*
+ * This are different state possible when the QMP client (libxl) is waiting.
+ * Transition from one state to the other is listed after.
+ *
+ * States:
+ *   Undefined
+ *   Disconnected
+ *      nothing, no allocated ressources
+ *   Connecting
+ *      Have allocated ressources,
+ *      Have connected to the QMP socket,
+ *      Waiting for server greeting.
+ *   Capability Negociation
+ *      QMP server is in Capabilities Negotiation mode.
+ *      Waiting for a response to the "qmp_capabilities" command
+ *   Connected
+ *      QMP server is in command mode,
+ *      commands can be issued
+ *
+ * Here is the transition from one state to the next:
+ *  Disconnected -> Connecting:
+ *    Connect to the QMP socket.
+ *  Connecting -> Capability Negociation
+ *    Server greeting received
+ *    Send "qmp_capabilities"
+ *  Capability Negociation -> Connected
+ *    Response to "qmp_capabilities" received"
+ *
+ * checkout qemu.git:docs/interop/qmp-spec.txt for more information.
+ */


> > +/*
> > + * This struct is used to register one command to send to QEMU with an
> > + * associated callback.
> 
> You still use the word `register' which I don't think is right.  It
> makes it sound as if there is a separate `registration' and `sending'.
> 
> How about
> 
>    This facility allows a command to be sent to qemu, and the response
>    to be handed to a callback function.  Each libxl__qmp_cmd_state
>    handles zero or one outstanding command; if multiple commands are
>    to be sent concurrently, multiple libxl__qmp_cmd_state's must be
>    used.
> 
> or some such ?

That's looks better.

> > + * Possible states:
> > + *  Undefined
> > + *    Might contain anything.
> > + *  Idle
> > + *    Struct contents are defined enough to pass to any
> > + *    libxl__qmp_cmd_* functions but is not registered and callback
> > + *    will not be called. The struct does not contain references to
> > + *    any allocated resources so can be thrown away.
> > + *  Active
> > + *    Currently waiting for a response from QEMU, and callback can be
> > + *    called. _dispose must be called to reclaim resources.
> 
> I don't think this set of states is accurate.  In particular, your API
> description (about connection management) implies that there are at
> least these states:
>   (i) undefined
>   (ii) there is no qmp connection open
>   (iii) we have sent a command and are expecting a callback
>   (iv) there is a qmp connection open, but no command is outstanding
> 
> (iv) does not fit into any of the categories above.

I don't think the state of a qmp connection can fit into
libxl__qmp_cmd_state. The "Active" state doesn't mean that a qmp
connection is open or that the command have been sent. It merely mean
that the syscall socket(3) and connect(3) have return successfully, and
there will be an attempt later to sent the command to qemu.

> > +/*
> > + * Initialize libxl__qmp_cmd_state.
> > + *    Which must be in Undefined or Idle state.
> > + *    On return it is Idle.
> 
> You might want to abbreviate this state notation, eg as is done for
> libxl__xs_transaction_... .  So here you could just write
>        Undefined/Idle -> Idle

Will do.

> > +/*
> > + * Register a command to be issued to QEMU.
> 
> Again, "register" has been inherited from libxl_ev_*.  I think it
> would be clearer to say that this function
> 
>      Sends a command to QEMU.

The API I'm describing haven't send anything to QEMU yet, the command is
merely put in a queue, and an attempt to send it will be done later,
when QEMU is ready to receive commands and when the socket is not
blocked.

> Looking through libxl_internal.h again, I see that there is
> libxl__ev_child, which provides another model for handling a thing
> which is sort of, but not exactly, like a libxl__ev_FOO.  There, the
> struct is called libxl_ev_*, but the actual function names are quite
> different.  There is no libxl__ev_child_register, only
> libxl__ev_child_fork.  And libxl__ev_child_deregister is entirely
> absent.
> 
> So you could call your think libxl__ev_qmp but have functions
> libxl__ev_qmp_send and libxl__ev_qmp_dispose/destroy.
> (and _init of course).
> 
> If you do this you need to do like libxl__ev_child_fork does, and
> write commentary describing the ways in which a libxl__ev_qmp is not
> like most libxl__ev_FOO.
> 
> I think I mind less what the struct is called than what the functions
> are called.  Your send function should probably be _send or _exec.
> The dispose function can be _dispose or _destroy, or similar.

I look into that.

> > +struct libxl__qmp_cmd_state {
> > +    /* read-only once Active and from within the callback */
> > +    uint32_t domid;
> > +    libxl__qmp_cmd_callback *callback;
> 
> You copied this pattern from libxl__ev_FOO.  I don't object to it.
> 
> But in general, I have sometimes found it more convenient to put these
> parameters in the struct and expect callers to fill them in.  You are
> doing that with the carefd.  Maybe you want to do it with all of
> them ?  See libxl__datacopier_start for an example.
> 
> Up to you.  I don't object to mixing the two styles within the same
> facility provided the internal docs are clear.

I've done the carefd differently because I didn't know how to put it
into the function parameters, beside having a new function. But your
sugestion about having the caller fill the struct sound good. I'll do
that. That way, a caller will know in which states are the different
fields.

> > +    /* private */
> 
> You should say "private for libxl__qmp_cmd_...".

Will do.

> > +    /*
> > +     * This value can be initialise before calling _qmp_cmd_exec. The
> > +     * file descriptor will sent to QEMU along with the command, then
> > +     * the fd will be closed.
> > +     */
> > +    libxl__carefd *efd;
> 
> Why not
>        libxl__carefd efd;
> ?

The libxl__carefd_* API returns a newly allocated efd, a pointer. I
can't preallocate it.

> Also, I don't think this description of the semantics is right.  The
> caller must always somehow arrange to initialise this value.
> Presumably _init clears it ?  Certainly this as a parameter to the
> operation, this should be up with domid and callback.
> 
> Maybe you want comments like the ones in libxl__datacopier_state etc.,
> which say /* caller must fill these in */.
> 
> And, you probably want to make it clear that the fd remains open in
> the libxl process.  (I assume it does.)

Well I was closing the fd once it was sent to QEMU. But we can have the
callbacks takes care of closing it instead.

> > +libxl__qmp_error_class = Enumeration("qmp_error_class", [
> > +    # No error
> > +    (0, "NONE"),
> > +    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
> > +    (1, "libxl_error"),
> > +    # QMP error classes described in QEMU sources code (QapiErrorClass)
> > +    (2, "GenericError"),
> > +    (3, "CommandNotFound"),
> > +    (4, "DeviceNotActive"),
> > +    (5, "DeviceNotFound"),
> > +    # Unrecognized QMP error class
> > +    (6, "Unknown"),
> 
> Are these numbers from qmp ?  Why not assign a bunch of libxl error
> values instead ?

No, these are strings from QEMU, numbers doesn't matter. Also I don't
know how well those are going to fit into libxl errors.

FYI, here is the description of the different errors generated by QEMU:

(qemu.git:qapi/common.json)
# @QapiErrorClass:
#
# QEMU error classes
#
# @GenericError: this is used for errors that don't require a specific error
#                class. This should be the default case for most errors
#
# @CommandNotFound: the requested command has not been found
#
# @DeviceNotActive: a device has failed to be become active
#
# @DeviceNotFound: the requested device has not been found

QEMU always associate an error messages when it send an error, so the
only thing t odo with GenericError is to log that message.

> I hope this review is helpful.
> 
> Thanks,
> Ian.

-- 
Anthony PERARD

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

Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Ian Jackson]

Sorry for not noticing that you had replied.

Anthony PERARD writes ("Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU"):
> Before replying to this email, here is the description of what state a
> QMP connection can be. This states are only internal to libxl_qmp.c, the
> rest of libxl doesn't need to know.
...
> +/*
> + * This are different state possible when the QMP client (libxl) is waiting.
> + * Transition from one state to the other is listed after.

These are the states of some internal libxl_qmp_something struct ?

> + * States:
> + *   Undefined
> + *   Disconnected
> + *      nothing, no allocated ressources
> + *   Connecting
> + *      Have allocated ressources,
> + *      Have connected to the QMP socket,
> + *      Waiting for server greeting.
> + *   Capability Negociation
> + *      QMP server is in Capabilities Negotiation mode.
> + *      Waiting for a response to the "qmp_capabilities" command
> + *   Connected
> + *      QMP server is in command mode,
> + *      commands can be issued

This looks like a plausible set of states.

> > > + * Possible states:
> > > + *  Undefined
> > > + *    Might contain anything.
> > > + *  Idle
> > > + *    Struct contents are defined enough to pass to any
> > > + *    libxl__qmp_cmd_* functions but is not registered and callback
> > > + *    will not be called. The struct does not contain references to
> > > + *    any allocated resources so can be thrown away.
> > > + *  Active
> > > + *    Currently waiting for a response from QEMU, and callback can be
> > > + *    called. _dispose must be called to reclaim resources.
> > 
> > I don't think this set of states is accurate.  In particular, your API
> > description (about connection management) implies that there are at
> > least these states:
> >   (i) undefined
> >   (ii) there is no qmp connection open
> >   (iii) we have sent a command and are expecting a callback
> >   (iv) there is a qmp connection open, but no command is outstanding
> > 
> > (iv) does not fit into any of the categories above.
> 
> I don't think the state of a qmp connection can fit into
> libxl__qmp_cmd_state. The "Active" state doesn't mean that a qmp
> connection is open or that the command have been sent. It merely mean
> that the syscall socket(3) and connect(3) have return successfully, and
> there will be an attempt later to sent the command to qemu.

I think you haven't quite understood my point.

My understanding of your API is that it gives the user of
libl__qmp_cmd a certain amount of visibility of the existence or
otherwise of the qmp connection.

You say:

  + * When called from within a callback, the same QMP connection will be
  + * reused to execute the new command. This is important in the case
  + * where the first command is "add-fd" and the second command use the
  + * fdset created by QEMU.

This implies that at the top of the callback, the qmp connection is
actually in some kind of extra state which is not covered by any of
your Undefined, Idle or Active.

It is not Undefined, obviously.

It is not Active because no response is being awaited any longer.
(If a response were being awaited, then it would not be sensible for
the callback to issue another command, because your rule is one
command at once.)

And it is not Idle because it contains references to allocated
resources - namely, the qmp connection fd.

So your documented state model is too poor to express what is going
on.  You need to write down at least one additional state, which you
might call `Connected'.


Also, I have just noticed that you say "from within a callback".  That
suggests that the code which makes the callback does something to the
libxl__qmp_state after after the callback returns.

That is contrary to the way that everything else works in libxl.  In
libxl, such a callback is made as the last thing before returning back
to the event loop.

The reason is that the state struct (in this case the libxl__qmp_cmd)
may be part of a larger struct, which is completed and deallocated
somewhere in the series of (nested) callback functions.  So the memory
may not be valid any more.


Another way to look at this is that you actually have a fourth state
which I will call WithinCallback.  On entry to the callback, the cmd
is in WithinCallback state.

In WithinCallback state, it is allowed to request that another command
is sent (putting the cmd back into Active).

But in the WithinCallback state, the libxl__qmp_cmd contains
references to resources and may not be freed.  Furthermore, as I read
your commentary, the WithinCallback state cannot be exited other than
to Active, or by returning from the callback.

That would make it very awkward for the user of this interface to ever
free the cmd.  After all, they can only free the memory for it when
the state is Idle or Undefined.  So they need to get it into Idle
which they can only do by returning, but then they have lost
control...


So this is why I say you should have a proper fourth state, which we
can call Connected or something.  On entry to the callback, the cmd is
in state Connected.  The cmd should stay Connected until it is
explicitly disposed of.

The code which makes the callback then does not need to do anything
after making the callback: the user has sent another command; or is
going to send another command; or has called _dispose.  In any case,
the caller has ownership.


> > > +/*
> > > + * Register a command to be issued to QEMU.
> > 
> > Again, "register" has been inherited from libxl_ev_*.  I think it
> > would be clearer to say that this function
> > 
> >      Sends a command to QEMU.
> 
> The API I'm describing haven't send anything to QEMU yet, the command is
> merely put in a queue, and an attempt to send it will be done later,
> when QEMU is ready to receive commands and when the socket is not
> blocked.

Yes.  But from the caller's point of view, those are implementation
details.  How about:

   Sends a command to QEMU (connecting, etc., as needed)

?

> > Up to you.  I don't object to mixing the two styles within the same
> > facility provided the internal docs are clear.
> 
> I've done the carefd differently because I didn't know how to put it
> into the function parameters, beside having a new function. But your
> sugestion about having the caller fill the struct sound good. I'll do
> that. That way, a caller will know in which states are the different
> fields.

Right.  Do put that in comments too :-).

> > > +    /*
> > > +     * This value can be initialise before calling _qmp_cmd_exec. The
> > > +     * file descriptor will sent to QEMU along with the command, then
> > > +     * the fd will be closed.
> > > +     */
> > > +    libxl__carefd *efd;
> > 
> > Why not
> >        libxl__carefd efd;
> > ?
> 
> The libxl__carefd_* API returns a newly allocated efd, a pointer. I
> can't preallocate it.

So it does.  Sorry!

> > Also, I don't think this description of the semantics is right.  The
> > caller must always somehow arrange to initialise this value.
> > Presumably _init clears it ?  Certainly this as a parameter to the
> > operation, this should be up with domid and callback.
> > 
> > Maybe you want comments like the ones in libxl__datacopier_state etc.,
> > which say /* caller must fill these in */.
> > 
> > And, you probably want to make it clear that the fd remains open in
> > the libxl process.  (I assume it does.)
> 
> Well I was closing the fd once it was sent to QEMU. But we can have the
> callbacks takes care of closing it instead.

I don't mind what the semantics are, but they should be clear, and all
the error cases need to work correctly.  Eg if asking to send a fd
causes the fd to become owned by the qmp machiner, then if the attempt
to send the qmp command fails (maybe becaue the qmp connection fails)
then it must be closed.

The semantics should probably be whichever are more convenient.
Personally i would lean towards qmp_cmd not taking ownership, because
if you do then someone who wants to keep a copy of the fd has to dup
it and duping a carefd is quite a faff.

> > > +libxl__qmp_error_class = Enumeration("qmp_error_class", [
> > > +    # No error
> > > +    (0, "NONE"),
> > > +    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
> > > +    (1, "libxl_error"),
> > > +    # QMP error classes described in QEMU sources code (QapiErrorClass)
> > > +    (2, "GenericError"),
> > > +    (3, "CommandNotFound"),
> > > +    (4, "DeviceNotActive"),
> > > +    (5, "DeviceNotFound"),
> > > +    # Unrecognized QMP error class
> > > +    (6, "Unknown"),
> > 
> > Are these numbers from qmp ?  Why not assign a bunch of libxl error
> > values instead ?
> 
> No, these are strings from QEMU, numbers doesn't matter. Also I don't
> know how well those are going to fit into libxl errors.

I meant, invent a libxl error number (and corresponding ERROR_QMP_BLAH
or whatever) for each one of these qmp errors.

> (qemu.git:qapi/common.json)
> # @QapiErrorClass:
> #
> # QEMU error classes
> #
> # @GenericError: this is used for errors that don't require a specific error
> #                class. This should be the default case for most errors
> #
> # @CommandNotFound: the requested command has not been found
> #
> # @DeviceNotActive: a device has failed to be become active
> #
> # @DeviceNotFound: the requested device has not been found
> 
> QEMU always associate an error messages when it send an error, so the
> only thing t odo with GenericError is to log that message.

I guess you should alwyas log the error anyway.  But discriminating
the different qmp errors will probably be useful ?

Ian.

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

Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

On Thu, Jul 12, 2018 at 05:36:00PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v3.1] libxl: Design of an async API to issue QMP commands to QEMU"):
> > I don't think the state of a qmp connection can fit into
> > libxl__qmp_cmd_state. The "Active" state doesn't mean that a qmp
> > connection is open or that the command have been sent. It merely mean
> > that the syscall socket(3) and connect(3) have return successfully, and
> > there will be an attempt later to sent the command to qemu.
> 
> I think you haven't quite understood my point.
> 
> My understanding of your API is that it gives the user of
> libl__qmp_cmd a certain amount of visibility of the existence or
> otherwise of the qmp connection.
> 
> You say:
> 
>   + * When called from within a callback, the same QMP connection will be
>   + * reused to execute the new command. This is important in the case
>   + * where the first command is "add-fd" and the second command use the
>   + * fdset created by QEMU.
> 
> This implies that at the top of the callback, the qmp connection is
> actually in some kind of extra state which is not covered by any of
> your Undefined, Idle or Active.
> 
> It is not Undefined, obviously.
> 
> It is not Active because no response is being awaited any longer.
> (If a response were being awaited, then it would not be sensible for
> the callback to issue another command, because your rule is one
> command at once.)
> 
> And it is not Idle because it contains references to allocated
> resources - namely, the qmp connection fd.
> 
> So your documented state model is too poor to express what is going
> on.  You need to write down at least one additional state, which you
> might call `Connected'.
> 
> 
> Also, I have just noticed that you say "from within a callback".  That
> suggests that the code which makes the callback does something to the
> libxl__qmp_state after after the callback returns.
> 
> That is contrary to the way that everything else works in libxl.  In
> libxl, such a callback is made as the last thing before returning back
> to the event loop.
> 
> The reason is that the state struct (in this case the libxl__qmp_cmd)
> may be part of a larger struct, which is completed and deallocated
> somewhere in the series of (nested) callback functions.  So the memory
> may not be valid any more.
> 
> 
> Another way to look at this is that you actually have a fourth state
> which I will call WithinCallback.  On entry to the callback, the cmd
> is in WithinCallback state.
> 
> In WithinCallback state, it is allowed to request that another command
> is sent (putting the cmd back into Active).
> 
> But in the WithinCallback state, the libxl__qmp_cmd contains
> references to resources and may not be freed.  Furthermore, as I read
> your commentary, the WithinCallback state cannot be exited other than
> to Active, or by returning from the callback.
> 
> That would make it very awkward for the user of this interface to ever
> free the cmd.  After all, they can only free the memory for it when
> the state is Idle or Undefined.  So they need to get it into Idle
> which they can only do by returning, but then they have lost
> control...
> 
> 
> So this is why I say you should have a proper fourth state, which we
> can call Connected or something.  On entry to the callback, the cmd is
> in state Connected.  The cmd should stay Connected until it is
> explicitly disposed of.
> 
> The code which makes the callback then does not need to do anything
> after making the callback: the user has sent another command; or is
> going to send another command; or has called _dispose.  In any case,
> the caller has ownership.

I'll reply to that with a new patch which I hope will anwser your
comments.


> > > Also, I don't think this description of the semantics is right.  The
> > > caller must always somehow arrange to initialise this value.
> > > Presumably _init clears it ?  Certainly this as a parameter to the
> > > operation, this should be up with domid and callback.
> > > 
> > > Maybe you want comments like the ones in libxl__datacopier_state etc.,
> > > which say /* caller must fill these in */.
> > > 
> > > And, you probably want to make it clear that the fd remains open in
> > > the libxl process.  (I assume it does.)
> > 
> > Well I was closing the fd once it was sent to QEMU. But we can have the
> > callbacks takes care of closing it instead.
> 
> I don't mind what the semantics are, but they should be clear, and all
> the error cases need to work correctly.  Eg if asking to send a fd
> causes the fd to become owned by the qmp machiner, then if the attempt
> to send the qmp command fails (maybe becaue the qmp connection fails)
> then it must be closed.
> 
> The semantics should probably be whichever are more convenient.
> Personally i would lean towards qmp_cmd not taking ownership, because
> if you do then someone who wants to keep a copy of the fd has to dup
> it and duping a carefd is quite a faff.

Sounds good, I'll do it this way (the caller will keep ownership).

> > > > +libxl__qmp_error_class = Enumeration("qmp_error_class", [
> > > > +    # No error
> > > > +    (0, "NONE"),
> > > > +    # Error generated by libxl (e.g. socket closed unexpectedly, no mem, ...)
> > > > +    (1, "libxl_error"),
> > > > +    # QMP error classes described in QEMU sources code (QapiErrorClass)
> > > > +    (2, "GenericError"),
> > > > +    (3, "CommandNotFound"),
> > > > +    (4, "DeviceNotActive"),
> > > > +    (5, "DeviceNotFound"),
> > > > +    # Unrecognized QMP error class
> > > > +    (6, "Unknown"),
> > > 
> > > Are these numbers from qmp ?  Why not assign a bunch of libxl error
> > > values instead ?
> > 
> > No, these are strings from QEMU, numbers doesn't matter. Also I don't
> > know how well those are going to fit into libxl errors.
> 
> I meant, invent a libxl error number (and corresponding ERROR_QMP_BLAH
> or whatever) for each one of these qmp errors.

That sounds good, I'll invent new libxl error numbers.

> > (qemu.git:qapi/common.json)
> > # @QapiErrorClass:
> > #
> > # QEMU error classes
> > #
> > # @GenericError: this is used for errors that don't require a specific error
> > #                class. This should be the default case for most errors
> > #
> > # @CommandNotFound: the requested command has not been found
> > #
> > # @DeviceNotActive: a device has failed to be become active
> > #
> > # @DeviceNotFound: the requested device has not been found
> > 
> > QEMU always associate an error messages when it send an error, so the
> > only thing t odo with GenericError is to log that message.
> 
> I guess you should alwyas log the error anyway.  But discriminating
> the different qmp errors will probably be useful ?

I do think both DeviceNotActive and DeviceNotFound can be usefull to
know to a caller, and in anycase, I will keep logging error messages
from qemu.

Thanks,

-- 
Anthony PERARD

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

[PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

All the functions will be implemented in later patches.

This patch includes the API that libxl can use to send QMP commands to
QEMU.

This patch also include a description of the internal states of the QMP
client in charge of connecting to QEMU, sending command and handling
responces.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_internal.h |  58 ++++++++++++++++++++
 tools/libxl/libxl_qmp.c      | 102 +++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl  |   4 ++
 3 files changed, 164 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2a1fb784ec..7c88f4c3b5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -357,6 +357,64 @@ struct libxl__ev_child {
 };
 
 
+/*
+ * QMP asynchronous calls
+ */
+
+/*
+ * This facility allows a command to be sent to QEMU, and the response to be
+ * handed to a callback function.  Each libxl__ev_qmp handles zero or one
+ * outstanding command; if multiple commands are to be sent concurrently,
+ * multiple libxl__ev_qmp's must be used.
+ *
+ * Possible states:
+ *  Undefined
+ *    Might contain anything.
+ *  Idle
+ *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
+ *    functions.
+ *    The struct does not contain references to any allocated private resources
+ *    so can be thrown away.
+ *  Active
+ *    Currently waiting for the callback to be called.
+ *    _dispose must be called to reclaim resources (or wait for the callback to
+ *    be called).
+ *
+ * libxl__ev_qmp_init: Undefined/Idle -> Idle
+ *
+ * libxl__ev_qmp_send: Idle -> Active (on error: Idle)
+ *    Sends a command to QEMU.
+ *
+ * libxl__ev_qmp_dispose: Active/Idle -> Idle
+ *
+ * callback:
+ *    When called, ev is Idle, so can be reused or thrown away.
+ *    When an error occured, it is called with response == NULL and the error
+ *    code in rc.
+ */
+typedef struct libxl__ev_qmp libxl__ev_qmp;
+typedef libxl__ev_qmp_callback(libxl__gc *gc, libxl__ev_qmp *ev,
+                               const libxl__json_object *response,
+                               int rc);
+
+_hidden void libxl__ev_qmp_init(libxl__ev_qmp *ev);
+_hidden int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
+                               const char *cmd, libxl__json_object *args);
+_hidden void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev);
+
+struct libxl__ev_qmp {
+    /* caller should include this in their own struct */
+    /* caller must fill these in, and they must all remain valid */
+    uint32_t domid;
+    libxl__ev_qmp_callback *callback;
+    libxl__carefd *efd; /* set to send a fd with the command, NULL otherwise */
+
+    /* remaining fields are private to libxl_ev_qmp_* */
+    int id;
+    LIBXL_TAILQ_ENTRY(libxl__ev_qmp) entry;
+};
+
+
 /*
  * evgen structures, which are the state we use for generating
  * events for the caller.
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 760f2798c7..1e6fbb64a5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1237,6 +1237,108 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
     return ret;
 }
 
+/* ------------ Implementation of asynchronous QMP calls ------------ */
+
+/*
+ * Implementation of the QMP client.
+ *
+ * This are different state possible in which the client can be in, with the
+ * list of possible transition listed after.
+ *
+ * States:
+ *   Disconnected
+ *      Nothing, no allocated ressources.
+ *   Connecting
+ *      Have allocated ressources, including a connection to a QMP socket.
+ *      Waiting for server greeting.
+ *   Capability Negociation
+ *      QMP server is in Capabilities Negotiation mode.
+ *      Waiting for a response to the "qmp_capabilities" command.
+ *   Connected
+ *      QMP server is in command mode, commands can be issued.
+ *      There is outstanding command to be sent and/or there are in-flight
+ *      libxl_ev_qmp with callback.
+ *   Within A Callback
+ *      The QMP client enter this state when a callback is called.
+ *      The connection to QEMU is kept open until at least the callback
+ *      return.
+ *
+ * Here is the transition from one state to the next:
+ *    Disconnected -> Connecting
+ *      Connect to the QMP socket.
+ *    Connecting -> Capability Negociation
+ *      Server greeting received
+ *      Send "qmp_capabilities"
+ *    Capability Negociation -> Connected
+ *      Response to "qmp_capabilities" received"
+ *
+ *    Connected -> Within A Callback
+ *      When a response is received, if there's a callback associated to it,
+ *      it is called.
+ *    Connected -> Disconnected
+ *      When both list of in-flight event and command to send are empty.
+ *
+ *    Within A Callback -> Connected
+ *      On return from a libxl__ev_qmp callback, there are more command to send
+ *      or there are other in-flight event.
+ *    Within A Callback -> Disconnected
+ *      On return from a libxl__ev_qmp callback, both list of command to send
+ *      and list of in-flight event are empty.
+ *      QMP socket connection is closed, all ressources are deallocated.
+ *
+ *   * -> Disconnected
+ *      Whenever a error occure with the QMP socket, all outstanding callback
+ *      will be called with an error, all ressource will be deallocated.
+ *
+ *
+ * checkout qemu.git:docs/interop/qmp-spec.txt for more information on the
+ * qmp protocol.
+ */
+
+typedef enum qmp_states {
+    qmp_state_disconnected = 1,
+    qmp_state_connecting,
+    qmp_state_capability_negociation,
+    qmp_state_connected,
+    qmp_state_within_callback,
+} qmp_states;
+
+typedef struct qmp_tx_buf qmp_tx_buf;
+struct qmp_tx_buf {
+    char *buf;
+    size_t len;
+    /* File descriptor to send along the command */
+    libxl__carefd *efd;
+    LIBXL_TAILQ_ENTRY(qmp_tx_buf) entry;
+};
+
+struct qmp_state {
+    libxl__carefd *cfd;
+    libxl__ev_fd efd;
+    uint32_t domid;
+
+    /* Current state */
+    qmp_states state;
+
+    /* id associated with the last generated command */
+    unsigned int last_id_used;
+
+    /* receive buffer, with:
+     * buf_size: current allocated size,
+     * buf_used: actual data in the buffer,
+     * buf_consumed: data already parsed.  */
+    char *rx_buf;
+    size_t buf_size;
+    size_t buf_used;
+    size_t buf_consumed;
+
+    /* List of buffers to send */
+    LIBXL_TAILQ_HEAD(qmp_tx_bufs, qmp_tx_buf) tx_buf;
+
+    /* List of in-flight events */
+    LIBXL_SLIST_HEAD(libxl__ev_qmps, libxl__ev_qmp) qmp_events;
+};
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 01ec1d1afa..bb2ea253f6 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -69,6 +69,10 @@ libxl_error = Enumeration("error", [
     (-23, "NOTFOUND"),
     (-24, "DOMAIN_DESTROYED"), # Target domain ceased to exist during op
     (-25, "FEATURE_REMOVED"), # For functionality that has been removed
+    (-26, "QMP_GENERIC_ERROR"), # unspecified qmp error
+    (-27, "QMP_COMMAND_NOT_FOUND"), # the requested command has not been found
+    (-28, "QMP_DEVICE_NOT_ACTIVE"), # a device has failed to be become active
+    (-29, "QMP_DEVICE_NOT_FOUND"), # the requested device has not been found
     ], value_namespace = "")
 
 libxl_domain_type = Enumeration("domain_type", [
-- 
Anthony PERARD


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

Re: [PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

On Mon, Jul 16, 2018 at 04:28:00PM +0100, Anthony PERARD wrote:
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 760f2798c7..1e6fbb64a5 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1237,6 +1237,108 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
>      return ret;
>  }
>  
> +/* ------------ Implementation of asynchronous QMP calls ------------ */
> +
> +/*
> + * Implementation of the QMP client.
> + *

Here, I wanted to add more description, but forgot to commit before to
sent the patch, it should read:

+ * This struct qmp_state is used by the libxl__ev_qmp_* functions, but it is
+ * not visible to users of libxl__ev_qmp_*. It is allocated as needed and
+ * stored in CTX in order to allow reuse of a same QMP connection between
+ * different users.
+ *


> + * This are different state possible in which the client can be in, with the
> + * list of possible transition listed after.
> + *
> + * States:
> + *   Disconnected
> + *      Nothing, no allocated ressources.
> + *   Connecting
> + *      Have allocated ressources, including a connection to a QMP socket.
> + *      Waiting for server greeting.
> + *   Capability Negociation
> + *      QMP server is in Capabilities Negotiation mode.
> + *      Waiting for a response to the "qmp_capabilities" command.
> + *   Connected
> + *      QMP server is in command mode, commands can be issued.
> + *      There is outstanding command to be sent and/or there are in-flight
> + *      libxl_ev_qmp with callback.
> + *   Within A Callback
> + *      The QMP client enter this state when a callback is called.
> + *      The connection to QEMU is kept open until at least the callback
> + *      return.
> + *
> + * Here is the transition from one state to the next:
> + *    Disconnected -> Connecting
> + *      Connect to the QMP socket.
> + *    Connecting -> Capability Negociation
> + *      Server greeting received
> + *      Send "qmp_capabilities"
> + *    Capability Negociation -> Connected
> + *      Response to "qmp_capabilities" received"
> + *
> + *    Connected -> Within A Callback
> + *      When a response is received, if there's a callback associated to it,
> + *      it is called.
> + *    Connected -> Disconnected
> + *      When both list of in-flight event and command to send are empty.
> + *
> + *    Within A Callback -> Connected
> + *      On return from a libxl__ev_qmp callback, there are more command to send
> + *      or there are other in-flight event.
> + *    Within A Callback -> Disconnected
> + *      On return from a libxl__ev_qmp callback, both list of command to send
> + *      and list of in-flight event are empty.
> + *      QMP socket connection is closed, all ressources are deallocated.
> + *
> + *   * -> Disconnected
> + *      Whenever a error occure with the QMP socket, all outstanding callback
> + *      will be called with an error, all ressource will be deallocated.
> + *
> + *
> + * checkout qemu.git:docs/interop/qmp-spec.txt for more information on the
> + * qmp protocol.
> + */
> +
> +typedef enum qmp_states {
> +    qmp_state_disconnected = 1,
> +    qmp_state_connecting,
> +    qmp_state_capability_negociation,
> +    qmp_state_connected,
> +    qmp_state_within_callback,
> +} qmp_states;
> +
> +typedef struct qmp_tx_buf qmp_tx_buf;
> +struct qmp_tx_buf {
> +    char *buf;
> +    size_t len;
> +    /* File descriptor to send along the command */
> +    libxl__carefd *efd;
> +    LIBXL_TAILQ_ENTRY(qmp_tx_buf) entry;
> +};
> +
> +struct qmp_state {
> +    libxl__carefd *cfd;
> +    libxl__ev_fd efd;
> +    uint32_t domid;
> +
> +    /* Current state */
> +    qmp_states state;


-- 
Anthony PERARD

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

Re: [PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU [and 1 more messages]

[Ian Jackson]

Anthony PERARD writes ("[PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU"):
> All the functions will be implemented in later patches.
> 
> This patch includes the API that libxl can use to send QMP commands to
> QEMU.
...
> + * This facility allows a command to be sent to QEMU, and the response to be
> + * handed to a callback function.  Each libxl__ev_qmp handles zero or one
> + * outstanding command; if multiple commands are to be sent concurrently,
> + * multiple libxl__ev_qmp's must be used.
> + *
> + * Possible states:
> + *  Undefined
> + *    Might contain anything.
> + *  Idle
> + *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
> + *    functions.
> + *    The struct does not contain references to any allocated private resources
> + *    so can be thrown away.
> + *  Active
> + *    Currently waiting for the callback to be called.
> + *    _dispose must be called to reclaim resources (or wait for the callback to
> + *    be called).
> + *
> + * libxl__ev_qmp_init: Undefined/Idle -> Idle
> + *
> + * libxl__ev_qmp_send: Idle -> Active (on error: Idle)
> + *    Sends a command to QEMU.
> + *
> + * libxl__ev_qmp_dispose: Active/Idle -> Idle
> + *
> + * callback:
> + *    When called, ev is Idle, so can be reused or thrown away.
> + *    When an error occured, it is called with response == NULL and the error
> + *    code in rc.

You have removed this text:

  + * When called from within a callback, the same QMP connection will be
  + * reused to execute the new command. This is important in the case
  + * where the first command is "add-fd" and the second command use the
  + * fdset created by QEMU.

That removes the need for a fourth state, as I discussed earlier.  But
does this not introduce the problem that this text was addressing: Ie
there is no way for the user of libxl_ev_qmp to ensure that the
commands to add-fd, and use the fdset, occur on the same qmp
connection.

What has changed since you wrote that ?  Or have I misunderstood
something ?

NB that I haven't yet read in detail the part about describing the
implementation, ie the qmp_state but I think this problem exists no
matter what that implementation looks like...

Anthony PERARD writes ("Re: [PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU"):
> On Mon, Jul 16, 2018 at 04:28:00PM +0100, Anthony PERARD wrote:
> > + * Implementation of the QMP client.
> > + *
> 
> Here, I wanted to add more description, but forgot to commit before to
> sent the patch, it should read:
> 
> + * This struct qmp_state is used by the libxl__ev_qmp_* functions, but it is
> + * not visible to users of libxl__ev_qmp_*. It is allocated as needed and
> + * stored in CTX in order to allow reuse of a same QMP connection between
> + * different users.

This makes sense, thanks.

Ian.

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

Re: [PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU [and 1 more messages]

[Anthony PERARD]

On Mon, Jul 16, 2018 at 06:04:30PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU"):
> > All the functions will be implemented in later patches.
> > 
> > This patch includes the API that libxl can use to send QMP commands to
> > QEMU.
> ...
> > + * This facility allows a command to be sent to QEMU, and the response to be
> > + * handed to a callback function.  Each libxl__ev_qmp handles zero or one
> > + * outstanding command; if multiple commands are to be sent concurrently,
> > + * multiple libxl__ev_qmp's must be used.
> > + *
> > + * Possible states:
> > + *  Undefined
> > + *    Might contain anything.
> > + *  Idle
> > + *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
> > + *    functions.
> > + *    The struct does not contain references to any allocated private resources
> > + *    so can be thrown away.
> > + *  Active
> > + *    Currently waiting for the callback to be called.
> > + *    _dispose must be called to reclaim resources (or wait for the callback to
> > + *    be called).
> > + *
> > + * libxl__ev_qmp_init: Undefined/Idle -> Idle
> > + *
> > + * libxl__ev_qmp_send: Idle -> Active (on error: Idle)
> > + *    Sends a command to QEMU.
> > + *
> > + * libxl__ev_qmp_dispose: Active/Idle -> Idle
> > + *
> > + * callback:
> > + *    When called, ev is Idle, so can be reused or thrown away.
> > + *    When an error occured, it is called with response == NULL and the error
> > + *    code in rc.
> 
> You have removed this text:
> 
>   + * When called from within a callback, the same QMP connection will be
>   + * reused to execute the new command. This is important in the case
>   + * where the first command is "add-fd" and the second command use the
>   + * fdset created by QEMU.
> 
> That removes the need for a fourth state, as I discussed earlier.  But
> does this not introduce the problem that this text was addressing: Ie
> there is no way for the user of libxl_ev_qmp to ensure that the
> commands to add-fd, and use the fdset, occur on the same qmp
> connection.
> 
> What has changed since you wrote that ?  Or have I misunderstood
> something ?
> 
> NB that I haven't yet read in detail the part about describing the
> implementation, ie the qmp_state but I think this problem exists no
> matter what that implementation looks like...

Please do read the implementation description, it should explain why
there is no fourth state.

Thanks.

> Anthony PERARD writes ("Re: [PATCH v3.2] libxl: Design of an async API to issue QMP commands to QEMU"):
> > On Mon, Jul 16, 2018 at 04:28:00PM +0100, Anthony PERARD wrote:
> > > + * Implementation of the QMP client.
> > > + *
> > 
> > Here, I wanted to add more description, but forgot to commit before to
> > sent the patch, it should read:
> > 
> > + * This struct qmp_state is used by the libxl__ev_qmp_* functions, but it is
> > + * not visible to users of libxl__ev_qmp_*. It is allocated as needed and
> > + * stored in CTX in order to allow reuse of a same QMP connection between
> > + * different users.
> 
> This makes sense, thanks.

-- 
Anthony PERARD

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