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

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

[Anthony PERARD]

Changes in v5:
    Plenty of patch have been applied.
    Other changes mostly are coding style and other typos.
    Some bug fixes.
    Details can be found in patch notes.

    I have left aside the change to cdrom_insert until I can found what to do
    with the userdata lock.

Changes in v4:
    Better API which meant a lot of other changes.

In order for libxl to be able to manage QEMU while it is restricted, a few
changes are needed. We need a new way to get a startup notification from QEMU
as xenstore may not be accessible when QEMU is ready. We also need to a
different way to have QEMU save it's state and to insert cdrom as a restricted
QEMU doesn't have access to the file system.

For both, we can use QMP, we can use it to query QEMU's status, and we can use
it to send a file descriptor through which QEMU can save its state, or it can
be a cdrom.

We take this opportunity to rewrite the QMP client, and this time been
asynchronous, the result is libxl__ev_qmp_*.

The plat de résistance in this patch series start with patch
"libxl: Design of an async API to issue QMP commands to QEMU"
which implement libxl__ev_qmp_* functions to turn the QMP client into
asynchronous mode.

This comes with changes that uses the new interface.
* "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.
* "libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp"
  Which rewrite libxl__qmp_save(), and adds the ability to have QEMU save
  its state to a file descriptor which libxl will have openned.

Patches series available in a git tag:
git fetch https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git libxl-ev-qmp-v5
git checkout -b libxl-ev-qmp-v5 FETCH_HEAD

Cheers,

Anthony PERARD (15):
  libxl: Design of an async API to issue QMP commands to QEMU
  libxl_qmp: Connect to QMP socket
  libxl_qmp: Implement fd callback and read data
  libxl_qmp: Parse JSON input from QMP
  libxl_qmp: Separate QMP message generation from qmp_send_prepare
  libxl_qmp: Prepare the command to be sent
  libxl_qmp: Handle write to QMP socket
  libxl_qmp: Handle messages from QEMU
  libxl_qmp: Respond to QMP greeting
  libxl_exec: Add libxl__spawn_initiate_failure
  libxl_dm: Pre-open QMP socket for QEMU
  libxl: QEMU startup sync based on QMP
  libxl_qmp: Store advertised QEMU version in libxl__ev_qmp
  libxl: Change libxl__domain_suspend_device_model() to be async.
  libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

 tools/libxl/libxl_create.c           |  30 +-
 tools/libxl/libxl_dm.c               | 130 ++++-
 tools/libxl/libxl_dom_suspend.c      |  22 +-
 tools/libxl/libxl_exec.c             |   7 +
 tools/libxl/libxl_internal.h         | 141 +++++-
 tools/libxl/libxl_qmp.c              | 695 +++++++++++++++++++++++++--
 tools/libxl/libxl_types.idl          |   4 +
 tools/libxl/libxl_types_internal.idl |   8 +
 8 files changed, 950 insertions(+), 87 deletions(-)

-- 
Anthony PERARD


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

[PATCH v5 01/15] 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.

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

Notes:
    v5:
        some changes in the comment

 tools/libxl/libxl_internal.h | 72 +++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 802382c704..979a9947f0 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -202,6 +202,8 @@ 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__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,72 @@ struct libxl__ev_child {
     LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
 };
 
+/*
+ * QMP asynchronous calls
+ *
+ * This facility allows a command to be sent to QEMU, and the response to be
+ * handed to a callback function.
+ *
+ * Commands can be chained, with a same connection. (e.g. "add-fd" will need to
+ * be chained to the next command). A libxl__ev_qmp can be reused when the
+ * callback is been called in order to use the same connection.
+ *
+ * Only one connection at a time can be made to one QEMU, so avoid keeping a
+ * libxl__ev_qmp Connected for to long and call libxl__ev_qmp_dispose as soon
+ * as it is not needed anymore.
+ *
+ * Possible states of a libxl__ev_qmp:
+ *  Undefined
+ *    Might contain anything.
+ *  Idle
+ *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
+ *    function.
+ *    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.
+ *  Connected
+ *    Struct contain allocated ressources.
+ *    Calling _send() with this same ev will use the same QMP connection.
+ *    _dispose() must be called to reclaim resources.
+ *
+ * libxl__ev_qmp_init: Undefined/Idle -> Idle
+ *
+ * libxl__ev_qmp_send: Idle/Connected -> Active (on error: Idle)
+ *    Sends a command to QEMU.
+ *    callback will be called when a response is received or when an error
+ *    as occured.
+ *
+ * libxl__ev_qmp_dispose: Connected/Active/Idle -> Idle
+ *
+ * callback: When called: Active -> Connected
+ *    When called, ev is Connected and can be reused or disposed of.
+ *    If an error occured, it is called with response == NULL and the error
+ *    code in rc.
+ *    The callback is only called once.
+ */
+typedef struct libxl__ev_qmp libxl__ev_qmp;
+typedef void libxl__ev_qmp_callback(libxl__egc *egc, 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 *cfd; /* set to send a fd with the command, NULL otherwise */
+
+    /* remaining fields are private to libxl_ev_qmp_* */
+    int id;
+};
+
 
 /*
  * evgen structures, which are the state we use for generating
@@ -1902,7 +1970,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;
@@ -1915,7 +1983,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,
-- 
Anthony PERARD


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

Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU

[Ian Jackson]

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

This patch looks good to me.  Although it's a bit odd to mix some
typedef name shuffling in the same patch, I think it's OK here given
that the rest is API commentary.

> +/*
> + * QMP asynchronous calls
> + *
> + * This facility allows a command to be sent to QEMU, and the response to be
> + * handed to a callback function.
> + *
> + * Commands can be chained, with a same connection. (e.g. "add-fd" will need to
> + * be chained to the next command). A libxl__ev_qmp can be reused when the
> + * callback is been called in order to use the same connection.

I think the word `chained' here could do with definition.  I think you
just mean that they are on the same connection which remains
Active/Connected throughout ?

Also, can you please wrap the commentary lines to 70 or 75 columns ?
Your current 79-character lines get wrap damage when displayed in 80
columns with the added `+' and are worse when `> ' is added.

Thanks,
Ian.

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

Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

On Wed, Oct 10, 2018 at 04:18:52PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU"):
> > All the functions will be implemented in later patches.
> 
> This patch looks good to me.  Although it's a bit odd to mix some
> typedef name shuffling in the same patch, I think it's OK here given
> that the rest is API commentary.
> 
> > +/*
> > + * QMP asynchronous calls
> > + *
> > + * This facility allows a command to be sent to QEMU, and the response to be
> > + * handed to a callback function.
> > + *
> > + * Commands can be chained, with a same connection. (e.g. "add-fd" will need to
> > + * be chained to the next command). A libxl__ev_qmp can be reused when the
> > + * callback is been called in order to use the same connection.
> 
> I think the word `chained' here could do with definition.  I think you
> just mean that they are on the same connection which remains
> Active/Connected throughout ?

Yes.


Maybe I could avoid the word "chained" and replace that paragraph with
something like:
"Commands can be submited one after an other with the same connection.
(e.g. the result from the "add-fd" command need to be use in a follow-up
command before disconnecting from QMP). A libxl__ev_qmp can be reused
when the callback is been called in order to use the same connection."

> Also, can you please wrap the commentary lines to 70 or 75 columns ?
> Your current 79-character lines get wrap damage when displayed in 80
> columns with the added `+' and are worse when `> ' is added.

Will do.

-- 
Anthony PERARD

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

Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU"):
> Maybe I could avoid the word "chained" and replace that paragraph with
> something like:
> "Commands can be submited one after an other with the same connection.
> (e.g. the result from the "add-fd" command need to be use in a follow-up
> command before disconnecting from QMP). A libxl__ev_qmp can be reused
> when the callback is been called in order to use the same connection."

LGTM.

Ian.

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

Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 4075 bytes --]

On Fri, Sep 07, 2018 at 04:10:50PM +0100, Anthony PERARD wrote:
> All the functions will be implemented in later patches.
> 
> This patch includes the API that libxl can use to send QMP commands to
> QEMU.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v5:
>         some changes in the comment
> 
>  tools/libxl/libxl_internal.h | 72 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 802382c704..979a9947f0 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -202,6 +202,8 @@ 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__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,72 @@ struct libxl__ev_child {
>      LIBXL_LIST_ENTRY(struct libxl__ev_child) entry;
>  };
>  
> +/*
> + * QMP asynchronous calls
> + *
> + * This facility allows a command to be sent to QEMU, and the response to be
> + * handed to a callback function.
> + *
> + * Commands can be chained, with a same connection. (e.g. "add-fd" will need to
> + * be chained to the next command). A libxl__ev_qmp can be reused when the
> + * callback is been called in order to use the same connection.
> + *
> + * Only one connection at a time can be made to one QEMU, so avoid keeping a
> + * libxl__ev_qmp Connected for to long and call libxl__ev_qmp_dispose as soon
> + * as it is not needed anymore.

From where this limitation comes? Was that true before? I ask, because
that limitation would ease Linux-based stubdomain case, where QMP over
console have indeed only a single channel.

> + *
> + * Possible states of a libxl__ev_qmp:
> + *  Undefined
> + *    Might contain anything.
> + *  Idle
> + *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
> + *    function.
> + *    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.
> + *  Connected
> + *    Struct contain allocated ressources.
> + *    Calling _send() with this same ev will use the same QMP connection.
> + *    _dispose() must be called to reclaim resources.
> + *
> + * libxl__ev_qmp_init: Undefined/Idle -> Idle
> + *
> + * libxl__ev_qmp_send: Idle/Connected -> Active (on error: Idle)

Does it also mean libxl__ev_qmp_init or libxl__ev_qmp_send should deal
with connecting to qemu, which _does not know it is a new connection_,
so will not send a greeting etc? In case of QMP over console, there is
no OOB method to signal open/close (at least not easily). I implemented
rather hacky/incomplete way to reset QMP connection - or rather - send
greeting again, but that may result in confusing situations, like
QEMU sending response to previous command to unsuspecting new libxl
client.

> + *    Sends a command to QEMU.
> + *    callback will be called when a response is received or when an error
> + *    as occured.
> + *
> + * libxl__ev_qmp_dispose: Connected/Active/Idle -> Idle
> + *
> + * callback: When called: Active -> Connected
> + *    When called, ev is Connected and can be reused or disposed of.
> + *    If an error occured, it is called with response == NULL and the error
> + *    code in rc.
> + *    The callback is only called once.
> + */

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH v5 01/15] libxl: Design of an async API to issue QMP commands to QEMU

[Anthony PERARD]

On Thu, Oct 11, 2018 at 01:49:01AM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Sep 07, 2018 at 04:10:50PM +0100, Anthony PERARD wrote:
> > + * Only one connection at a time can be made to one QEMU, so avoid keeping a
> > + * libxl__ev_qmp Connected for to long and call libxl__ev_qmp_dispose as soon
> > + * as it is not needed anymore.
> 
> From where this limitation comes? Was that true before? I ask, because
> that limitation would ease Linux-based stubdomain case, where QMP over
> console have indeed only a single channel.

Actually, it is more like the QMP server will only talk to one client
(of a socket) at a time. You can initiate several connect() syscall to
the QMP unix-socket, but there is only the first one connection that
will have QMP activity, the other connection will wait. Once the first
connection is disconnected, the second connection will have QMP
activities.

So the limitation comes from QEMU, you will still need to be carefull
with QMP over console and only send one command at a time.

> > + *
> > + * Possible states of a libxl__ev_qmp:
> > + *  Undefined
> > + *    Might contain anything.
> > + *  Idle
> > + *    Struct contents are defined enough to pass to any libxl__ev_qmp_*
> > + *    function.
> > + *    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.
> > + *  Connected
> > + *    Struct contain allocated ressources.
> > + *    Calling _send() with this same ev will use the same QMP connection.
> > + *    _dispose() must be called to reclaim resources.
> > + *
> > + * libxl__ev_qmp_init: Undefined/Idle -> Idle
> > + *
> > + * libxl__ev_qmp_send: Idle/Connected -> Active (on error: Idle)
> 
> Does it also mean libxl__ev_qmp_init or libxl__ev_qmp_send should deal
> with connecting to qemu, which _does not know it is a new connection_,
> so will not send a greeting etc? In case of QMP over console, there is
> no OOB method to signal open/close (at least not easily). I implemented
> rather hacky/incomplete way to reset QMP connection - or rather - send
> greeting again, but that may result in confusing situations, like
> QEMU sending response to previous command to unsuspecting new libxl
> client.

libxl__ev_qmp_send does deal with connecting to QEMU, and it _does_ know
it is a new connection simply because we currently use a unix socket and
that works on connection-mode.

As for how greeting works: QEMU initiate this phase, a client (libxl)
only as to react. QEMU would send "Hi I'm QEMU 3.0" to new connections.

So in the case of QMP-over-console, maybe the client could try to send
the command, and if QEMU reply with "please do greeting" (actually it's
capability negociation) first the we just need to do it.

-- 
Anthony PERARD

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

[PATCH v5 02/15] libxl_qmp: Connect to QMP socket

[Anthony PERARD]

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

Callback functions will be implemented in following patches.

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

Notes:
    v5:
        nits
        use a define instead of a static int for QMP_CAPABILITY_NEGOCIATION_MSGID
        use a switch in qmp_ev_callback_writable to check qmp_state
        Add a description of the different value of libxl__qmp_state enum.

 tools/libxl/libxl_internal.h | 16 +++++++
 tools/libxl/libxl_qmp.c      | 91 ++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 979a9947f0..9bcab39f00 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -414,6 +414,19 @@ _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);
 
+typedef enum {
+    /* initial state */
+    qmp_state_disconnected = 1,
+    /* connected to QMP socket, waiting for greeting message */
+    qmp_state_connecting,
+    /* greeting message received */
+    qmp_state_greeting,
+    /* qmp_capabilities command sent, waiting for reply */
+    qmp_state_capability_negotiation,
+    /* ready to send commands */
+    qmp_state_connected,
+} libxl__qmp_state;
+
 struct libxl__ev_qmp {
     /* caller should include this in their own struct */
     /* caller must fill these in, and they must all remain valid */
@@ -423,6 +436,9 @@ struct libxl__ev_qmp {
 
     /* remaining fields are private to libxl_ev_qmp_* */
     int id;
+    libxl__carefd *qmp_cfd;
+    libxl__ev_fd qmp_efd;
+    libxl__qmp_state qmp_state;
 };
 
 
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index bdf1778cf1..167829a5ec 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1280,6 +1280,97 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
     return ret;
 }
 
+/* ------------ Implementation of libxl__ev_qmp ---------------- */
+
+static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
+                               int fd, short events, short revents)
+{
+}
+
+static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
+{
+    int rc, r;
+    struct sockaddr_un un;
+    const char *qmp_socket_path;
+
+    if (ev->qmp_state != qmp_state_disconnected)
+        return 0;
+
+    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+
+    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+
+    libxl__carefd_begin();
+    ev->qmp_cfd = libxl__carefd_opened(CTX, socket(AF_UNIX, SOCK_STREAM, 0));
+    if (!ev->qmp_cfd) {
+        LOGED(ERROR, ev->domid, "socket() failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    rc = libxl_fd_set_nonblock(CTX, libxl__carefd_fd(ev->qmp_cfd), 1);
+    if (rc)
+        goto out;
+
+    rc = libxl__prepare_sockaddr_un(gc, &un, qmp_socket_path, "QMP socket");
+    if (rc)
+        goto out;
+
+    r = connect(libxl__carefd_fd(ev->qmp_cfd),
+                (struct sockaddr *) &un, sizeof(un));
+    if (r) {
+        LOGED(ERROR, ev->domid, "Failed to connect to QMP socket %s",
+              qmp_socket_path);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = libxl__ev_fd_register(gc, &ev->qmp_efd, qmp_ev_fd_callback,
+                               libxl__carefd_fd(ev->qmp_cfd), POLLIN);
+    if (rc)
+        goto out;
+
+    ev->qmp_state = qmp_state_connecting;
+    return 0;
+
+out:
+    libxl__carefd_close(ev->qmp_cfd);
+    ev->qmp_cfd = NULL;
+    return rc;
+}
+
+
+/*
+ * libxl__ev_qmp_*
+ */
+
+void libxl__ev_qmp_init(libxl__ev_qmp *ev)
+{
+    ev->id = -1;
+
+    ev->qmp_cfd = NULL;
+    libxl__ev_fd_init(&ev->qmp_efd);
+    ev->qmp_state = qmp_state_disconnected;
+}
+
+int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
+                       const char *cmd, libxl__json_object *args)
+{
+    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
+
+    /* Connect to QEMU if not already connected */
+    return qmp_ev_connect(gc, ev);
+}
+
+void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
+{
+    LOGD(DEBUG, ev->domid, " ev %p", ev);
+
+    libxl__ev_fd_deregister(gc, &ev->qmp_efd);
+    libxl__carefd_close(ev->qmp_cfd);
+
+    libxl__ev_qmp_init(ev);
+}
+
 /*
  * 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 v5 02/15] libxl_qmp: Connect to QMP socket

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 02/15] libxl_qmp: Connect to QMP socket"):
> This is a first patch to implement libxl__ev_qmp, it only connects to
> the QMP socket of QEMU and registers a fd callback that does nothing.
...
> +typedef enum {
> +    /* initial state */
> +    qmp_state_disconnected = 1,
> +    /* connected to QMP socket, waiting for greeting message */
> +    qmp_state_connecting,
> +    /* greeting message received */
> +    qmp_state_greeting,
> +    /* qmp_capabilities command sent, waiting for reply */
> +    qmp_state_capability_negotiation,
> +    /* ready to send commands */
> +    qmp_state_connected,
> +} libxl__qmp_state;

IWBN to relate these private states to the outward-facing API states
like `Connected'.

I often write a table of legal field values - see for example,
libxl_exec.c near l.241 and libxl_stream_read.c near l.35.  But maybe
this is not complicated enough to need that.

> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> +{
...
> +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> +                (struct sockaddr *) &un, sizeof(un));

Up to here:

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

But:

> +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> +                       const char *cmd, libxl__json_object *args)
> +{
> +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> +
> +    /* Connect to QEMU if not already connected */
> +    return qmp_ev_connect(gc, ev);
> +}

I think it would be nicer to read a complete implementation of this
function.  Right now it's obviously wrong and impossible to review.

So please postpone this hunk.

Thanks,
Ian.

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

Re: [PATCH v5 02/15] libxl_qmp: Connect to QMP socket

[Anthony PERARD]

On Wed, Oct 10, 2018 at 04:29:03PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 02/15] libxl_qmp: Connect to QMP socket"):
> > This is a first patch to implement libxl__ev_qmp, it only connects to
> > the QMP socket of QEMU and registers a fd callback that does nothing.
> ...
> > +typedef enum {
> > +    /* initial state */
> > +    qmp_state_disconnected = 1,
> > +    /* connected to QMP socket, waiting for greeting message */
> > +    qmp_state_connecting,
> > +    /* greeting message received */
> > +    qmp_state_greeting,
> > +    /* qmp_capabilities command sent, waiting for reply */
> > +    qmp_state_capability_negotiation,
> > +    /* ready to send commands */
> > +    qmp_state_connected,
> > +} libxl__qmp_state;
> 
> IWBN to relate these private states to the outward-facing API states
> like `Connected'.

I think, that would be:
  qmp_state_disconnected,             Idle
  qmp_state_connecting,               Active
  qmp_state_greeting,                 Active
  qmp_state_capability_negotiation,   Active
  qmp_state_connected,                Active/Connected

I can try to squeeze this information somewhere.

> I often write a table of legal field values - see for example,
> libxl_exec.c near l.241 and libxl_stream_read.c near l.35.  But maybe
> this is not complicated enough to need that.

Indeed, I don't think the relation is complicated enough.

> > +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> > +{
> ...
> > +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> > +                (struct sockaddr *) &un, sizeof(un));
> 
> Up to here:
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> But:
> 
> > +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> > +                       const char *cmd, libxl__json_object *args)
> > +{
> > +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> > +
> > +    /* Connect to QEMU if not already connected */
> > +    return qmp_ev_connect(gc, ev);
> > +}
> 
> I think it would be nicer to read a complete implementation of this
> function.  Right now it's obviously wrong and impossible to review.
> 
> So please postpone this hunk.

It would have been nice if I could, but we can't have unused static
function. But let me reply to the review of the next patch.

-- 
Anthony PERARD

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

[PATCH v5 03/15] libxl_qmp: 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 a buffer.

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

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

Notes:
    v5:
        some cleanup
        remove read loop that only handled EINTR, simply return
    
    v4:
        remove use of a linked list of receive buffer, and use realloc instead.

 tools/libxl/libxl_internal.h |  8 +++
 tools/libxl/libxl_qmp.c      | 95 ++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9bcab39f00..0590801b30 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -439,6 +439,14 @@ struct libxl__ev_qmp {
     libxl__carefd *qmp_cfd;
     libxl__ev_fd qmp_efd;
     libxl__qmp_state qmp_state;
+    /* 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;
 };
 
 
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 167829a5ec..9ea303edf0 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, ev->domid, f, ##__VA_ARGS__)
+#else
+#  define LOG_QMP(f, ...)
+#endif
+
 /*
  * QMP types & constant
  */
@@ -1282,9 +1288,94 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
 
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
+/*
+ * QMP FD callbacks
+ */
+
+static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
+{
+    EGC_GC;
+    ssize_t r;
+
+    if (!ev->rx_buf) {
+        ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
+        ev->buf_size = QMP_RECEIVE_BUFFER_SIZE;
+        ev->buf_used = 0;
+        ev->buf_consumed = 0;
+    }
+
+    /* Check if last buffer still have space, or increase size */
+    /* The -1 is because there is always space for a NUL character */
+    if (ev->buf_used == ev->buf_size - 1) {
+        ev->buf_size += QMP_RECEIVE_BUFFER_SIZE;
+        ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->buf_size);
+    }
+
+    /* The -1 is because there is always space for a NUL character */
+    r = read(fd, ev->rx_buf + ev->buf_used, ev->buf_size - ev->buf_used - 1);
+    if (r < 0) {
+        if (errno == EINTR)
+            return 0;
+        assert(errno);
+        if (errno == EWOULDBLOCK)
+            return 0;
+        LOGED(ERROR, ev->domid, "error reading QMP socket");
+        return ERROR_FAIL;
+    }
+
+    if (r == 0) {
+        LOGD(ERROR, ev->domid, "No data read on QMP socket");
+        return 0;
+    }
+
+    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
+
+    ev->buf_used += r;
+    assert(ev->buf_used < ev->buf_size);
+
+    return 0;
+}
+
+static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
+{
+    EGC_GC;
+
+    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
+
+    /* On error, deallocate all private ressources */
+    libxl__ev_qmp_dispose(gc, ev);
+}
+
 static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
                                int fd, short events, short revents)
 {
+    EGC_GC;
+    int rc;
+
+    libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
+
+    if (revents & (POLLHUP)) {
+        LOGD(DEBUG, ev->domid, "received POLLHUP from QMP socket");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    if (revents & ~(POLLIN|POLLOUT)) {
+        LOGD(ERROR, ev->domid,
+             "unexpected poll event 0x%x on QMP socket (expected POLLIN "
+             "and/or POLLOUT)",
+            revents);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (revents & POLLIN) {
+        rc = qmp_ev_callback_readable(egc, ev, fd);
+        if (rc)
+            goto out;
+    }
+out:
+    if (rc)
+        qmp_ev_callback_error(egc, ev);
 }
 
 static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
@@ -1350,6 +1441,8 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qmp_cfd = NULL;
     libxl__ev_fd_init(&ev->qmp_efd);
     ev->qmp_state = qmp_state_disconnected;
+
+    ev->rx_buf = NULL;
 }
 
 int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
@@ -1365,6 +1458,8 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
 {
     LOGD(DEBUG, ev->domid, " ev %p", ev);
 
+    free(ev->rx_buf);
+
     libxl__ev_fd_deregister(gc, &ev->qmp_efd);
     libxl__carefd_close(ev->qmp_cfd);
 
-- 
Anthony PERARD


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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 03/15] libxl_qmp: 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 a buffer.
...
> +    if (!ev->rx_buf) {
> +        ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
> +        ev->buf_size = QMP_RECEIVE_BUFFER_SIZE;
> +        ev->buf_used = 0;
> +        ev->buf_consumed = 0;
> +    }
> +
> +    /* Check if last buffer still have space, or increase size */
> +    /* The -1 is because there is always space for a NUL character */
> +    if (ev->buf_used == ev->buf_size - 1) {
> +        ev->buf_size += QMP_RECEIVE_BUFFER_SIZE;
> +        ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->buf_size);
> +    }

I think this is unnecessarily complex.  I think you should set buf_*
to 0 in _init, and arrange to realloc(0, new_size) if the buffer is
not big enough for "some space" plus a nul.

Also, you should increase the buffer size exponentially.  And you
should put a limit on the buffer size, in case the sender goes mad.

> +    /* The -1 is because there is always space for a NUL character */
> +    r = read(fd, ev->rx_buf + ev->buf_used, ev->buf_size - ev->buf_used - 1);

Lines too long again.  (I will stop complaining about this now but can
you pleae fix it throughout?)

> +    if (r < 0) {
> +        if (errno == EINTR)
> +            return 0;

No, you need to go round again.  I'm afraid the whole of this function
needs to be in a loop.  This is because you are not guaranteed that
you will get more than one call to your readable callback per
transition from not-readable to readable.

> +        assert(errno);
> +        if (errno == EWOULDBLOCK)
> +            return 0;

And again.

> +        LOGED(ERROR, ev->domid, "error reading QMP socket");

I think you need to look up the error handling for (possibly
long-running) connect operation fails on a stream socket which were in
nonblocking mode when you called connect().  You may need to handle
EINPROGRESS from the connect() syscall itself (that was in the
previous patch).

> +    if (r == 0) {
> +        LOGD(ERROR, ev->domid, "No data read on QMP socket");

You mean "unexpected EOF" not "no data read".  Normally this would
mean that qemu died or crashed.

> +        return 0;

Err, and then this needs to be handled as an error, not simply
ignored.  If it happens, it will keep happening.

> +    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
> +
> +    ev->buf_used += r;
> +    assert(ev->buf_used < ev->buf_size);

It would be really helpful if these partial functions were to contain
an `xxx' or something where there is missing code.

As it is I know that more code will occur here, but nothing
straightforward in the review will process will spot it the mistake if
you forget to add it...

The loop I mentioned could be in qmp_ev_fd_callback.

TBH I find the split between qmp_ev_fd_callback and
qmp_ev_callback_readable a bit confusing but I know that I'm unusual
in preferring longer functions.

> +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> +{
> +    EGC_GC;
> +
> +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> +
> +    /* On error, deallocate all private ressources */
> +    libxl__ev_qmp_dispose(gc, ev);

Missing /* xxx */ to report the error upwards.


Hrm.  I realise this is going to be annoying, but I think I should
probably stop now because of the lack of xxx's means it's hard for me
to review.

What do you think would be best:
 (i) I should, in my own tree, squash several of your commits down
     so I can review them together
 (ii) You repost with a lot of XXXs added
 (iii) We intend to squash the commits in xen.git ?

I think (ii) is a fair amount of work for polishing an intermediate
state, and probably a waste.  I'm inclined to (i) (iii).  Can you tell
me which patches I should sqaush ?

I think I need up to `libxl_qmp: Respond to QMP greeting' ?

Regards,
Ian.

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data

[Anthony PERARD]

On Wed, Oct 10, 2018 at 04:47:08PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 03/15] libxl_qmp: 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 a buffer.
> ...
> > +    if (!ev->rx_buf) {
> > +        ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
> > +        ev->buf_size = QMP_RECEIVE_BUFFER_SIZE;
> > +        ev->buf_used = 0;
> > +        ev->buf_consumed = 0;
> > +    }
> > +
> > +    /* Check if last buffer still have space, or increase size */
> > +    /* The -1 is because there is always space for a NUL character */
> > +    if (ev->buf_used == ev->buf_size - 1) {
> > +        ev->buf_size += QMP_RECEIVE_BUFFER_SIZE;
> > +        ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->buf_size);
> > +    }
> 
> I think this is unnecessarily complex.  I think you should set buf_*
> to 0 in _init, and arrange to realloc(0, new_size) if the buffer is
> not big enough for "some space" plus a nul.
> 
> Also, you should increase the buffer size exponentially.  And you
> should put a limit on the buffer size, in case the sender goes mad.

Ok, I'll see what I can do.

> > +    /* The -1 is because there is always space for a NUL character */
> > +    r = read(fd, ev->rx_buf + ev->buf_used, ev->buf_size - ev->buf_used - 1);
> 
> Lines too long again.  (I will stop complaining about this now but can
> you pleae fix it throughout?)

But the comment is less than 70chr long, and the next line follow the
coding style, which is "Lines are limited to 75-80 characters.", and
I'm pretty sure that 77 is less that 75-80!

If the coding style described in CODING_STYLE change before my next
submition, I can wrap too long lines ;-).

> > +    if (r < 0) {
> > +        if (errno == EINTR)
> > +            return 0;
> 
> No, you need to go round again.  I'm afraid the whole of this function
> needs to be in a loop.  This is because you are not guaranteed that
> you will get more than one call to your readable callback per
> transition from not-readable to readable.

I've did it without a loop because POLLIN means "data may be read
without blocking", and I can't find anything speaking about a relation
with transition from one state to the other.

But I can add a loop around the code, as that seems to confuse everyone
that there is none.

> > +        assert(errno);
> > +        if (errno == EWOULDBLOCK)
> > +            return 0;
> 
> And again.

Surely here I need to return on EWOULDBLOCK. The fd is obviously in
non-readable state, so there is going to be a transition.

> > +        LOGED(ERROR, ev->domid, "error reading QMP socket");
> 
> I think you need to look up the error handling for (possibly
> long-running) connect operation fails on a stream socket which were in
> nonblocking mode when you called connect().  You may need to handle
> EINPROGRESS from the connect() syscall itself (that was in the
> previous patch).

Looks like it, I'll have a look at the error handling of the connect()
call.

> > +    if (r == 0) {
> > +        LOGD(ERROR, ev->domid, "No data read on QMP socket");
> 
> You mean "unexpected EOF" not "no data read".  Normally this would
> mean that qemu died or crashed.
> 
> > +        return 0;
> 
> Err, and then this needs to be handled as an error, not simply
> ignored.  If it happens, it will keep happening.

I think I'm been lazy and have just let this error been handle in the
next event where POLLHUP should be set. I can try to handle this error
better here.

> > +    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
> > +
> > +    ev->buf_used += r;
> > +    assert(ev->buf_used < ev->buf_size);
> 
> It would be really helpful if these partial functions were to contain
> an `xxx' or something where there is missing code.
> 
> As it is I know that more code will occur here, but nothing
> straightforward in the review will process will spot it the mistake if
> you forget to add it...
> 
> The loop I mentioned could be in qmp_ev_fd_callback.
> 
> TBH I find the split between qmp_ev_fd_callback and
> qmp_ev_callback_readable a bit confusing but I know that I'm unusual
> in preferring longer functions.
> 
> > +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> > +{
> > +    EGC_GC;
> > +
> > +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> > +
> > +    /* On error, deallocate all private ressources */
> > +    libxl__ev_qmp_dispose(gc, ev);
> 
> Missing /* xxx */ to report the error upwards.
> 
> 
> Hrm.  I realise this is going to be annoying, but I think I should
> probably stop now because of the lack of xxx's means it's hard for me
> to review.
> 
> What do you think would be best:
>  (i) I should, in my own tree, squash several of your commits down
>      so I can review them together
>  (ii) You repost with a lot of XXXs added
>  (iii) We intend to squash the commits in xen.git ?
> 
> I think (ii) is a fair amount of work for polishing an intermediate
> state, and probably a waste.  I'm inclined to (i) (iii).  Can you tell
> me which patches I should sqaush ?
> 
> I think I need up to `libxl_qmp: Respond to QMP greeting' ?

Squashing all the relevant commits might not be that bad, since all
commits implement different functions.

There is one patch that can be reviewed separatly, and then yes, you
need up to 'libxl_qmp: Respond to QMP greeting'. That give us:

pick      libxl_qmp: Separate QMP message generation from qmp_send_prepare
pick      libxl_qmp: Connect to QMP socket
squash    libxl_qmp: Implement fd callback and read data
squash    libxl_qmp: Parse JSON input from QMP
squash    libxl_qmp: Prepare the command to be sent
squash    libxl_qmp: Handle write to QMP socket
squash    libxl_qmp: Handle messages from QEMU
squash    libxl_qmp: Respond to QMP greeting

That give us a patch with only 500 insertions.

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data"):
> On Wed, Oct 10, 2018 at 04:47:08PM +0100, Ian Jackson wrote:
> > Lines too long again.  (I will stop complaining about this now but can
> > you pleae fix it throughout?)
> 
> But the comment is less than 70chr long, and the next line follow the
> coding style, which is "Lines are limited to 75-80 characters.", and
> I'm pretty sure that 77 is less that 75-80!

It's less than 80 but not less than 75.  I have sent a patch to change
this in CODING_STYLE.  We can argue about this in that other thread.
(In practical terms these lines do result in wrap damage on my
screen, which makes it significantly harder to read, so I'm not just
quibbling for no reason.)

> > No, you need to go round again.  I'm afraid the whole of this function
> > needs to be in a loop.  This is because you are not guaranteed that
> > you will get more than one call to your readable callback per
> > transition from not-readable to readable.
> 
> I've did it without a loop because POLLIN means "data may be read
> without blocking", and I can't find anything speaking about a relation
> with transition from one state to the other.

You're right that this is not properly documented.  You'll see that I
have sent a patch to fix that.

In practice, if the application has its own event loop which uses
epoll and EPOLLET then indeed you'll only get the callback once per
transition from unready to ready.  Some event-like descriptor
readiness reporting mechanisms support only those semantics.

> > > +        assert(errno);
> > > +        if (errno == EWOULDBLOCK)
> > > +            return 0;
> > 
> > And again.
> 
> Surely here I need to return on EWOULDBLOCK. The fd is obviously in
> non-readable state, so there is going to be a transition.

Err, sorry, yes.

> > > +    if (r == 0) {
> > > +        LOGD(ERROR, ev->domid, "No data read on QMP socket");
> > 
> > You mean "unexpected EOF" not "no data read".  Normally this would
> > mean that qemu died or crashed.
> > 
> > > +        return 0;
> > 
> > Err, and then this needs to be handled as an error, not simply
> > ignored.  If it happens, it will keep happening.
> 
> I think I'm been lazy and have just let this error been handle in the
> next event where POLLHUP should be set. I can try to handle this error
> better here.

I think you might not get POLLHUP.  For example if qemu did
shutdown(,SHUT_WR);  Thanks.

> > Hrm.  I realise this is going to be annoying, but I think I should
> > probably stop now because of the lack of xxx's means it's hard for me
> > to review.
...
> There is one patch that can be reviewed separatly, and then yes, you
> need up to 'libxl_qmp: Respond to QMP greeting'. That give us:
> 
> pick      libxl_qmp: Separate QMP message generation from qmp_send_prepare
> pick      libxl_qmp: Connect to QMP socket
> squash    libxl_qmp: Implement fd callback and read data
> squash    libxl_qmp: Parse JSON input from QMP
> squash    libxl_qmp: Prepare the command to be sent
> squash    libxl_qmp: Handle write to QMP socket
> squash    libxl_qmp: Handle messages from QEMU
> squash    libxl_qmp: Respond to QMP greeting
> 
> That give us a patch with only 500 insertions.

I will do that and patchbomb myself with the resulting email and then
reply as if you had sent it to me.

Thanks,
Ian.

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Ian Jackson]

As discussed, I squwashed several of these patches together and I'm
now doing my code review on the result.  I've gone through it in some
detail.


Again, I asked for more internal documentation about the legal states
etc.  I will have to read it in detail again I'm afraid after that is
done.

The reason I ask for this is that this is a complicated and
substantial pile of code.  It's not sensible for anyone to try to hold
it in their head at once - we will make mistakes.  And it should be
possbile to modify it without reading all of it.

So, it should be possible for anyone to:

 * Look only at the summary internal architecture state machine
   comments and so on, and confirm to themselves that this is a
   sensible design and that all the possible states are represented
   and that the interlocking states of the detailed variables are
   both sufficient, and of manageable complexity.

 * Read any small fragment of code and see that transforms legal
   states into other legal states, by reference to the above.

Right now this is not really possible.  I look at things like this:

> +    ev->id = ev->last_id_used;
> +    ev->tx_buf = buf;
> +    ev->tx_buf_len = len;
> +
> +    libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);

in qmp_ev_prepare_cmd, and I need to guess whether the surrounding
code has all the needed error checks and whetehr it does all that is
needed.

I can't be sure, because the only way to infer the rules about what
combinations of values are legal in the variables is to read the
entire program.  If I somehow peer at it and with great perspicacity
discover a bug, it won't be clear whether the bug is that the variable
is set wrong, or used wrong, or that there is a design error.


Now on to the details.

Firstly, a small apology.  I misread your squashing instructions
and merged in
  libxl_qmp: Separate QMP message generation from qmp_send_prepare
when I shouldn't have done.  FAOD I like it separated out, so please
don't squash it in even though my review comments below do include it.

In your next respin could usefully buble it up I think.


Ian Jackson writes ("[PATCH 2/8] libxl_qmp: PORTMANTEAU starting with: Connect to QMP socket"):
> +typedef enum {
> +    /* initial state */
> +    qmp_state_disconnected = 1,
> +    /* connected to QMP socket, waiting for greeting message */
> +    qmp_state_connecting,
> +    /* greeting message received */
> +    qmp_state_greeting,
> +    /* qmp_capabilities command sent, waiting for reply */
> +    qmp_state_capability_negotiation,
> +    /* ready to send commands */
> +    qmp_state_connected,
> +} libxl__qmp_state;

In an earlier email in response to this same v5 I wrote:

  IWBN to relate these private states to the outward-facing API states
  like `Connected'.

  I often write a table of legal field values - see for example,
  libxl_exec.c near l.241 and libxl_stream_read.c near l.35.  But maybe
  this is not complicated enough to need that.

Seeing this in context with all the new fields in libxl__ev_qmp I
do think this would help.  (I appreciate you've not had a chance to do
that yet.)

Ie ideally, if you can make it fit, a table whose columns are the
qmp_state_* values and whose rows are: the private variables in
libxl__ev_qmp; the outward-facing state; the qmp-facing protocol
state.  (I suggest that way round because there are only 5 states and
more proposed rows.)

>      int id;
> +    libxl__carefd *qmp_cfd;
> +    libxl__ev_fd qmp_efd;
> +    libxl__qmp_state qmp_state;
> +    unsigned int last_id_used;

Why is id signed but last_id_used unsigned ?  This seems odd.

> -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_cmd(libxl__gc *gc, const char *cmd,
> +                             const libxl__json_object *args,
> +                             int id, size_t *len_r)
...
> +    const unsigned char *buf;

Would be useful to write here that the memory for buf is owned by
hand.  And therefore to have hand first.

> +    libxl_yajl_length len;
>      yajl_gen_status s;
>      yajl_gen hand;

hand should perhaps be initialised to NULL in case new code is added
before it is allocated ?

> +    ret = libxl__malloc(NOGC, len + 3);
> +    strncpy(ret, (const char *)buf, len + 3);
> +    strncpy(ret + len, "\r\n", 3);
> +    len += 2;

I'm not sure why this can't be done with libxl_sprintf and a suitable
%<something>n.  This open-coding of +3 etc. is quite fiddly.

> +/* ------------ Implementation of libxl__ev_qmp ---------------- */
> +
> +/* hard coded message ID used for capability negotiation */
> +#define QMP_CAPABILITY_NEGOTIATION_MSGID 1
> +
> +static int qmp_ev_prepare_cmd(libxl__gc *gc,
> +                              libxl__ev_qmp *ev,
> +                              const char *cmd,
> +                              const libxl__json_object *args)
> +{
> +    char *buf = NULL;
> +    size_t len;
> +
> +    buf = qmp_prepare_cmd(gc, cmd, args, ++ev->last_id_used, &len);
> +    if (!buf)
> +        return ERROR_FAIL;
> +
> +    ev->id = ev->last_id_used;

This duplicate reference to last_id_used is a bit odd.  Perhaps this
should go above and then qmp_prepare_cmd could reference it ?

> +    ev->tx_buf = buf;
> +    ev->tx_buf_len = len;

I think it would be wise to assert, earlier in this function, that
the buffer is empty.

> +static int qmp_error_class_to_libxl_error_code(const libxl__qmp_error_class c)
> +{
> +    switch (c) {
> +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> +        return ERROR_QMP_GENERIC_ERROR;
> +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> +        return ERROR_QMP_COMMAND_NOT_FOUND;
> +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> +        return ERROR_QMP_DEVICE_NOT_FOUND;

Urgh.  The slightly different names means that this can't be
macro-ified.  Without that, it would be really easy for someone in the
future to accidentally write something like this:

> +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> +        return ERROR_QMP_GENERIC_ERROR;
> +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> +        return ERROR_QMP_GENERIC_ERROR;
> +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> +        return ERROR_QMP_DEVICE_NOT_FOUND;

and it's hard to spot.

What are LIBXL__QMP_ERROR_CLASSes and why are they even different from
ERROR_* values ?  Maybe one of them is the QMP integer values and one
of them is the corresponding libxl integer values ?

Anyway, can we not make this less open-coded somehow.

> +    default:
> +        abort();

But what happens if qemu invents a new error code ?  I don't think
aborting is likely to be right.

> +/* return 1 when a user callback as been called */
> +static int qmp_ev_handle_response(libxl__egc *egc,
> +                                  libxl__ev_qmp *ev,
> +                                  const libxl__json_object *resp,
> +                                  libxl__qmp_message_type type)
> +{
> +    EGC_GC;
> +    const libxl__json_object *response;
> +    const libxl__json_object *o;
> +    int rc;
> +    int id;
> +
> +    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);

What is the dead store from "error" doing ?

> +    id = libxl__json_object_get_integer(o);
> +    if (id != ev->id && id != QMP_CAPABILITY_NEGOTIATION_MSGID)
> +        return 0;

Err, is this an expected situation ?  ISTM that if qemu sends us
messages with uknown ids we should declare it gone wrong.

> +    switch (type) {
> +    case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
> +        response = libxl__json_map_get("return", resp, JSON_ANY);
> +        rc = 0;
> +        break;
> +    }
> +    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
> +        const char *s;
> +        const libxl__json_object *err;
> +        libxl__qmp_error_class error_class;
> +
> +        rc = ERROR_FAIL;
> +        response = NULL;
> +
> +        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))
> +            rc = qmp_error_class_to_libxl_error_code(error_class);
> +
> +        o = libxl__json_map_get("desc", err, JSON_STRING);
> +        s = libxl__json_object_get_string(o);
> +        if (s)
> +            LOGD(ERROR, ev->domid, "%s", s);
> +
> +        break;

Surely this should print more debugging (or maybe even error log
messages) if the error json document was not in the expected form ?

> +    /*
> +     * Even if the current state is capability_negotiation and the correct ID
> +     * as been received, call the callback on error.
> +     */
> +    if (ev->qmp_state == qmp_state_capability_negotiation &&
> +        id == QMP_CAPABILITY_NEGOTIATION_MSGID &&
> +        rc == 0) {
> +        ev->qmp_state = qmp_state_connected;
> +        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);
> +        return 0;

I don't much like the open-coding of libx__ev_fd_modify here, although
the alternative might be a very small function.  Consider that,
though ?  Or a comment ?

> +    } else {
> +        ev->id = -1;
> +        ev->callback(egc, ev, response, rc); /* must be last */
> +        return 1;

Surely this should set the internal state to a new state ?

> +    }
> +}
> +
> +/* return 1 when a user callback as been called */
> +static int qmp_ev_handle_message(libxl__egc *egc,
> +                                 libxl__ev_qmp *ev,
> +                                 const libxl__json_object *resp)

If you have it like this, this explanation of the return value needs
to be clearer.  There is only one call site, so it might be better to
fold it into the fd callback.

> +{
> +    EGC_GC;
> +    libxl__qmp_message_type type = qmp_response_type(resp);
> +
> +    switch (type) {
> +    case LIBXL__QMP_MESSAGE_TYPE_QMP:
> +        /* greeting message */
> +        assert(ev->qmp_state == qmp_state_connecting);

This assert is wrong.  A hostile qemu could crash libxl.

I think you need a `declare qemu has gone wrong' function which
cleanly tears the thing down and makes a suitable call to the
callback.

> +        ev->qmp_state = qmp_state_greeting;
> +        /* Allow qmp_ev_callback_writable to be called in order to send
> +         * qmp_capabilities */
> +        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);

See my comments about writeable, above.

Another possible approach to this would be to have a function called
something like
   qmp_ev_ensure_reading_writing
which looks at ev->qmp_state and makes an appropriate call to
libxl__ev_fd_modify.

That would make it easy to verify the correctness against the table of
possible states, which I asked for right at the start of this review.

> +static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
> +{
> +    EGC_GC;
> +    ssize_t r;
> +    char *end = NULL;
...

I think I already commented on some of this so I won't repeat myself.

> +    /* workaround strstr limitation */
> +    ev->rx_buf[ev->buf_used] = '\0';

It's possible that you wanted `memmem' which is in glibc and FreeBSD's
libc, at least ?

> +    /*
> +     * Search for the end of a QMP message: "\r\n" in the newly received
> +     * bytes + the last byte on the previous read() if any

This is to avoid rescanning the buffer pointlessly ?

> +     * end: This will point to the byte right after \r\n
> +     */
> +    end = strstr(ev->rx_buf + ev->buf_used - r
> +                 + (ev->buf_used - r == 0 ? 0 : -1),
> +                 "\r\n");

This is really quite fiddly.  I presume that there should be no bare
\n in this stream.  How about searching for just \n instead, and then
checking for and stripping the \r ?  (Instead of my memmem
suggestion.)

> +    while (end) {
> +        libxl__json_object *o;
> +        /* Start parsing from s */
> +        char *s = ev->rx_buf + ev->buf_consumed;

This complicated buffer management would really benefit from some
comments the stating invariants between rx_buf, buf_consumed, and so
on.  Please provide that, and then I will have to re-review it.

> +        /* Findout how much can be parsed */
> +        size_t len = end - s;
> +
> +        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
> +
> +        /* Replace \n by \0 so that libxl__json_parse can use strlen */
> +        s[len - 1] = '\0';

Doesn't this feed the \r to libxl__json_parse ?

Also, why does this have to be a loop ?  Does qemu really send
multiple json documents end to end, and only sometimes with
intervening \r\n ?  Does it ever send a json document without \r\n at
the end and then stop speaking for a while ?

> +        ev->buf_consumed += len;
> +
> +        if (ev->buf_consumed >= ev->buf_used) {
> +            free(ev->rx_buf);

Surely buf_consumed <= buf_used ?  Maybe you should assert that.

> +        /* check if there is another message received at the same time */
> +        if (ev->rx_buf) {
> +            end = strstr(ev->rx_buf + ev->buf_consumed, "\r\n");

Wait, you have two calls to strstr ?  Now I am confused about the code
structure.  You definitely need to write this so that the searching
for delimiters is only done once.

> +        /* Must be last and return when the user callback is called */
> +        if (qmp_ev_handle_message(egc, ev, o) == 1)
> +            return 0;

There seems to be nothing in any of this that handles the case where
a malicious or buggy qemu sends an unsolicited response.  I think that
will probably make a wild callback ?

> +static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
> +{
> +    int rc;
> +    char *buf;
> +    size_t len;
> +    int send_fd = -1;
> +
> +    /* No need to call qmp_ev_callback_writable again, everything that needs to
> +     * be send for now will be in this call. */

So you guarantee never to send any message which is too large to fit
into a socket buffer ?  Do you know what that length is ?

> +    case qmp_state_connected:
> +        if (!ev->tx_buf)
> +            return 0;
> +
> +        buf = ev->tx_buf;
> +        len = ev->tx_buf_len;
> +        send_fd = libxl__carefd_fd(ev->cfd);
> +
> +        ev->tx_buf = NULL;

I don't like the way you leave tx_buf_len set to a nonzero value.
A sensible invariant would be that tx_buf_len is always valid and that
therefore if tx_buf is 0, so is tx_buf_len.

Where is tx_buf freed ?  Indeed, what is tx_buf's memory management ?
That needs to be written down.

> +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> +{
> +    EGC_GC;
> +
> +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> +
> +    /* On error, deallocate all private ressources */
> +    libxl__ev_qmp_dispose(gc, ev);

This surely needs to set the state.  Presumably that should be done in
libxl__ev_qmp_dispose but AFAICT it isn't.

> +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> +                               int fd, short events, short revents)
> +{
> +    EGC_GC;
> +    int rc;

You could implement the retry loop here.  You could assign a positive
value return code from qmp_ev_callback_writable /
qmp_ev_callback_readable to indicate that it is done.

> +    libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
> +
> +    if (revents & (POLLHUP)) {
> +        LOGD(DEBUG, ev->domid, "received POLLHUP from QMP socket");

Surely these should be LOGD(ERROR,...).  This is not expected, is it,
and it is probably going to cause something else to fail.

> +        rc = ERROR_FAIL;

I think it might be a good idea to invent a new
ERROR_QEMU_QMP_PROTOCOL_ERROR or something (look at the other codes to
pick a good name...).

> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> +{
> +    int rc, r;
> +    struct sockaddr_un un;
> +    const char *qmp_socket_path;
> +
> +    if (ev->qmp_state != qmp_state_disconnected)
> +        return 0;
> +
> +    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
> +
> +    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
> +
> +    libxl__carefd_begin();
> +    ev->qmp_cfd = libxl__carefd_opened(CTX, socket(AF_UNIX, SOCK_STREAM, 0));

I find this socket call nested in the arguments to
libxl__carefd_opened rather un-plain.

> +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> +                (struct sockaddr *) &un, sizeof(un));
> +    if (r) {

I commented on the error handling here already I think.

> +void libxl__ev_qmp_init(libxl__ev_qmp *ev)
> +{
> +    ev->id = -1;
> +
> +    ev->qmp_cfd = NULL;
> +    libxl__ev_fd_init(&ev->qmp_efd);
> +    ev->qmp_state = qmp_state_disconnected;
> +    ev->last_id_used = QMP_CAPABILITY_NEGOTIATION_MSGID + 1;

Going back a bit, earlier we had:

> +#define QMP_CAPABILITY_NEGOTIATION_MSGID 1

I recommend using a different value.  Can we safely let this wrap ?
If so maybe we could use 0x786c7100 which spells "xlq\0" or something.
This can make it easier to see where rogue values are coming from, to
distinguish arguments in debug output, etc.

> +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> +                       const char *cmd, libxl__json_object *args)
> +{
> +    int rc;
> +
> +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);

Needs an assert about the state.

> +    /* Connect to QEMU if not already connected */
> +    rc = qmp_ev_connect(gc, ev);
> +    if (rc)
> +        goto out;
> +
> +    rc = qmp_ev_prepare_cmd(gc, ev, cmd, args);
> +    if (rc)
> +        goto out;
> +
> +out:
> +    if (rc)
> +        libxl__ev_qmp_dispose(gc, ev);

I think you need to write in your API documents that if callback was
called indicating an error, the connection may be made Idle.


Thanks,
Ian.

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Anthony PERARD]

On Mon, Oct 15, 2018 at 05:35:36PM +0100, Ian Jackson wrote:
> > +static int qmp_error_class_to_libxl_error_code(const libxl__qmp_error_class c)
> > +{
> > +    switch (c) {
> > +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> > +        return ERROR_QMP_GENERIC_ERROR;
> > +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> > +        return ERROR_QMP_COMMAND_NOT_FOUND;
> > +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> > +        return ERROR_QMP_DEVICE_NOT_FOUND;
> 
> Urgh.  The slightly different names means that this can't be
> macro-ified.  Without that, it would be really easy for someone in the
> future to accidentally write something like this:
>
> > +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> > +        return ERROR_QMP_GENERIC_ERROR;
> > +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> > +        return ERROR_QMP_GENERIC_ERROR;
> > +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> > +        return ERROR_QMP_DEVICE_NOT_FOUND;
> 
> and it's hard to spot.
> 
> What are LIBXL__QMP_ERROR_CLASSes and why are they even different from
> ERROR_* values ?  Maybe one of them is the QMP integer values and one
> of them is the corresponding libxl integer values ?

The issue here is that, in QMP, the error is a string and in camel case,
e.g. "GenericError".  I've use the idl to generate an enum from the
different error strings.

> Anyway, can we not make this less open-coded somehow.

I could remove some underscores '_' from the libxl ERROR_* integer
values, e.g. ERROR_QMP_GENERICERROR. But if you have a better suggestion
on how to transform "CommandNotFound" into a ERROR_QMP_*, I'll take it.

> > +    default:
> > +        abort();
> 
> But what happens if qemu invents a new error code ?  I don't think
> aborting is likely to be right.

If qemu invent a new error code, libxl should not call
qmp_error_class_to_libxl_error_code(), because libxl would not have been
able to generate a libxl__qmp_error_class from the new error code.

But I could add an assert(false) and return ERROR_FAIL instead of the
abort.

> > +/* return 1 when a user callback as been called */
> > +static int qmp_ev_handle_response(libxl__egc *egc,
> > +                                  libxl__ev_qmp *ev,
> > +                                  const libxl__json_object *resp,
> > +                                  libxl__qmp_message_type type)
> > +{
> > +    EGC_GC;
> > +    const libxl__json_object *response;
> > +    const libxl__json_object *o;
> > +    int rc;
> > +    int id;
> > +
> > +    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);
> 
> What is the dead store from "error" doing ?

It's not dead, I reuse "o", as I get "desc" from "o".
I could just create new variables here instead of reusing the same one
(o) over and over again, e.g. obj_id, obj_error, obj_desc.

Should I add here that we are parsing: {"error":{"desc": X }} ?

> > +    switch (type) {
> > +    case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
> > +        response = libxl__json_map_get("return", resp, JSON_ANY);
> > +        rc = 0;
> > +        break;
> > +    }
> > +    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
> > +        const char *s;
> > +        const libxl__json_object *err;
> > +        libxl__qmp_error_class error_class;
> > +
> > +        rc = ERROR_FAIL;
> > +        response = NULL;
> > +
> > +        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))
> > +            rc = qmp_error_class_to_libxl_error_code(error_class);
> > +
> > +        o = libxl__json_map_get("desc", err, JSON_STRING);
> > +        s = libxl__json_object_get_string(o);
> > +        if (s)
> > +            LOGD(ERROR, ev->domid, "%s", s);
> > +
> > +        break;
> 
> Surely this should print more debugging (or maybe even error log
> messages) if the error json document was not in the expected form ?

Do you mean that if the QMP server doesn't respect the QMP protocol, I
should print more debug here?

If we are at this point in the programme, we know that the server sent:
{"error": X }

I'm tempted to say that if "class" or "desc" isn't present, we could say
so and declare the server broken (and disconnect).

> > +        /* Findout how much can be parsed */
> > +        size_t len = end - s;
> > +
> > +        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
> > +
> > +        /* Replace \n by \0 so that libxl__json_parse can use strlen */
> > +        s[len - 1] = '\0';
> 
> Doesn't this feed the \r to libxl__json_parse ?
> 
> Also, why does this have to be a loop ?  Does qemu really send
> multiple json documents end to end, and only sometimes with
> intervening \r\n ?  Does it ever send a json document without \r\n at
> the end and then stop speaking for a while ?

QEMU will send two messages at once, e.g. when asking to eject or change
the cdrom, QEMU will often send the response to our command and an event
at the same time. So we need a loop to parse all messages received.

But there is always a \r\n at the end of every commands.

> > +        ev->buf_consumed += len;
> > +
> > +        if (ev->buf_consumed >= ev->buf_used) {
> > +            free(ev->rx_buf);
> 
> Surely buf_consumed <= buf_used ?  Maybe you should assert that.

I'm attempting to detect here if there's something left in the buffer
that hasn't been parsed yet. If there is nothing left, we can free the
buffer.

Or do you mean that the condition should be buf_consumed == buf_used ?

> > +static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
> > +{
> > +    int rc;
> > +    char *buf;
> > +    size_t len;
> > +    int send_fd = -1;
> > +
> > +    /* No need to call qmp_ev_callback_writable again, everything that needs to
> > +     * be send for now will be in this call. */
> 
> So you guarantee never to send any message which is too large to fit
> into a socket buffer ?  Do you know what that length is ?

I'm using libxl_write_exactly().

On the other end, I've just realize that I'm also using
libxl__sendmsg_fds(), which I guess may not send everything. But the
function also doesn't seems to the caller know how much have been sent.

Should I keep using libxl_write_exactly() ?
I'm going to need to be able to track how much data have been sent for
at least sendmsg() anyway, and try again later.

> > +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> > +{
> > +    EGC_GC;
> > +
> > +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> > +
> > +    /* On error, deallocate all private ressources */
> > +    libxl__ev_qmp_dispose(gc, ev);
> 
> This surely needs to set the state.  Presumably that should be done in
> libxl__ev_qmp_dispose but AFAICT it isn't.

It is done by libxl__ev_qmp_init, which _dispose calls, itn't that
enough?

> > +void libxl__ev_qmp_init(libxl__ev_qmp *ev)
> > +{
> > +    ev->id = -1;
> > +
> > +    ev->qmp_cfd = NULL;
> > +    libxl__ev_fd_init(&ev->qmp_efd);
> > +    ev->qmp_state = qmp_state_disconnected;
> > +    ev->last_id_used = QMP_CAPABILITY_NEGOTIATION_MSGID + 1;
> 
> Going back a bit, earlier we had:
> 
> > +#define QMP_CAPABILITY_NEGOTIATION_MSGID 1
> 
> I recommend using a different value.  Can we safely let this wrap ?
> If so maybe we could use 0x786c7100 which spells "xlq\0" or something.
> This can make it easier to see where rogue values are coming from, to
> distinguish arguments in debug output, etc.

ID can be string, or any json value. Maybe we could have
{"libxl-id": number} as "id"?
But 0x786c7100 as starting point would work fine as well.
I don't think it matter if the value wrap. Beside adding 1 to generate a
new id, and compare, I don't think the id should be use for anything
else.

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]"):
> On Mon, Oct 15, 2018 at 05:35:36PM +0100, Ian Jackson wrote:
> > What are LIBXL__QMP_ERROR_CLASSes and why are they even different from
> > ERROR_* values ?  Maybe one of them is the QMP integer values and one
> > of them is the corresponding libxl integer values ?
> 
> The issue here is that, in QMP, the error is a string and in camel case,
> e.g. "GenericError".  I've use the idl to generate an enum from the
> different error strings.

You're using the idl generator to generate a table mapping
"GenericError" to LIBXL__QMP_ERROR_CLASS_GENERICERROR (by a series of
string lookups done in the libxl string to enum value function) and
then a handwritten function to convert that to
ERROR_QMP_GENERIC_ERROR, which is in turn defined in the ERROR
enum in the idl ?

I don't think the first use of the idl generator (for
LIBXL__QMP_ERROR_CLASS) is really helping very much here.

How about this: introduce in the IDL only ERROR_QMP_* values like
ERROR_QMP_GENERIC_ERROR.  The idl compiler will make a table mapping
"QMP_GENERIC_ERROR" to ERROR_QMP_GENERIC_ERROR.

You can then write a custom lookup function
  int libxl_qmp_error_class_to_libxl_error_code
      (libxl_gc *gc, const char *eclass)
which:
 * Iterates over that table, looking for strings in the table
   which start QMP_.  So it finds at "QMP_GENERIC_ERROR".
 * If an entry does start with QMP_, it uses a custom string
   comparison routine which ignores case, and skips underscores
   in the libxl error code string.  So it matches like this:
       QMP_GENERIC_ERROR
           Generic Error
 * If no matching entry is found it logs the real QMP
   error string and returns ERROR_UNKNOWN_QMP_ERROR
   (which must not be called LIBXL_QMP_UNKNOWN_ERROR in
   case QMP has or grows an "UnknownError" error code).

This is not the fastest approach imaginable but it's no worse than the
existing O(n) error string lookup and I hope this isn't a hot path.


> > > +/* return 1 when a user callback as been called */
> > > +static int qmp_ev_handle_response(libxl__egc *egc,
> > > +                                  libxl__ev_qmp *ev,
> > > +                                  const libxl__json_object *resp,
> > > +                                  libxl__qmp_message_type type)
> > > +{
> > > +    EGC_GC;
> > > +    const libxl__json_object *response;
> > > +    const libxl__json_object *o;
> > > +    int rc;
> > > +    int id;
> > > +
> > > +    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);
> > 
> > What is the dead store from "error" doing ?
> 
> It's not dead, I reuse "o", as I get "desc" from "o".
> I could just create new variables here instead of reusing the same one
> (o) over and over again, e.g. obj_id, obj_error, obj_desc.

Oh.  Sorry.  Yes.  The idiom with the reuse of `o' is fine, I think,
but:

> Should I add here that we are parsing: {"error":{"desc": X }} ?

That would be lovely.  In general that would make these functions much
easier to read.

> > > +    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
> > > +        const char *s;
> > > +        const libxl__json_object *err;
> > > +        libxl__qmp_error_class error_class;
> > > +
> > > +        rc = ERROR_FAIL;
> > > +        response = NULL;
> > > +
> > > +        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))
> > > +            rc = qmp_error_class_to_libxl_error_code(error_class);
> > > +
> > > +        o = libxl__json_map_get("desc", err, JSON_STRING);
> > > +        s = libxl__json_object_get_string(o);
> > > +        if (s)
> > > +            LOGD(ERROR, ev->domid, "%s", s);
> > > +
> > > +        break;
> > 
> > Surely this should print more debugging (or maybe even error log
> > messages) if the error json document was not in the expected form ?
> 
> Do you mean that if the QMP server doesn't respect the QMP protocol, I
> should print more debug here?
> 
> If we are at this point in the programme, we know that the server sent:
> {"error": X }
> 
> I'm tempted to say that if "class" or "desc" isn't present, we could say
> so and declare the server broken (and disconnect).

Yes.  Precisely.  But I think what happens with your current code is
that it falls through to the bottom with little in the way of
diagnostics.

One approach would be to add an error logging string parameter to
libxl__json_*_get, which (if non-null, and the input object is
non-null) logs an appropriate complaint including the logging string
parameter and the looked-up key.

> > > +        /* Findout how much can be parsed */
> > > +        size_t len = end - s;
> > > +
> > > +        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
> > > +
> > > +        /* Replace \n by \0 so that libxl__json_parse can use strlen */
> > > +        s[len - 1] = '\0';
> > 
> > Doesn't this feed the \r to libxl__json_parse ?
> > 
> > Also, why does this have to be a loop ?  Does qemu really send
> > multiple json documents end to end, and only sometimes with
> > intervening \r\n ?  Does it ever send a json document without \r\n at
> > the end and then stop speaking for a while ?
> 
> QEMU will send two messages at once, e.g. when asking to eject or change
> the cdrom, QEMU will often send the response to our command and an event
> at the same time. So we need a loop to parse all messages received.
> 
> But there is always a \r\n at the end of every commands.

So I think this is more complicated than it needs to be - and more
relaxed than it ought to be.

In my earlier comment I suggested that qmp_ev_callback_readable (or
its parent) needs to keep calling read() until it gets EWOULDBLOCK.  I
still think that's right.  If you do that, then you don't need this
loop either, because if you structure things right, the outer loop can
do it.

I tried to find an example of how to do this but I couldn't find a
simple one.  I guess it is OK to do it with nested loops.

But I think you should treat only `\n' as the delimiter.  This will
considerably simplify the buffer handling.  (You should check and trim
the preceding `\r' before passing things to libxl__json_parse of
course.)

> > > +        ev->buf_consumed += len;
> > > +
> > > +        if (ev->buf_consumed >= ev->buf_used) {
> > > +            free(ev->rx_buf);
> > 
> > Surely buf_consumed <= buf_used ?  Maybe you should assert that.
> 
> I'm attempting to detect here if there's something left in the buffer
> that hasn't been parsed yet. If there is nothing left, we can free the
> buffer.
> 
> Or do you mean that the condition should be buf_consumed == buf_used ?

It felt odd to me to read `ev->buf_consumed >= ev->buf_used' when I
knew that buf_consumed <= buf_used.  I am suggesting adding
  assert(buf_consumed <= buf_used);
and then writing
  if (ev->buf_consumed == ev->buf_used)

To be honest this condition is a bit fiddly.  Why not do this after
subtracting buf_consumed from buf_used and copying the old data down ?
Then it would read
  if (!ev->buf_used)

But even better, if you change your code to permit ev->rx_buf != NULL
when ev->rx_buf_used==0, this code goes away completely.

See my comments about permissible states of the buffer setup.

> > > +static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
> > > +{
> > > +    int rc;
> > > +    char *buf;
> > > +    size_t len;
> > > +    int send_fd = -1;
> > > +
> > > +    /* No need to call qmp_ev_callback_writable again, everything that needs to
> > > +     * be send for now will be in this call. */
> > 
> > So you guarantee never to send any message which is too large to fit
> > into a socket buffer ?  Do you know what that length is ?
> 
> I'm using libxl_write_exactly().

Yes.  The fd is in nonblocking mode.  If you call libxl_write_exactly
with more data than will fit into the socket buffer,
libxl_write_exactly will bomb out with EWOULDBLOCK (which it treats as
a fatal error).  It can't do anything else.

IDK if this is going to be a problem, but it is a significant
limitation of this function.  At the very least you should add an
assert that the message is small, along with some commentary about the
size of socket buffers.

If that is not suitable, the usual approach is to prepare and store a
buffer containing the data to be sent, and then walk across it with a
series of write calls.  Like the aoutils datacopier.  (You could
perhaps use the aoutils datacopier with libxl__datacopier_prefixdata
if you wanted, but you'd have to find a /dev/null to pass for
readfd...)

> On the other end, I've just realize that I'm also using
> libxl__sendmsg_fds(), which I guess may not send everything. But the
> function also doesn't seems to the caller know how much have been sent.

libxl_write_exactly does not return until it has completed and if it
gets an errno other than EINTR it calls it failure.  Likewise
libxl_sendmsg_fds.

If you want to send fds in the middle of a long stream, where the
operation might block, you need to decide which byte the fds should be
associated with, and then call libxl__sendmsg_fds separately for just
that byte (and, presumably, write(2)) for the others.  You'll need to
enhance libxl__sendmsg_fds to understand that EWOULDBLOCK should be
handled by its caller (and maybe that in that case there should be a
check that datalen==1).

> Should I keep using libxl_write_exactly() ?
> I'm going to need to be able to track how much data have been sent for
> at least sendmsg() anyway, and try again later.

You can't use libxl_write_exactly unless you can prove it won't block.

> > > +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> > > +{
> > > +    EGC_GC;
> > > +
> > > +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> > > +
> > > +    /* On error, deallocate all private ressources */
> > > +    libxl__ev_qmp_dispose(gc, ev);
> > 
> > This surely needs to set the state.  Presumably that should be done in
> > libxl__ev_qmp_dispose but AFAICT it isn't.
> 
> It is done by libxl__ev_qmp_init, which _dispose calls, itn't that
> enough?

Maybe.  That depends on your table of permissible internal and
external states.

> > Going back a bit, earlier we had:
> > 
> > > +#define QMP_CAPABILITY_NEGOTIATION_MSGID 1
> > 
> > I recommend using a different value.  Can we safely let this wrap ?
> > If so maybe we could use 0x786c7100 which spells "xlq\0" or something.
> > This can make it easier to see where rogue values are coming from, to
> > distinguish arguments in debug output, etc.
> 
> ID can be string, or any json value.

Oh!

> Maybe we could have
> {"libxl-id": number} as "id"?

Making it a whole json object seems a bit overkill.  How about
  GCSPRINTF("libxl-id-%lu", id++)
?

Or did you intend to fish the id out and treat it, in the libxl code,
as just an integer ?

> But 0x786c7100 as starting point would work fine as well.
> I don't think it matter if the value wrap. Beside adding 1 to generate a
> new id, and compare, I don't think the id should be use for anything
> else.

Right.

I think TBH (uint32_t)0x786c7100 is probably good and simple.

Ian.

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Anthony PERARD]

On Mon, Oct 29, 2018 at 05:31:59PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]"):
> > On Mon, Oct 15, 2018 at 05:35:36PM +0100, Ian Jackson wrote:
> > > > +        ev->buf_consumed += len;
> > > > +
> > > > +        if (ev->buf_consumed >= ev->buf_used) {
> > > > +            free(ev->rx_buf);
> > > 
> > > Surely buf_consumed <= buf_used ?  Maybe you should assert that.
> > 
> > I'm attempting to detect here if there's something left in the buffer
> > that hasn't been parsed yet. If there is nothing left, we can free the
> > buffer.
> > 
> > Or do you mean that the condition should be buf_consumed == buf_used ?
> 
> It felt odd to me to read `ev->buf_consumed >= ev->buf_used' when I
> knew that buf_consumed <= buf_used.  I am suggesting adding
>   assert(buf_consumed <= buf_used);
> and then writing
>   if (ev->buf_consumed == ev->buf_used)
> 
> To be honest this condition is a bit fiddly.  Why not do this after
> subtracting buf_consumed from buf_used and copying the old data down ?

Somehow, that feels wrong to move the data inside the buffer.

> Then it would read
>   if (!ev->buf_used)
> 
> But even better, if you change your code to permit ev->rx_buf != NULL
> when ev->rx_buf_used==0, this code goes away completely.

I guess you mean to keep the buffer even once there's nothing left to
parse? (And only free it once we call _dispose().)


-- 
Anthony PERARD

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

Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]"):
> On Mon, Oct 29, 2018 at 05:31:59PM +0000, Ian Jackson wrote:
> > To be honest this condition is a bit fiddly.  Why not do this after
> > subtracting buf_consumed from buf_used and copying the old data down ?
> 
> Somehow, that feels wrong to move the data inside the buffer.

You are going to have to move the data sometimes.  Suppose qemu keeps
sending you half messages and then pausing.  You'll never have an
empty buffer.

> > Then it would read
> >   if (!ev->buf_used)
> > 
> > But even better, if you change your code to permit ev->rx_buf != NULL
> > when ev->rx_buf_used==0, this code goes away completely.
> 
> I guess you mean to keep the buffer even once there's nothing left to
> parse? (And only free it once we call _dispose().)

Yes.  It seems simpler.

Ian.

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

[PATCH v5 04/15] libxl_qmp: Parse JSON input from QMP

[Anthony PERARD]

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

Notes:
    v5:
        initialize len and s at declaration time
        remove old comment
    
    v4:
        simplification of the patch due to use of a single allocated space for the
        receive buffer.

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 9ea303edf0..3817c70830 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1296,6 +1296,7 @@ static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
 {
     EGC_GC;
     ssize_t r;
+    char *end = NULL;
 
     if (!ev->rx_buf) {
         ev->rx_buf = libxl__malloc(NOGC, QMP_RECEIVE_BUFFER_SIZE);
@@ -1333,6 +1334,57 @@ static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
     ev->buf_used += r;
     assert(ev->buf_used < ev->buf_size);
 
+    /* workaround strstr limitation */
+    ev->rx_buf[ev->buf_used] = '\0';
+
+    /*
+     * Search for the end of a QMP message: "\r\n" in the newly received
+     * bytes + the last byte on the previous read() if any
+     *
+     * end: This will point to the byte right after \r\n
+     */
+    end = strstr(ev->rx_buf + ev->buf_used - r
+                 + (ev->buf_used - r == 0 ? 0 : -1),
+                 "\r\n");
+    if (end)
+        end += 2;
+
+    while (end) {
+        libxl__json_object *o;
+        /* Start parsing from s */
+        char *s = ev->rx_buf + ev->buf_consumed;
+        /* Findout how much can be parsed */
+        size_t len = end - s;
+
+        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
+
+        /* Replace \n by \0 so that libxl__json_parse can use strlen */
+        s[len - 1] = '\0';
+        o = libxl__json_parse(gc, s);
+
+        if (!o) {
+            LOGD(ERROR, ev->domid, "Parse error");
+            return ERROR_FAIL;
+        }
+
+        ev->buf_consumed += len;
+
+        if (ev->buf_consumed >= ev->buf_used) {
+            free(ev->rx_buf);
+            ev->rx_buf = NULL;
+        }
+
+        /* check if there is another message received at the same time */
+        if (ev->rx_buf) {
+            end = strstr(ev->rx_buf + ev->buf_consumed, "\r\n");
+            if (end)
+                end += 2;
+        } else
+            end = NULL;
+
+        LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, o));
+    }
+
     return 0;
 }
 
-- 
Anthony PERARD


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

[PATCH v5 05/15] libxl_qmp: Separate QMP message generation from qmp_send_prepare

[Anthony PERARD]

To be able to re-use qmp_prepare_qmp_cmd with 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>
---

Notes:
    v5:
        rename qmp_prepare_qmp_cmd to qmp_prepare_cmd
        fix coding style

 tools/libxl/libxl_qmp.c | 56 ++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 3817c70830..a537dd13e8 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -577,17 +577,15 @@ 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_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);
 
@@ -604,7 +602,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);
@@ -614,6 +612,34 @@ 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_cmd(gc, cmd, args, ++qmp->last_id_used, NULL);
+
+    if (!buf) {
         LOGD(ERROR, qmp->domid, "Failed to generate a qmp command");
         goto out;
     }
@@ -629,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,
@@ -647,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;
@@ -656,12 +680,10 @@ static int qmp_send(libxl__qmp_handler *qmp,
     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;
 
     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

[PATCH v5 06/15] libxl_qmp: Prepare the command to be sent

[Anthony PERARD]

The actual sent will be done in a separate patch.

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

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0590801b30..0f3eda249a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -439,6 +439,7 @@ struct libxl__ev_qmp {
     libxl__carefd *qmp_cfd;
     libxl__ev_fd qmp_efd;
     libxl__qmp_state qmp_state;
+    unsigned int last_id_used;
     /* receive buffer, with:
      * buf_size: current allocated size,
      * buf_used: actual data in the buffer,
@@ -447,6 +448,8 @@ struct libxl__ev_qmp {
     size_t buf_size;
     size_t buf_used;
     size_t buf_consumed;
+    char *tx_buf;
+    size_t tx_buf_len;
 };
 
 
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index a537dd13e8..b5cc0af513 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1310,6 +1310,25 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
 
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
+static int qmp_ev_prepare_cmd(libxl__gc *gc,
+                              libxl__ev_qmp *ev,
+                              const char *cmd,
+                              const libxl__json_object *args)
+{
+    char *buf = NULL;
+    size_t len;
+
+    buf = qmp_prepare_cmd(gc, cmd, args, ++ev->last_id_used, &len);
+    if (!buf)
+        return ERROR_FAIL;
+
+    ev->id = ev->last_id_used;
+    ev->tx_buf = buf;
+    ev->tx_buf_len = len;
+
+    return 0;
+}
+
 /*
  * QMP FD callbacks
  */
@@ -1418,6 +1437,9 @@ static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
 
     /* On error, deallocate all private ressources */
     libxl__ev_qmp_dispose(gc, ev);
+
+    ev->id = -1;
+    ev->callback(egc, ev, NULL, ERROR_FAIL);
 }
 
 static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
@@ -1515,17 +1537,32 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qmp_cfd = NULL;
     libxl__ev_fd_init(&ev->qmp_efd);
     ev->qmp_state = qmp_state_disconnected;
+    ev->last_id_used = 0;
 
     ev->rx_buf = NULL;
+    ev->tx_buf = NULL;
 }
 
 int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
                        const char *cmd, libxl__json_object *args)
 {
+    int rc;
+
     LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
 
     /* Connect to QEMU if not already connected */
-    return qmp_ev_connect(gc, ev);
+    rc = qmp_ev_connect(gc, ev);
+    if (rc)
+        goto out;
+
+    rc = qmp_ev_prepare_cmd(gc, ev, cmd, args);
+    if (rc)
+        goto out;
+
+out:
+    if (rc)
+        libxl__ev_qmp_dispose(gc, ev);
+    return rc;
 }
 
 void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
@@ -1533,6 +1570,7 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
     LOGD(DEBUG, ev->domid, " ev %p", ev);
 
     free(ev->rx_buf);
+    free(ev->tx_buf);
 
     libxl__ev_fd_deregister(gc, &ev->qmp_efd);
     libxl__carefd_close(ev->qmp_cfd);
-- 
Anthony PERARD


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

[PATCH v5 07/15] libxl_qmp: Handle write to QMP socket

[Anthony PERARD]

The libxl__ev_qmp_* will now send the command to QEMU when the socket is
ready for writes.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---

Notes:
    v5:
        rename buf_fd to send_fd

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index b5cc0af513..351fb1a292 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1326,6 +1326,8 @@ static int qmp_ev_prepare_cmd(libxl__gc *gc,
     ev->tx_buf = buf;
     ev->tx_buf_len = len;
 
+    libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);
+
     return 0;
 }
 
@@ -1429,6 +1431,43 @@ static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
     return 0;
 }
 
+static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
+{
+    int rc;
+    char *buf;
+    size_t len;
+    int send_fd = -1;
+
+    /* No need to call qmp_ev_callback_writable again, everything that needs to
+     * be send for now will be in this call. */
+    libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events & ~POLLOUT);
+
+    if (ev->qmp_state == qmp_state_connected) {
+        if (!ev->tx_buf)
+            return 0;
+
+        buf = ev->tx_buf;
+        len = ev->tx_buf_len;
+        send_fd = libxl__carefd_fd(ev->cfd);
+
+        ev->tx_buf = NULL;
+    } else {
+        return 0;
+    }
+
+    LOG_QMP("sending: '%.*s'", (int)len, buf);
+
+    if (send_fd >= 0) {
+        rc = libxl__sendmsg_fds(gc, fd, buf, len, 1, &send_fd, "QMP socket");
+    } else {
+        rc = libxl_write_exactly(CTX, fd, buf, len,
+                                 "QMP command", "QMP socket");
+    }
+
+    free(buf);
+    return rc;
+}
+
 static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
 {
     EGC_GC;
@@ -1464,6 +1503,11 @@ static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
         goto out;
     }
 
+    if (revents & POLLOUT) {
+        rc = qmp_ev_callback_writable(gc, ev, fd);
+        if (rc)
+            goto out;
+    }
     if (revents & POLLIN) {
         rc = qmp_ev_callback_readable(egc, ev, fd);
         if (rc)
-- 
Anthony PERARD


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

[PATCH v5 08/15] libxl_qmp: Handle messages from QEMU

[Anthony PERARD]

This patch will handle messages received, and calls callbacks associated
with the libxl__ev_qmp when the expected response is received.

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

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

Notes:
    v5:
        Adding default:abort() in qmp_ev_handle_message.

 tools/libxl/libxl_qmp.c              | 119 +++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl          |   4 +
 tools/libxl/libxl_types_internal.idl |   8 ++
 3 files changed, 131 insertions(+)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 351fb1a292..ffbc055110 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1331,6 +1331,121 @@ static int qmp_ev_prepare_cmd(libxl__gc *gc,
     return 0;
 }
 
+/*
+ * Handle messages received from QMP server
+ */
+
+static int qmp_error_class_to_libxl_error_code(const libxl__qmp_error_class c)
+{
+    switch (c) {
+    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
+        return ERROR_QMP_GENERIC_ERROR;
+    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
+        return ERROR_QMP_COMMAND_NOT_FOUND;
+    case LIBXL__QMP_ERROR_CLASS_DEVICENOTACTIVE:
+        return ERROR_QMP_DEVICE_NOT_ACTIVE;
+    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
+        return ERROR_QMP_DEVICE_NOT_FOUND;
+    default:
+        abort();
+    }
+}
+
+/* return 1 when a user callback as been called */
+static int qmp_ev_handle_response(libxl__egc *egc,
+                                  libxl__ev_qmp *ev,
+                                  const libxl__json_object *resp,
+                                  libxl__qmp_message_type type)
+{
+    EGC_GC;
+    const libxl__json_object *response;
+    const libxl__json_object *o;
+    int rc;
+    int id;
+
+    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, ev->domid, "%s", error_desc);
+        else
+            LOGD(ERROR, ev->domid, "Received unexpected message: %s",
+                 libxl__json_object_to_json(gc, resp));
+        return 0;
+    }
+
+    id = libxl__json_object_get_integer(o);
+    if (id != ev->id)
+        return 0;
+
+    switch (type) {
+    case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
+        response = libxl__json_map_get("return", resp, JSON_ANY);
+        rc = 0;
+        break;
+    }
+    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
+        const char *s;
+        const libxl__json_object *err;
+        libxl__qmp_error_class error_class;
+
+        rc = ERROR_FAIL;
+        response = NULL;
+
+        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))
+            rc = qmp_error_class_to_libxl_error_code(error_class);
+
+        o = libxl__json_map_get("desc", err, JSON_STRING);
+        s = libxl__json_object_get_string(o);
+        if (s)
+            LOGD(ERROR, ev->domid, "%s", s);
+
+        break;
+    }
+    default:
+        abort();
+    }
+
+    ev->id = -1;
+    ev->callback(egc, ev, response, rc); /* must be last */
+    return 1;
+}
+
+/* return 1 when a user callback as been called */
+static int qmp_ev_handle_message(libxl__egc *egc,
+                                 libxl__ev_qmp *ev,
+                                 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 0;
+    case LIBXL__QMP_MESSAGE_TYPE_RETURN:
+    case LIBXL__QMP_MESSAGE_TYPE_ERROR:
+        return qmp_ev_handle_response(egc, ev, resp, type);
+    case LIBXL__QMP_MESSAGE_TYPE_EVENT:
+        /* Event are ignored */
+        return 0;
+    case LIBXL__QMP_MESSAGE_TYPE_INVALID:
+        return 0;
+    default:
+        abort();
+    }
+
+    return 0;
+}
+
 /*
  * QMP FD callbacks
  */
@@ -1426,6 +1541,10 @@ static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
             end = NULL;
 
         LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, o));
+
+        /* Must be last and return when the user callback is called */
+        if (qmp_ev_handle_message(egc, ev, o) == 1)
+            return 0;
     }
 
     return 0;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 4a385801ba..e104fea941 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", [
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 37402e49cb..1536d02215 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -13,6 +13,14 @@ libxl__qmp_message_type = Enumeration("qmp_message_type", [
     (5, "invalid"),
     ])
 
+libxl__qmp_error_class = Enumeration("qmp_error_class", [
+    # QMP error classes described in QEMU sources code (QapiErrorClass)
+    (1, "GenericError"),
+    (2, "CommandNotFound"),
+    (3, "DeviceNotActive"),
+    (4, "DeviceNotFound"),
+    ])
+
 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 v5 09/15] libxl_qmp: Respond to QMP greeting

[Anthony PERARD]

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

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index ffbc055110..90308b1598 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1310,6 +1310,9 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
 
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
+/* hard coded message ID used for capability negotiation */
+#define QMP_CAPABILITY_NEGOTIATION_MSGID 1
+
 static int qmp_ev_prepare_cmd(libxl__gc *gc,
                               libxl__ev_qmp *ev,
                               const char *cmd,
@@ -1380,7 +1383,7 @@ static int qmp_ev_handle_response(libxl__egc *egc,
     }
 
     id = libxl__json_object_get_integer(o);
-    if (id != ev->id)
+    if (id != ev->id && id != QMP_CAPABILITY_NEGOTIATION_MSGID)
         return 0;
 
     switch (type) {
@@ -1414,9 +1417,21 @@ static int qmp_ev_handle_response(libxl__egc *egc,
         abort();
     }
 
-    ev->id = -1;
-    ev->callback(egc, ev, response, rc); /* must be last */
-    return 1;
+    /*
+     * Even if the current state is capability_negotiation and the correct ID
+     * as been received, call the callback on error.
+     */
+    if (ev->qmp_state == qmp_state_capability_negotiation &&
+        id == QMP_CAPABILITY_NEGOTIATION_MSGID &&
+        rc == 0) {
+        ev->qmp_state = qmp_state_connected;
+        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);
+        return 0;
+    } else {
+        ev->id = -1;
+        ev->callback(egc, ev, response, rc); /* must be last */
+        return 1;
+    }
 }
 
 /* return 1 when a user callback as been called */
@@ -1430,6 +1445,11 @@ static int qmp_ev_handle_message(libxl__egc *egc,
     switch (type) {
     case LIBXL__QMP_MESSAGE_TYPE_QMP:
         /* greeting message */
+        assert(ev->qmp_state == qmp_state_connecting);
+        ev->qmp_state = qmp_state_greeting;
+        /* Allow qmp_ev_callback_writable to be called in order to send
+         * qmp_capabilities */
+        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | POLLOUT);
         return 0;
     case LIBXL__QMP_MESSAGE_TYPE_RETURN:
     case LIBXL__QMP_MESSAGE_TYPE_ERROR:
@@ -1561,7 +1581,13 @@ static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
      * be send for now will be in this call. */
     libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events & ~POLLOUT);
 
-    if (ev->qmp_state == qmp_state_connected) {
+    switch (ev->qmp_state) {
+    case qmp_state_greeting:
+        buf = qmp_prepare_cmd(gc, "qmp_capabilities", NULL,
+                              QMP_CAPABILITY_NEGOTIATION_MSGID, &len);
+        ev->qmp_state = qmp_state_capability_negotiation;
+        break;
+    case qmp_state_connected:
         if (!ev->tx_buf)
             return 0;
 
@@ -1570,7 +1596,8 @@ static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
         send_fd = libxl__carefd_fd(ev->cfd);
 
         ev->tx_buf = NULL;
-    } else {
+        break;
+    default:
         return 0;
     }
 
@@ -1700,7 +1727,7 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
     ev->qmp_cfd = NULL;
     libxl__ev_fd_init(&ev->qmp_efd);
     ev->qmp_state = qmp_state_disconnected;
-    ev->last_id_used = 0;
+    ev->last_id_used = QMP_CAPABILITY_NEGOTIATION_MSGID + 1;
 
     ev->rx_buf = NULL;
     ev->tx_buf = NULL;
-- 
Anthony PERARD


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

[PATCH v5 10/15] libxl_exec: Add libxl__spawn_initiate_failure

[Anthony PERARD]

This function can be used by user of libxl__spawn_* when they setup a
notification other than xenstore. The parent can already report success
via libxl__spawn_initiate_detach(), this new function can be used for
failure instead of waiting for the timeout.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---

Notes:
    v5:
        fix typos

 tools/libxl/libxl_exec.c     |  7 +++++++
 tools/libxl/libxl_internal.h | 21 ++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 02e6c917f0..fb9621b10a 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -373,6 +373,13 @@ void libxl__spawn_initiate_detach(libxl__gc *gc, libxl__spawn_state *ss)
     spawn_detach(gc, ss);
 }
 
+void libxl__spawn_initiate_failure(libxl__gc *gc, libxl__spawn_state *ss, int rc)
+{
+    assert(rc);
+    ss->rc = rc;
+    spawn_detach(gc, ss);
+}
+
 static void spawn_fail(libxl__egc *egc, libxl__spawn_state *ss, int rc)
 /* Caller must have logged.  Must be last thing in calling function,
  * as it may make the callback.  Precondition: Attached or Detaching. */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0f3eda249a..b6c0279bb8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1550,7 +1550,8 @@ _hidden void libxl__spawn_init(libxl__spawn_state*);
  *
  * The inner child must soon exit or exec.  It must also soon exit or
  * notify the parent of its successful startup by writing to the
- * xenstore path xspath.
+ * xenstore path xspath OR via other means that the parent will have
+ * to setup.
  *
  * The user (in the parent) will be called back (confirm_cb) every
  * time that xenstore path is modified.
@@ -1606,6 +1607,24 @@ _hidden int libxl__spawn_spawn(libxl__egc *egc, libxl__spawn_state *spawn);
  */
 _hidden void libxl__spawn_initiate_detach(libxl__gc *gc, libxl__spawn_state*);
 
+/*
+ * libxl__spawn_initiate_failure - Propagate failure from the caller to the
+ * callee.
+ *
+ * Works by killing the intermediate process from spawn_spawn.
+ * After this function returns, failures will be reported via failure_cb.
+ *
+ * This is not synchronous: there will be a further callback when
+ * the detach is complete.
+ *
+ * Logs errors.
+ *
+ * The spawn state must be Attached entry and will be Attached Failed
+ * on return.
+ */
+_hidden void libxl__spawn_initiate_failure(libxl__gc *gc,
+                                           libxl__spawn_state *ss, int rc);
+
 /*
  * If successful, this should return 0.
  *
-- 
Anthony PERARD


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

Re: [PATCH v5 10/15] libxl_exec: Add libxl__spawn_initiate_failure

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 10/15] libxl_exec: Add libxl__spawn_initiate_failure"):
> This function can be used by user of libxl__spawn_* when they setup a
> notification other than xenstore. The parent can already report success
> via libxl__spawn_initiate_detach(), this new function can be used for
> failure instead of waiting for the timeout.

Thanks.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 0f3eda249a..b6c0279bb8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1550,7 +1550,8 @@ _hidden void libxl__spawn_init(libxl__spawn_state*);
>   *
>   * The inner child must soon exit or exec.  It must also soon exit or
>   * notify the parent of its successful startup by writing to the
> - * xenstore path xspath.
> + * xenstore path xspath OR via other means that the parent will have
> + * to setup.

      to set up

> +/*
> + * libxl__spawn_initiate_failure - Propagate failure from the caller to the
> + * callee.
> + *
> + * Works by killing the intermediate process from spawn_spawn.
> + * After this function returns, failures will be reported via failure_cb.

      After this function returns, a failure will be reported

> + * The spawn state must be Attached entry and will be Attached Failed
> + * on return.

                              Attached on entry

And, there is no such public state as `Attached Failed'.  That is a
private state.  The comment about libxl__spawn_initiate_failure
putting the spawn into state Attached Failed should be in
libxl_exec.c, next to that function.

Writing that the state afterwards is still Attached makes it clear
that this function may be called multiple times.  How about:

      The spawn state must be Attached on entry and will remain
      Attached.  It is possible for a spawn to fail for multiple
      reasons, for example call(s) to libxl__spawn_initiate_failure
      and also for some other reason.  In that case the last rc value
      from any source will take precedence.

But is that really the semantics we want?  Maybe the first or last
call to libxl__spawn_initiate_failure should take precedence ?

Iqn.

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

Re: [PATCH v5 10/15] libxl_exec: Add libxl__spawn_initiate_failure

[Anthony PERARD]

On Tue, Oct 16, 2018 at 03:02:37PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 10/15] libxl_exec: Add libxl__spawn_initiate_failure"):
> > + * The spawn state must be Attached entry and will be Attached Failed
> > + * on return.
> 
>                               Attached on entry
> 
> And, there is no such public state as `Attached Failed'.  That is a
> private state.  The comment about libxl__spawn_initiate_failure
> putting the spawn into state Attached Failed should be in
> libxl_exec.c, next to that function.
> 
> Writing that the state afterwards is still Attached makes it clear
> that this function may be called multiple times.  How about:
> 
>       The spawn state must be Attached on entry and will remain
>       Attached.  It is possible for a spawn to fail for multiple
>       reasons, for example call(s) to libxl__spawn_initiate_failure
>       and also for some other reason.  In that case the last rc value
>       from any source will take precedence.
> 
> But is that really the semantics we want?  Maybe the first or last
> call to libxl__spawn_initiate_failure should take precedence ?

"The first rc value [...] will take precedence" sound better I think,
instead of the last.

I'll modify libxl__spawn_initiate_failure to only set ss->rc if it it
isn't already.

-- 
Anthony PERARD

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

[PATCH v5 11/15] 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:
    v5:
        use libxl__remove_file
        few changes in coding style
        remove stale includes (sys/socket, sys/un) which are now in libxl_internal.h
    
    v4:
        separate the logic to open a socket into a function.
        Use libxl__prepare_sockaddr_un() to check path size

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

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index abd31ee6f2..c6bf81b863 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -910,12 +910,54 @@ static char *qemu_disk_ide_drive_string(libxl__gc *gc, const char *target_path,
     return drive;
 }
 
+static int libxl__pre_open_qmp_socket(libxl__gc *gc, int domid, int *fd_r)
+{
+    int rc;
+    int fd;
+    struct sockaddr_un un;
+    const char *path = libxl__qemu_qmp_path(gc, domid);
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0) {
+        LOGED(ERROR, domid, "socket() failed");
+        return ERROR_FAIL;
+    }
+
+    rc = libxl__prepare_sockaddr_un(gc, &un, path, "QEMU's QMP socket");
+    if (rc)
+        goto out;
+
+    rc = libxl__remove_file(gc, path);
+    if (rc)
+        goto out;
+
+    if (bind(fd, (struct sockaddr*) &un, sizeof(un)) < 0) {
+        LOGED(ERROR, domid, "bind('%s') failed", path);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (listen(fd, 1) < 0) {
+        LOGED(ERROR, domid, "listen() failed");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    *fd_r = fd;
+    rc = 0;
+
+out:
+    if (rc && fd >= 0)
+        close(fd);
+    return rc;
+}
+
 static int libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         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;
@@ -944,10 +986,25 @@ 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,server,nowait",
-                               libxl__qemu_qmp_path(gc, 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 rc;
+
+        rc = libxl__pre_open_qmp_socket(gc, guest_domid, dm_monitor_fd);
+        if (rc)
+            return rc;
+
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
+                                   *dm_monitor_fd));
+    } else {
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxl-cmd,"
+                                   "path=%s,server,nowait",
+                                   libxl__qemu_qmp_path(gc, guest_domid)));
+    }
 
     flexarray_append(dm_args, "-no-shutdown");
     flexarray_append(dm_args, "-mon");
@@ -1726,7 +1783,7 @@ 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. */
 {
@@ -1739,10 +1796,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);
@@ -1963,7 +2021,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;
@@ -2249,6 +2307,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();
@@ -2266,7 +2325,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;
 
@@ -2364,6 +2424,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 v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 11/15] 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.

Thanks for this.  I have only tiny comments about this.  (I checked the
error handling patterns and they seemed conventional and correct.)

FAOD, and I think this is not entirely clear from your commit message:

AIUI the overall effect of this patch is not to enable any new
functionality and not to fix any bug.  It just moves the qemu socket
creation from qemu to libxl, but nothing in libxl relies on this yet.

Am I right ?  If so please put that in the commit message.

> +static int libxl__pre_open_qmp_socket(libxl__gc *gc, int domid, int *fd_r)
...
> +    if (bind(fd, (struct sockaddr*) &un, sizeof(un)) < 0) {
> +        LOGED(ERROR, domid, "bind('%s') failed", path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }

From libxl/CODING_STYLE:

    * Function calls which might fail (ie most function calls) are
      handled by putting the return/status value into a variable, and
      then checking it in a separate statement:
              char *dompath = libxl__xs_get_dompath(gc, bl->domid);
              if (!dompath) { rc = ERROR_FAIL; goto out; }

This needs changing throughout the series, I'm afraid.

>  static int libxl__build_device_model_args_new(libxl__gc *gc,
>                                          const char *dm, int guest_domid,
>                                          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)
...
> -                     GCSPRINTF("socket,id=libxl-cmd,"
> -                               "path=%s,server,nowait",
> -                               libxl__qemu_qmp_path(gc, guest_domid)));
> +    /* If we have to use dm_restrict, QEMU need to be new enough and will have

                                              needs

> +     * the new interface where we can pre-open the QMP socket. */
> +    if (libxl_defbool_val(b_info->dm_restrict))
> +    {

Misplaced brace, should be end of the previous line.

> @@ -1739,10 +1796,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);

Jolly good.  But I would be tempted to move or copy this assert to
libxl__pre_open_qmp_socket.  What do you think ?

>      ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
>                                           guest_config, &args, NULL,
> -                                         d_state, NULL);
> +                                         d_state, NULL, NULL);

Did you consider adding dm_monitor_fd to d_state ?

Ian.

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

Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Anthony PERARD]

On Tue, Oct 16, 2018 at 03:11:03PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> >      ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> >                                           guest_config, &args, NULL,
> > -                                         d_state, NULL);
> > +                                         d_state, NULL, NULL);
> 
> Did you consider adding dm_monitor_fd to d_state ?

No, I didn't, I'm not sur what can go into d_state. But that seems
better that adding an argument to many function prototypes.

On the other hand, I would need to make sure that dm_monitor_fd have a
proper initial value (-1) when needed.

-- 
Anthony PERARD

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

Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> On Tue, Oct 16, 2018 at 03:11:03PM +0100, Ian Jackson wrote:
> > Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> > >      ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> > >                                           guest_config, &args, NULL,
> > > -                                         d_state, NULL);
> > > +                                         d_state, NULL, NULL);
> > 
> > Did you consider adding dm_monitor_fd to d_state ?
> 
> No, I didn't, I'm not sur what can go into d_state. But that seems
> better that adding an argument to many function prototypes.

I think pretty much anything to do with creating a domain can go in
there.

> On the other hand, I would need to make sure that dm_monitor_fd have a
> proper initial value (-1) when needed.

Yes, it would have to be initialised along with the other members of
libxl__domain_build_state.

Ian.

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

Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Anthony PERARD]

On Mon, Nov 12, 2018 at 03:14:30PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> > On Tue, Oct 16, 2018 at 03:11:03PM +0100, Ian Jackson wrote:
> > > Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> > > >      ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
> > > >                                           guest_config, &args, NULL,
> > > > -                                         d_state, NULL);
> > > > +                                         d_state, NULL, NULL);
> > > 
> > > Did you consider adding dm_monitor_fd to d_state ?
> > 
> > No, I didn't, I'm not sur what can go into d_state. But that seems
> > better that adding an argument to many function prototypes.
> 
> I think pretty much anything to do with creating a domain can go in
> there.
> 
> > On the other hand, I would need to make sure that dm_monitor_fd have a
> > proper initial value (-1) when needed.
> 
> Yes, it would have to be initialised along with the other members of
> libxl__domain_build_state.

I didn't manage to findout where this might be. There is
libxl__build_pre() but I don't know if it's always called. Is this the
right place, or is it initialised somewhere else?

Also, now I would probably need to call libxl__pre_open_qmp_socket()
from libxl__spawn_local_dm() as the d_state (libxl_domain_build_info) is
const when passed to libxl__build_device_model_args. But that is
probably fine.

-- 
Anthony PERARD

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

Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU"):
> On Mon, Nov 12, 2018 at 03:14:30PM +0000, Ian Jackson wrote:
> > Yes, it would have to be initialised along with the other members of
> > libxl__domain_build_state.
> 
> I didn't manage to findout where this might be. There is
> libxl__build_pre() but I don't know if it's always called. Is this the
> right place, or is it initialised somewhere else?

Eerrrgh.

Just went off on an absurd wild goose chase to try to answer this
question.  I think libxl__domain_build_state predates the current more
rigorous approach to async functions; indeed, I think it predates any
async stuff at all.

I did git-log -G and found this
  59f8f46a491c9bdc1ad3e0c5ae4f8b48068d13cd
  tools: libxl: remove libxl_domain_build_state from the IDL

AFAICT in that commit it works like this:

* There is a libxl__domain_build_state as a local variable in
  each of do_domain_create and libxl__create_stubdom, which are
  synchronous functions.

* In each case, the struct is passed by reference into
  libxl__domain_build.  libxl__domain_build does not initialise it
  before passing it to libxl__build_pre.  It is filled in as we go.

At some point, libxl_domain_build_state became a member of various
parent state structures.  Those parent state structures (and the loose
state) are all initialised to zero at allocation time.

In 50f8ba84a50ebf80dd22067a04062dbaaf2621ff
  libxl: arm: Fix build after c/s 74fd984ae
libxl__domain_make started to get a pointer to the
libxl__domain_build_state.

AFAICT I think currently libxl__domain_make is actually the first
thing that touches anything in libxl__domain_build_state.  So it
should initialise it.

The layering here is ... a mess.

> Also, now I would probably need to call libxl__pre_open_qmp_socket()
> from libxl__spawn_local_dm() as the d_state (libxl_domain_build_info) is
> const when passed to libxl__build_device_model_args. But that is
> probably fine.

d_state is libxl_domain_build_state isn't it ?
libxl_domain_build_info is d_info or some such.

Ian.

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

[PATCH v5 12/15] 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>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---

Notes:
    v5:
        removed empty success branch in device_model_qmp_cb()
        call libxl__ev_qmp_init() earlier in libxl__spawn_local_dm.
            otherwise the error path would use an uninitialised
            libxl__ev_qmp.
    
    v4:
        moved to libxl__dm_spawn_* from libxl__spawn_*

 tools/libxl/libxl_dm.c       | 51 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  1 +
 2 files changed, 52 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c6bf81b863..9e23274bd8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2278,6 +2278,8 @@ static void device_model_startup_failed(libxl__egc *egc,
                                         int rc);
 static void device_model_detached(libxl__egc *egc,
                                   libxl__spawn_state *spawn);
+static void device_model_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                const libxl__json_object *response, int rc);
 
 /* our "next step" function, called from those callbacks and elsewhere */
 static void device_model_spawn_outcome(libxl__egc *egc,
@@ -2309,6 +2311,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     int dm_state_fd = -1;
     int dm_monitor_fd = -1;
 
+    libxl__ev_qmp_init(&dmss->qmp);
+
     if (libxl_defbool_val(b_info->device_model_stubdomain)) {
         abort();
     }
@@ -2409,6 +2413,16 @@ retry_transaction:
     spawn->failure_cb = device_model_startup_failed;
     spawn->detached_cb = device_model_detached;
 
+    if (dm_monitor_fd >= 0) {
+        /* There is a valid QMP socket available now,
+         * use it to find out when QEMU is ready */
+        dmss->qmp.callback = device_model_qmp_cb;
+        dmss->qmp.domid = domid;
+        dmss->qmp.cfd = NULL;
+        rc = libxl__ev_qmp_send(gc, &dmss->qmp, "query-status", NULL);
+        if (rc) goto out_close;
+    }
+
     rc = libxl__spawn_spawn(egc, spawn);
     if (rc < 0)
         goto out_close;
@@ -2478,6 +2492,41 @@ static void device_model_detached(libxl__egc *egc,
     device_model_spawn_outcome(egc, dmss, 0);
 }
 
+static void device_model_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
+                                const libxl__json_object *response,
+                                int rc)
+{
+    EGC_GC;
+    libxl__dm_spawn_state *dmss = CONTAINER_OF(ev, *dmss, qmp);
+    const libxl__json_object *o;
+    const char *status;
+
+    libxl__ev_qmp_dispose(gc, ev);
+
+    if (rc)
+        goto failed;
+
+    o = libxl__json_map_get("status", response, JSON_STRING);
+    if (!o) {
+        LOGD(DEBUG, ev->domid, "QMP unexpected response");
+        rc = ERROR_FAIL;
+        goto failed;
+    }
+    status = libxl__json_object_get_string(o);
+    if (strcmp(status, "running")) {
+        LOGD(DEBUG, ev->domid, "Unexpected QEMU status: %s", status);
+        rc = ERROR_FAIL;
+        goto failed;
+    }
+
+    libxl__spawn_initiate_detach(gc, &dmss->spawn);
+    return;
+
+failed:
+    LOGD(ERROR, ev->domid, "QEMU did not start properly, rc=%d", rc);
+    libxl__spawn_initiate_failure(gc, &dmss->spawn, rc);
+}
+
 static void device_model_spawn_outcome(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc)
@@ -2501,6 +2550,8 @@ static void device_model_spawn_outcome(libxl__egc *egc,
         }
     }
 
+    libxl__ev_qmp_dispose(gc, &dmss->qmp);
+
  out:
     dmss->callback(egc, dmss, rc);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b6c0279bb8..bde34a3ac1 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3892,6 +3892,7 @@ struct libxl__dm_spawn_state {
     libxl_domain_config *guest_config;
     libxl__domain_build_state *build_state; /* relates to guest_domid */
     libxl__dm_spawn_cb *callback;
+    libxl__ev_qmp qmp;
 };
 
 _hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
-- 
Anthony PERARD


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

Re: [PATCH v5 12/15] libxl: QEMU startup sync based on QMP

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 12/15] 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"
...
> +static void device_model_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
> +                                const libxl__json_object *response,
> +                                int rc)
> +{
> +    EGC_GC;
> +    libxl__dm_spawn_state *dmss = CONTAINER_OF(ev, *dmss, qmp);
> +    const libxl__json_object *o;
> +    const char *status;
> +
> +    libxl__ev_qmp_dispose(gc, ev);

I missed who owns the memory for `response' ?  In the absence of any
indication to the contrary I think it should come from the EGC_GC.

...

Just checked qmp_ev_callback_readable and this is indeed true.  Jolly
good.

This actual functional change is pleasingly straightforward.

> +    if (rc)
> +        goto failed;
> +
> +    o = libxl__json_map_get("status", response, JSON_STRING);
> +    if (!o) {
> +        LOGD(DEBUG, ev->domid, "QMP unexpected response");

I think many of these DEBUG should be ERROR and many of these
ERROR_FAIL should be ERROR_QEMU_DID_SOME_WRONG_THING.

It's rather odd that neither this patch, nor anything that follows,
does not stop libxl from also watching the xenstore path.  I think it
would be better to suppress that rather than leave vestigial
behaviour.  Can you add patch(es) to do that, right after this one ?

You may need to add a patch to tolerate xspath==0 in the spawn code.

Thanks,
Ian.

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

Re: [PATCH v5 12/15] libxl: QEMU startup sync based on QMP

[Anthony PERARD]

On Tue, Oct 16, 2018 at 03:23:26PM +0100, Ian Jackson wrote:
> It's rather odd that neither this patch, nor anything that follows,
> does not stop libxl from also watching the xenstore path.  I think it
> would be better to suppress that rather than leave vestigial
> behaviour.  Can you add patch(es) to do that, right after this one ?

I can try. That also mean that we need to setup a new timeout that
xswait is setting up right now.

I don't really have an awnser on how to start timeout when we use
libxl__ev_qmp. I guess just adding a call to libxl__ev_xswatch_register
whenever we interact with libxl__ev_qmp is fine. I don't think we can do
something similair to xswait.

Do you have better ideas?

Thanks,

-- 
Anthony PERARD

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

Re: [PATCH v5 12/15] libxl: QEMU startup sync based on QMP

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 12/15] libxl: QEMU startup sync based on QMP"):
> On Tue, Oct 16, 2018 at 03:23:26PM +0100, Ian Jackson wrote:
> > It's rather odd that neither this patch, nor anything that follows,
> > does not stop libxl from also watching the xenstore path.  I think it
> > would be better to suppress that rather than leave vestigial
> > behaviour.  Can you add patch(es) to do that, right after this one ?
> 
> I can try. That also mean that we need to setup a new timeout that
> xswait is setting up right now.

Oh, good point.  That seems a bit tedious and fiddly.  Maybe it is
better to leave it until we can throw away the xswait entirely - since
that way there are, for now, fewer separate code paths in libxl.

> I don't really have an awnser on how to start timeout when we use
> libxl__ev_qmp. I guess just adding a call to libxl__ev_xswatch_register
> whenever we interact with libxl__ev_qmp is fine. I don't think we can do
> something similair to xswait.

I'm not sure I understand.  If you want to have a timeout that's not
part of an xswait, use libxl__ev_time.

Ian.

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

[PATCH v5 13/15] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp

[Anthony PERARD]

This will be used in a later patch.

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

Notes:
    v5:
        initialise qemu_version struct in libxl__ev_qmp_init

 tools/libxl/libxl_internal.h |  7 +++++++
 tools/libxl/libxl_qmp.c      | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index bde34a3ac1..0631ab1a03 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -434,6 +434,13 @@ struct libxl__ev_qmp {
     libxl__ev_qmp_callback *callback;
     libxl__carefd *cfd; /* set to send a fd with the command, NULL otherwise */
 
+    /* read-only when Connected */
+    struct {
+        int major;
+        int minor;
+        int micro;
+    } qemu_version;
+
     /* remaining fields are private to libxl_ev_qmp_* */
     int id;
     libxl__carefd *qmp_cfd;
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 90308b1598..77380a869c 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1440,12 +1440,28 @@ static int qmp_ev_handle_message(libxl__egc *egc,
                                  const libxl__json_object *resp)
 {
     EGC_GC;
+    const libxl__json_object *o;
     libxl__qmp_message_type type = qmp_response_type(resp);
 
     switch (type) {
     case LIBXL__QMP_MESSAGE_TYPE_QMP:
         /* greeting message */
         assert(ev->qmp_state == qmp_state_connecting);
+
+        /* Store advertised QEMU version */
+        o = libxl__json_map_get("QMP", resp, JSON_MAP);
+        o = libxl__json_map_get("version", o, JSON_MAP);
+        o = libxl__json_map_get("qemu", o, JSON_MAP);
+        ev->qemu_version.major = libxl__json_object_get_integer(
+            libxl__json_map_get("major", o, JSON_INTEGER));
+        ev->qemu_version.minor = libxl__json_object_get_integer(
+            libxl__json_map_get("minor", o, JSON_INTEGER));
+        ev->qemu_version.micro = libxl__json_object_get_integer(
+            libxl__json_map_get("micro", o, JSON_INTEGER));
+        LOGD(DEBUG, ev->domid, "QEMU version: %d.%d.%d",
+             ev->qemu_version.major, ev->qemu_version.minor,
+             ev->qemu_version.micro);
+
         ev->qmp_state = qmp_state_greeting;
         /* Allow qmp_ev_callback_writable to be called in order to send
          * qmp_capabilities */
@@ -1722,6 +1738,10 @@ out:
 
 void libxl__ev_qmp_init(libxl__ev_qmp *ev)
 {
+    ev->qemu_version.major = -1;
+    ev->qemu_version.minor = -1;
+    ev->qemu_version.micro = -1;
+
     ev->id = -1;
 
     ev->qmp_cfd = NULL;
-- 
Anthony PERARD


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

Re: [PATCH v5 13/15] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 13/15] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp"):
> This will be used in a later patch.
...
> +    /* read-only when Connected */
> +    struct {
> +        int major;
> +        int minor;
> +        int micro;
> +    } qemu_version;

This should surely say `and not to be accessed by the caller the rest
of the time' ?

> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 90308b1598..77380a869c 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1440,12 +1440,28 @@ static int qmp_ev_handle_message(libxl__egc *egc,
>                                   const libxl__json_object *resp)
>  {
>      EGC_GC;
> +    const libxl__json_object *o;
>      libxl__qmp_message_type type = qmp_response_type(resp);
>  
>      switch (type) {
>      case LIBXL__QMP_MESSAGE_TYPE_QMP:
>          /* greeting message */
>          assert(ev->qmp_state == qmp_state_connecting);
> +
> +        /* Store advertised QEMU version */
> +        o = libxl__json_map_get("QMP", resp, JSON_MAP);
> +        o = libxl__json_map_get("version", o, JSON_MAP);
> +        o = libxl__json_map_get("qemu", o, JSON_MAP);
> +        ev->qemu_version.major = libxl__json_object_get_integer(
> +            libxl__json_map_get("major", o, JSON_INTEGER));
> +        ev->qemu_version.minor = libxl__json_object_get_integer(
> +            libxl__json_map_get("minor", o, JSON_INTEGER));
> +        ev->qemu_version.micro = libxl__json_object_get_integer(
> +            libxl__json_map_get("micro", o, JSON_INTEGER));

This is quite repetitive.  Can we have a macro or a loop or
something ?  Maybe a macro so we can have

#define FISH_VSN(thing) ...
           FISH_VSN(major);
           FISH_VSN(minor);
           FISH_VSN(micro);
#undef FISH_VSN

or whatever you want to call it.

Ian.

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

[PATCH v5 14/15] libxl: Change libxl__domain_suspend_device_model() to be async.

[Anthony PERARD]

This create an extra step for the two call sites of the function.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

---
libxl_domain_soft_reset() haven't been tested, as it doesn't appear to
possible to call the function from xl.
---
 tools/libxl/libxl_create.c      | 30 ++++++++++++++++++++++--------
 tools/libxl/libxl_dom_suspend.c |  8 ++++++--
 tools/libxl/libxl_internal.h    |  4 +++-
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 60676304e9..4d42c2057a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1761,6 +1761,9 @@ error:
     domcreate_complete(egc, &cdcs->dcs, rc);
 }
 
+static void soft_reset_dm_suspended(libxl__egc *egc,
+                                    libxl__domain_suspend_state *dsps,
+                                    int rc);
 static int do_domain_soft_reset(libxl_ctx *ctx,
                                 libxl_domain_config *d_config,
                                 uint32_t domid_soft_reset,
@@ -1843,30 +1846,41 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
         goto out;
     }
 
-    rc = libxl__domain_suspend_device_model(gc, &dss->dsps);
+    dss->dsps.ao = ao;
+    dss->dsps.callback_device_model_done = soft_reset_dm_suspended;
+    rc = libxl__domain_suspend_device_model(egc, &dss->dsps);
     if (rc) {
         LOGD(ERROR, domid_soft_reset, "failed to suspend device model.");
         goto out;
     }
 
+    return AO_INPROGRESS;
+
+ out:
+    return AO_CREATE_FAIL(rc);
+}
+
+static void soft_reset_dm_suspended(libxl__egc *egc,
+                                    libxl__domain_suspend_state *dsps,
+                                    int rc)
+{
+    STATE_AO_GC(dsps->ao);
+    libxl__domain_soft_reset_state *srs = CONTAINER_OF(dsps, *srs, dss.dsps);
+    libxl__app_domain_create_state *cdcs = &srs->cdcs;
+
     /*
      * Ask all backends to disconnect by removing the domain from
      * xenstore. On the creation path the domain will be introduced to
      * xenstore again with probably different store/console/...
      * channels.
      */
-    xs_release_domain(ctx->xsh, cdcs->dcs.domid_soft_reset);
+    xs_release_domain(CTX->xsh, cdcs->dcs.domid_soft_reset);
 
     srs->dds.ao = ao;
-    srs->dds.domid = domid_soft_reset;
+    srs->dds.domid = cdcs->dcs.domid_soft_reset;
     srs->dds.callback = domain_soft_reset_cb;
     srs->dds.soft_reset = true;
     libxl__domain_destroy(egc, &srs->dds);
-
-    return AO_INPROGRESS;
-
- out:
-    return AO_CREATE_FAIL(rc);
 }
 
 static void domain_create_cb(libxl__egc *egc,
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 1e904bae8a..51c432a00a 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -68,9 +68,10 @@ out:
 
 /*----- callbacks, called by xc_domain_save -----*/
 
-int libxl__domain_suspend_device_model(libxl__gc *gc,
+int libxl__domain_suspend_device_model(libxl__egc *egc,
                                        libxl__domain_suspend_state *dsps)
 {
+    STATE_AO_GC(dsps->ao);
     int ret = 0;
     uint32_t const domid = dsps->domid;
     const char *const filename = dsps->dm_savefile;
@@ -94,6 +95,7 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
         return ERROR_INVAL;
     }
 
+    dsps->callback_device_model_done(egc, dsps, ret);
     return ret;
 }
 
@@ -378,13 +380,15 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
     libxl__ev_time_deregister(gc, &dsps->guest_timeout);
 
     if (dsps->type == LIBXL_DOMAIN_TYPE_HVM) {
-        rc = libxl__domain_suspend_device_model(gc, dsps);
+        dsps->callback_device_model_done = domain_suspend_common_done;
+        rc = libxl__domain_suspend_device_model(egc, dsps);
         if (rc) {
             LOGD(ERROR, dsps->domid,
                  "libxl__domain_suspend_device_model failed ret=%d", rc);
             domain_suspend_common_done(egc, dsps, rc);
             return;
         }
+        return;
     }
     domain_suspend_common_done(egc, dsps, 0);
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0631ab1a03..7f0ad75160 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3407,6 +3407,8 @@ struct libxl__domain_suspend_state {
     libxl__ev_time guest_timeout;
 
     const char *dm_savefile;
+    void (*callback_device_model_done)(libxl__egc*,
+                                 struct libxl__domain_suspend_state*, int ok);
     void (*callback_common_done)(libxl__egc*,
                                  struct libxl__domain_suspend_state*, int ok);
 };
@@ -4024,7 +4026,7 @@ static inline bool libxl__save_helper_inuse(const libxl__save_helper_state *shs)
 }
 
 /* Each time the dm needs to be saved, we must call suspend and then save */
-_hidden int libxl__domain_suspend_device_model(libxl__gc *gc,
+_hidden int libxl__domain_suspend_device_model(libxl__egc *egc,
                                            libxl__domain_suspend_state *dsps);
 
 _hidden const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid);
-- 
Anthony PERARD


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

Re: [PATCH v5 14/15] libxl: Change libxl__domain_suspend_device_model() to be async.

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 14/15] libxl: Change libxl__domain_suspend_device_model() to be async."):
> This create an extra step for the two call sites of the function.
...
> -int libxl__domain_suspend_device_model(libxl__gc *gc,
> +int libxl__domain_suspend_device_model(libxl__egc *egc,
>                                         libxl__domain_suspend_state *dsps)
>  {
> +    STATE_AO_GC(dsps->ao);
>      int ret = 0;
>      uint32_t const domid = dsps->domid;
>      const char *const filename = dsps->dm_savefile;
> @@ -94,6 +95,7 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
>          return ERROR_INVAL;
>      }
>  
> +    dsps->callback_device_model_done(egc, dsps, ret);
>      return ret;
>  }

This function has broken error handling now, I'm afraid.  The actual
problem is that if it reaches the end there with rc!=0 (not currently
possible but might occur in the future), it will *both* call the
callback and return nonzero.

The root cause of this is the decision to make the function have two
ways of reporting errors: both via callback, and via return value.
Normally it is better when making a function like this async, to make
it return void.  (After making it handle all of its errors with `goto
out', first.)  Then all errors go via the callback.

That does introduce a new reentrancy hazard.  But in general we have
been able to deal with that by putting a doc comment
/* ... perhaps reentrantly */ or some such nearby.

That is sufficient when all the call sites the last thing in their
function.  Which I think is true here ?

If not, then you do need both error paths but then you mustn't call
_done reentrantly at all, as you do.  And that is not compatible with
easily converting this synchronous function into an async one, as you
are doing.

Thanks,
Ian.

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

[PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Anthony PERARD]

The re-implementation is done because we want to be able to send the
file description that QEMU can use to save its state. When QEMU is
restricted, it would not be able to write to a path.

This replace both libxl__qmp_stop() and libxl__qmp_save().

qmp_qemu_check_version() was only used by libxl__qmp_save(), so it is
replace by a version using libxl__ev_qmp instead.

Coding style fixed in libxl__domain_suspend_device_model() for the
return value.

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

Notes:
    v5:
        rename goto 'out' label to 'error', as it is use only for errors.
        re-add/keep the comment about the "live" parameter in dm_state_fd_ready
        use libxl__remove_file instead of plain unlink
    
    v4:
        This patch replace the patch "libxl_qmp: Have QEMU save its state to a file
        descriptor" from previous version of the serie.
        It uses libxl__ev_qmp instead.

 tools/libxl/libxl_dom_suspend.c |  16 ++--
 tools/libxl/libxl_internal.h    |   9 +-
 tools/libxl/libxl_qmp.c         | 153 +++++++++++++++++++++++++-------
 3 files changed, 130 insertions(+), 48 deletions(-)

diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 51c432a00a..0f4e1be115 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -34,6 +34,7 @@ int libxl__domain_suspend_init(libxl__egc *egc,
     libxl__ev_evtchn_init(&dsps->guest_evtchn);
     libxl__ev_xswatch_init(&dsps->guest_watch);
     libxl__ev_time_init(&dsps->guest_timeout);
+    libxl__ev_qmp_init(&dsps->qmp);
 
     if (type == LIBXL_DOMAIN_TYPE_INVALID) goto out;
     dsps->type = type;
@@ -72,7 +73,7 @@ int libxl__domain_suspend_device_model(libxl__egc *egc,
                                        libxl__domain_suspend_state *dsps)
 {
     STATE_AO_GC(dsps->ao);
-    int ret = 0;
+    int rc;
     uint32_t const domid = dsps->domid;
     const char *const filename = dsps->dm_savefile;
 
@@ -81,22 +82,18 @@ int libxl__domain_suspend_device_model(libxl__egc *egc,
         LOGD(DEBUG, domid, "Saving device model state to %s", filename);
         libxl__qemu_traditional_cmd(gc, domid, "save");
         libxl__wait_for_device_model_deprecated(gc, domid, "paused", NULL, NULL, NULL);
+        dsps->callback_device_model_done(egc, dsps, 0);
+        rc = 0;
         break;
     }
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-        if (libxl__qmp_stop(gc, domid))
-            return ERROR_FAIL;
-        /* Save DM state into filename */
-        ret = libxl__qmp_save(gc, domid, filename, dsps->live);
-        if (ret)
-            unlink(filename);
+        rc = libxl__qmp_suspend_save(gc, dsps);
         break;
     default:
         return ERROR_INVAL;
     }
 
-    dsps->callback_device_model_done(egc, dsps, ret);
-    return ret;
+    return rc;
 }
 
 static void domain_suspend_common_wait_guest(libxl__egc *egc,
@@ -402,6 +399,7 @@ static void domain_suspend_common_done(libxl__egc *egc,
     libxl__ev_evtchn_cancel(gc, &dsps->guest_evtchn);
     libxl__ev_xswatch_deregister(gc, &dsps->guest_watch);
     libxl__ev_time_deregister(gc, &dsps->guest_timeout);
+    libxl__ev_qmp_dispose(gc, &dsps->qmp);
     dsps->callback_common_done(egc, dsps, rc);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7f0ad75160..f2162f7e92 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1941,13 +1941,8 @@ _hidden int libxl__qmp_pci_del(libxl__gc *gc, int domid,
                                libxl_device_pci *pcidev);
 /* Resume hvm domain */
 _hidden int libxl__qmp_system_wakeup(libxl__gc *gc, int domid);
-/* Suspend QEMU. */
-_hidden int libxl__qmp_stop(libxl__gc *gc, int domid);
 /* Resume QEMU. */
 _hidden int libxl__qmp_resume(libxl__gc *gc, int domid);
-/* Save current QEMU state into fd. */
-_hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename,
-                            bool live);
 /* Load current QEMU state from file. */
 _hidden int libxl__qmp_restore(libxl__gc *gc, int domid, const char *filename);
 /* Set dirty bitmap logging status */
@@ -3405,6 +3400,7 @@ struct libxl__domain_suspend_state {
     libxl__xswait_state pvcontrol;
     libxl__ev_xswatch guest_watch;
     libxl__ev_time guest_timeout;
+    libxl__ev_qmp qmp;
 
     const char *dm_savefile;
     void (*callback_device_model_done)(libxl__egc*,
@@ -3416,6 +3412,9 @@ int libxl__domain_suspend_init(libxl__egc *egc,
                                libxl__domain_suspend_state *dsps,
                                libxl_domain_type type);
 
+_hidden int libxl__qmp_suspend_save(libxl__gc *gc,
+                                    libxl__domain_suspend_state *dsps);
+
 struct libxl__domain_save_state {
     /* set by caller of libxl__domain_save */
     libxl__ao *ao;
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 77380a869c..e2c3fca085 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -398,13 +398,14 @@ static int qmp_handle_response(libxl__gc *gc, libxl__qmp_handler *qmp,
     return 0;
 }
 
-static bool qmp_qemu_check_version(libxl__qmp_handler *qmp, int major,
-                                   int minor, int micro)
+static bool qmp_ev_qemu_check_version(libxl__ev_qmp *ev, int major,
+                                      int minor, int micro)
 {
-    return qmp->version.major > major ||
-        (qmp->version.major == major &&
-            (qmp->version.minor > minor ||
-             (qmp->version.minor == minor && qmp->version.micro >= micro)));
+    return ev->qemu_version.major > major ||
+        (ev->qemu_version.major == major &&
+            (ev->qemu_version.minor > minor ||
+             (ev->qemu_version.minor == minor &&
+              ev->qemu_version.micro >= micro)));
 }
 
 /*
@@ -1013,29 +1014,6 @@ int libxl__qmp_system_wakeup(libxl__gc *gc, int domid)
     return qmp_run_command(gc, domid, "system_wakeup", NULL, NULL, NULL);
 }
 
-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;
-
-    qmp = libxl__qmp_initialize(gc, domid);
-    if (!qmp)
-        return ERROR_FAIL;
-
-    qmp_parameters_add_string(gc, &args, "filename", (char *)filename);
-
-    /* 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);
-
-    rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
-                              NULL, NULL, qmp->timeout);
-    libxl__qmp_close(qmp);
-    return rc;
-}
-
 int libxl__qmp_restore(libxl__gc *gc, int domid, const char *state_file)
 {
     libxl__json_object *args = NULL;
@@ -1064,11 +1042,6 @@ static int qmp_change(libxl__gc *gc, libxl__qmp_handler *qmp,
     return rc;
 }
 
-int libxl__qmp_stop(libxl__gc *gc, int domid)
-{
-    return qmp_run_command(gc, domid, "stop", NULL, NULL, NULL);
-}
-
 int libxl__qmp_resume(libxl__gc *gc, int domid)
 {
     return qmp_run_command(gc, domid, "cont", NULL, NULL, NULL);
@@ -1308,6 +1281,118 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
     return ret;
 }
 
+
+/*
+ * Function using libxl__ev_qmp
+ */
+
+static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
+                       const libxl__json_object *response, int rc);
+static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
+                              const libxl__json_object *response, int rc);
+static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
+                           const libxl__json_object *response, int rc);
+int libxl__qmp_suspend_save(libxl__gc *gc, libxl__domain_suspend_state *dsps)
+{
+    libxl__ev_qmp *ev = &dsps->qmp;
+    uint32_t const domid = dsps->domid;
+
+    ev->domid = domid;
+    ev->callback = dm_stopped;
+    ev->cfd = NULL;
+
+    return libxl__ev_qmp_send(gc, ev, "stop", NULL);
+}
+
+static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
+                       const libxl__json_object *response, int rc)
+{
+    EGC_GC;
+    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
+    const char *const filename = dsps->dm_savefile;
+    uint32_t const domid = ev->domid;
+
+    if (rc)
+        goto error;
+
+    libxl__carefd_begin();
+    ev->cfd = libxl__carefd_opened(CTX,
+                                 open(filename, O_WRONLY | O_CREAT, 0600));
+    if (!ev->cfd) {
+        LOGED(ERROR, domid, "Failed to open file %s for QEMU", filename);
+        rc = ERROR_FAIL;
+        goto error;
+    }
+
+    ev->callback = dm_state_fd_ready;
+    rc = libxl__ev_qmp_send(gc, ev, "add-fd", NULL);
+    if (rc)
+        goto error;
+
+    return;
+error:
+    if (ev->cfd) {
+        libxl__carefd_close(ev->cfd);
+        unlink(filename);
+        ev->cfd = NULL;
+    }
+    dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
+                              const libxl__json_object *response, int rc)
+{
+    EGC_GC;
+    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
+    libxl__json_object *args = NULL;
+    const libxl__json_object *o;
+    int fdset;
+
+    libxl__carefd_close(ev->cfd);
+    ev->cfd = NULL;
+
+    if (rc)
+        goto error;
+
+    o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
+    if (!o) {
+        rc = ERROR_FAIL;
+        goto error;
+    }
+    fdset = libxl__json_object_get_integer(o);
+
+    ev->callback = dm_state_saved;
+
+    /* 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_ev_qemu_check_version(ev, 2, 11, 0))
+        qmp_parameters_add_bool(gc, &args, "live", dsps->live);
+    QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", fdset);
+    rc = libxl__ev_qmp_send(gc, ev, "xen-save-devices-state", args);
+    if (rc)
+        goto error;
+
+    return;
+error:
+    if (rc)
+        libxl__remove_file(gc, dsps->dm_savefile);
+    dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
+                           const libxl__json_object *response, int rc)
+{
+    EGC_GC;
+    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
+
+    if (rc)
+        unlink(dsps->dm_savefile);
+
+    libxl__ev_qmp_dispose(gc, ev);
+    dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+
 /* ------------ Implementation of libxl__ev_qmp ---------------- */
 
 /* hard coded message ID used for capability negotiation */
-- 
Anthony PERARD


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

Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Ian Jackson]

Anthony PERARD writes ("[PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp"):
> The re-implementation is done because we want to be able to send the
> file description that QEMU can use to save its state. When QEMU is
> restricted, it would not be able to write to a path.

Thanks, this basically LGTM.  I have only very minor comments.

> -static bool qmp_qemu_check_version(libxl__qmp_handler *qmp, int major,
> -                                   int minor, int micro)
> +static bool qmp_ev_qemu_check_version(libxl__ev_qmp *ev, int major,
> +                                      int minor, int micro)
>  {
> -    return qmp->version.major > major ||
> -        (qmp->version.major == major &&
> -            (qmp->version.minor > minor ||
> -             (qmp->version.minor == minor && qmp->version.micro >= micro)));
> +    return ev->qemu_version.major > major ||
> +        (ev->qemu_version.major == major &&
> +            (ev->qemu_version.minor > minor ||
> +             (ev->qemu_version.minor == minor &&
> +              ev->qemu_version.micro >= micro)));

Not your fault, but:

Ow my eyes.  Does this not make you dizzy ?  How about a pre-patch
which replaces this with

#define CHECK(field) do{ \
   if (qmp->version.field > (field)) return +1; \
   if (qmp->version.field < (field)) return -1; \
  }while(0)
   CHECK(major);
   CHECK(minor);
   CHECK(micro);
   return 0;
#undef CHECK

?

Then your actual change here is

 -   if (qmp->version.field > (field)) return +1; \
 -   if (qmp->version.field < (field)) return -1; \
 +   if (ev->qemu_version.field > (field)) return +1; \
 +   if (ev->qemu_version.field < (field)) return -1; \

Or some such.

Up to you, anyway.  While your diff there is hard to read it is at
least making things no worse and the compiler would have spotted
a missed search-and-replace of qmp->version.  You did use
search-and-replace, didn't you ? :-)

> +/*
> + * Function using libxl__ev_qmp
> + */
> +
> +static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
> +                       const libxl__json_object *response, int rc);
> +static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
> +                              const libxl__json_object *response, int rc);
> +static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
> +                           const libxl__json_object *response, int rc);
> +int libxl__qmp_suspend_save(libxl__gc *gc, libxl__domain_suspend_state *dsps)

Would you mind adding a blank line just before `int
libxl__qmp_suspend_save' ?

> +    if (rc)
> +        goto error;
> +
> +    libxl__carefd_begin();
> +    ev->cfd = libxl__carefd_opened(CTX,
> +                                 open(filename, O_WRONLY | O_CREAT, 0600));

Does this statefile fd really need to be a carefd ?  Is it a pipe or a
file ?  If it is a file, is it of nontrivial size ?

If it's a file of a few kb, which I think is the case, then the worst
result of (with very low probability) leaking this fd into another
process is simply that the file might be kept alive for a while after
it was deleted.

> +    /* live parameter was added to QEMU 2.11. It signal QEMU that the save

          The `live' parameter                   It signals

> +     * operation is for a live migration rather that for taking a snapshot. */

                                            rather than

> +    if (qmp_ev_qemu_check_version(ev, 2, 11, 0))

If you adopt my proposal above then maybe qmp_ev_qemu_check_version
should be renamed qmp_ev_qemu_compare_version and you would then write

       if (qmp_ev_qemu_compare_version(ev, 2, 11, 0) >= 0) {

> +    if (rc)
> +        goto error;
> +
> +    return;
> +error:

I think a blank line before `error:' would be better.

Thanks,
Ian.

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

Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Anthony PERARD]

On Tue, Oct 16, 2018 at 04:28:55PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp"):
> > +    if (rc)
> > +        goto error;
> > +
> > +    libxl__carefd_begin();
> > +    ev->cfd = libxl__carefd_opened(CTX,
> > +                                 open(filename, O_WRONLY | O_CREAT, 0600));
> 
> Does this statefile fd really need to be a carefd ?  Is it a pipe or a
> file ?  If it is a file, is it of nontrivial size ?

Yes, it needs to be caredfd, because that's what the libxl__ev_qmp API
wants. I don't know yet if it is a good idee to have the _ev_qmp API
only takes fd. Do you think it's fine to have libxl__ev_qmp API takes a
simple fd and let callers handle the fd the way they want?

(In previous version of the patch series, libxl__ev_qmp used to close
the carefd. That's not the case anymore, and that carefd is only read,
so I don't think it matter anymore which kind it is between int and
carefd.)

> If it's a file of a few kb, which I think is the case, then the worst
> result of (with very low probability) leaking this fd into another
> process is simply that the file might be kept alive for a while after
> it was deleted.


-- 
Anthony PERARD

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

Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp"):
> On Tue, Oct 16, 2018 at 04:28:55PM +0100, Ian Jackson wrote:
> > Does this statefile fd really need to be a carefd ?  Is it a pipe or a
> > file ?  If it is a file, is it of nontrivial size ?
> 
> Yes, it needs to be caredfd, because that's what the libxl__ev_qmp API
> wants. I don't know yet if it is a good idee to have the _ev_qmp API
> only takes fd. Do you think it's fine to have libxl__ev_qmp API takes a
> simple fd and let callers handle the fd the way they want?
> 
> (In previous version of the patch series, libxl__ev_qmp used to close
> the carefd. That's not the case anymore, and that carefd is only read,
> so I don't think it matter anymore which kind it is between int and
> carefd.)

Ah.  In OOP terms I think the API should take the narrowest class that
has all the required methods.  In this case that means that since you
don't need to close the fd you shouldn't prejudge whether it's a
carefd or not - so you should make it an int.

Regardless of whether it's an fd or a carefd, I think the qmp API
caller needs to keep the thing open until the qmp async operation is
done, right ?

Feel free to try to talk me out of this view.  I was asking the
question to provoke thought, not necessarily to push a particular
opinion.

Ian.

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

Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Anthony PERARD]

On Fri, Nov 09, 2018 at 05:11:05PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp"):
> > On Tue, Oct 16, 2018 at 04:28:55PM +0100, Ian Jackson wrote:
> > > Does this statefile fd really need to be a carefd ?  Is it a pipe or a
> > > file ?  If it is a file, is it of nontrivial size ?
> > 
> > Yes, it needs to be caredfd, because that's what the libxl__ev_qmp API
> > wants. I don't know yet if it is a good idee to have the _ev_qmp API
> > only takes fd. Do you think it's fine to have libxl__ev_qmp API takes a
> > simple fd and let callers handle the fd the way they want?
> > 
> > (In previous version of the patch series, libxl__ev_qmp used to close
> > the carefd. That's not the case anymore, and that carefd is only read,
> > so I don't think it matter anymore which kind it is between int and
> > carefd.)
> 
> Ah.  In OOP terms I think the API should take the narrowest class that
> has all the required methods.  In this case that means that since you
> don't need to close the fd you shouldn't prejudge whether it's a
> carefd or not - so you should make it an int.
> 
> Regardless of whether it's an fd or a carefd, I think the qmp API
> caller needs to keep the thing open until the qmp async operation is
> done, right ?

Yes, there is "they must all remain valid" in the struct libxl__ev_qmp
comments.

> Feel free to try to talk me out of this view.  I was asking the
> question to provoke thought, not necessarily to push a particular
> opinion.

I don't like much that the possibility exist to leak stuff, but on the
other end with the API change, I would need to store the carefd
somewhere else. I think in this case I downgrade to int as it is easier,
and as you said, it would be a small file with a low probability of been
leaked, so it doesn't matter too much.

Thanks,

-- 
Anthony PERARD

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