* [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*
@ 2018-11-23 13:53 Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK Anthony PERARD
` (15 more replies)
0 siblings, 16 replies; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-ev-qmp-v
Changes in v7:
plenty, with new patches
Changes in v6:
Implementation of libxl__ev_qmp_* functions have been squashed to a single
patch. And with that, a lot of changes in order to make it simpler to read
the implementation, have better error reporting and a few bug fix.
Checkout more detail changelog in the notes of each patch, as there is
many.
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.
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.
Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-ev-qmp-v
Cheers,
Anthony PERARD (14):
libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK
libxl_qmp: Separate QMP message generation from qmp_send_prepare
libxl_qmp: Change qmp_qemu_check_version to compare version
libxl: Add wrapper around libxl__json_object_to_json JSON
libxl: Design of an async API to issue QMP commands to QEMU
libxl_qmp: Implementation of libxl__ev_qmp_*
libxl_exec: Add libxl__spawn_initiate_failure
libxl: Add init/dispose of for libxl__domain_build_state
libxl_dm: Pre-open QMP socket for QEMU
libxl: Add dmss_init/dispose for libxl__dm_spawn_state
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 | 54 +-
tools/libxl/libxl_dm.c | 142 ++++-
tools/libxl/libxl_dom_suspend.c | 37 +-
tools/libxl/libxl_exec.c | 11 +-
tools/libxl/libxl_internal.h | 172 +++++-
tools/libxl/libxl_qmp.c | 992 ++++++++++++++++++++++++++++++--
tools/libxl/libxl_types.idl | 7 +
tools/libxl/libxl_utils.c | 25 +-
8 files changed, 1334 insertions(+), 106 deletions(-)
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 14:30 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare Anthony PERARD
` (14 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This patch change the behavior of libxl__sendmsg_fds to retry sendmsg on
EINTR error.
This patch also allow a caller of libxl__sendmsg_fds to deal with
EWOULDBLOCK. The function now requires to send only 1 byte of data so
that when dealing with non-blocking fds a EWOULDBLOCK error would mean
that the fds haven't been sent yet. Current caller already send only 1
byte.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
always assert datalen == 1, but only fail when sendmsg haven't send
everything (r != datalen)
check sendmsg return value on success as well (check for short write)
tools/libxl/libxl_internal.h | 5 ++++-
tools/libxl/libxl_utils.c | 25 ++++++++++++++++++++-----
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e498435e16..d96c5b7f89 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1864,7 +1864,10 @@ _hidden void libxl__qmp_cleanup(libxl__gc *gc, uint32_t domid);
_hidden int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
const libxl_domain_config *guest_config);
-/* on failure, logs */
+/* `datalen` should be 1 byte
+ * When dealing with a non-blocking fd, it returns
+ * ERROR_NOT_READY on EWOULDBLOCK
+ * logs on other failures. */
int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
const void *data, size_t datalen,
int nfds, const int fds[], const char *what);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 5854717b11..d53db8c37d 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1070,6 +1070,8 @@ int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
struct iovec iov;
int r;
+ assert(datalen == 1);
+
iov.iov_base = (void*)data;
iov.iov_len = datalen;
@@ -1088,11 +1090,24 @@ int libxl__sendmsg_fds(libxl__gc *gc, int carrier,
msg.msg_controllen = cmsg->cmsg_len;
- r = sendmsg(carrier, &msg, 0);
- if (r < 0) {
- LOGE(ERROR, "failed to send fd-carrying message (%s)", what);
- return ERROR_FAIL;
- }
+ while (1) {
+ r = sendmsg(carrier, &msg, 0);
+ if (r < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK) {
+ return ERROR_NOT_READY;
+ }
+ LOGE(ERROR, "failed to send fd-carrying message (%s)", what);
+ return ERROR_FAIL;
+ }
+ if (r != datalen) {
+ LOG(ERROR, "sendmsg have written %d instead of %ld",
+ r, datalen);
+ return ERROR_FAIL;
+ }
+ break;
+ };
return 0;
}
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 14:32 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 03/14] libxl_qmp: Change qmp_qemu_check_version to compare version Anthony PERARD
` (13 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
.. to be able to re-use qmp_prepare_cmd with libxl__ev_qmp.
This patch also add the QMP end of command '\r\n' into the generated
string as every caller will needs this.
There should be no functional change.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
got rid of len_r in qmp_prepare_cmd parameters
callers will need to call strlen.
The returned value (`ret`) is now allocated within `gc` (instead of
NOGC)
v6:
comment about ownership of buf
use lib__sprintf instead of two strncpy
v5:
rename qmp_prepare_qmp_cmd to qmp_prepare_cmd
fix coding style
tools/libxl/libxl_qmp.c | 49 ++++++++++++++++++++++++++---------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 6a5c997546..45a2cc423b 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -571,17 +571,16 @@ 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)
{
- const unsigned char *buf = NULL;
- char *ret = NULL;
- libxl_yajl_length len = 0;
+ yajl_gen hand = NULL;
+ /* memory for 'buf' is owned by 'hand' */
+ 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);
@@ -598,7 +597,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);
@@ -607,7 +606,27 @@ static char *qmp_send_prepare(libxl__gc *gc, libxl__qmp_handler *qmp,
s = yajl_gen_get_buf(hand, &buf, &len);
- if (s) {
+ if (s != yajl_gen_status_ok)
+ goto out;
+
+ ret = libxl__sprintf(gc, "%*.*s\r\n", (int)len, (int)len, buf);
+
+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)
+{
+ char *buf;
+ callback_id_pair *elm;
+
+ buf = qmp_prepare_cmd(gc, cmd, args, ++qmp->last_id_used);
+
+ if (!buf) {
LOGD(ERROR, qmp->domid, "Failed to generate a qmp command");
goto out;
}
@@ -623,13 +642,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,
@@ -650,9 +666,6 @@ 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:
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 03/14] libxl_qmp: Change qmp_qemu_check_version to compare version
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON Anthony PERARD
` (12 subsequent siblings)
15 siblings, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This patch makes the function simpler to read. It also add the ability
for a caller to tell if QEMU is newer or have the exact version.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Notes:
v6:
new patch
tools/libxl/libxl_qmp.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 45a2cc423b..73f2202b4f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -392,13 +392,27 @@ 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)
+/*
+ * return values:
+ * < 0 if qemu's version < asked version
+ * = 0 if qemu's version == asked version
+ * > 0 if qemu's version > asked version
+ */
+static int qmp_qemu_compare_version(libxl__qmp_handler *qmp, 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)));
+#define CHECK_VERSION(level) do { \
+ if (qmp->version.level > (level)) return +1; \
+ if (qmp->version.level < (level)) return -1; \
+} while (0)
+
+ CHECK_VERSION(major);
+ CHECK_VERSION(minor);
+ CHECK_VERSION(micro);
+
+#undef CHECK_VERSION
+
+ return 0;
}
/*
@@ -1012,7 +1026,7 @@ int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
/* 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))
+ if (qmp_qemu_compare_version(qmp, 2, 11, 0) >= 0)
qmp_parameters_add_bool(gc, &args, "live", live);
rc = qmp_synchronous_send(qmp, "xen-save-devices-state", args,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (2 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 03/14] libxl_qmp: Change qmp_qemu_check_version to compare version Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 14:33 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 05/14] libxl: Design of an async API to issue QMP commands to QEMU Anthony PERARD
` (11 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
That wrapper is going to be used to safely log a json_object, as
libxl__json_object_to_json return NULL on error. In the error case,
JSON() will return an invalid json string.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
new patch
There are no user yet because the first users is going to be in
"libxl_qmp: Implementation of libxl__ev_qmp_*" which is already a
huge patch, that 3 lines might get lost.
tools/libxl/libxl_internal.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d96c5b7f89..38c8c3a59d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2034,6 +2034,9 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
_hidden char *libxl__json_object_to_json(libxl__gc *gc,
const libxl__json_object *args);
+/* Always return a valid string, but invalid json on error. */
+#define JSON(o) \
+ (libxl__json_object_to_json(gc, (o)) ? : "<invalid-json-object>")
/* Based on /local/domain/$domid/dm-version xenstore key
* default is qemu xen traditional */
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 05/14] libxl: Design of an async API to issue QMP commands to QEMU
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (3 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_* Anthony PERARD
` (10 subsequent siblings)
15 siblings, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Notes:
v7:
acked, but with:
fd field renamed to payload_fd
libxl__ao ao field added
added to _send that callback isn't called synchronously.
v6:
use libxl_domid type for domid instead of plain uin32_t
avoid the work "chained", rewrite the paragraph about sending one
cmd after another
Rewrite the comment about the callback, and explain that on error,
the `ev` may be Idle or may still be Connected
Change the carefd to a simple int (field cfd -> fd)
v5:
some changes in the comment
tools/libxl/libxl_internal.h | 76 +++++++++++++++++++++++++++++++++++-
1 file changed, 74 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 38c8c3a59d..1c7a3b22f4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -186,6 +186,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,
@@ -349,6 +351,76 @@ 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 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.
+ *
+ * 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.
+ * callback isn't called synchronously.
+ *
+ * libxl__ev_qmp_dispose: Connected/Active/Idle -> Idle
+ *
+ * callback: When called: Active -> Connected (on error: Idle/Connected)
+ * When called, ev is Connected and can be reused or disposed of.
+ * On error, the callback is called with response == NULL and the
+ * error code in rc. The new state of ev depending on the value of rc:
+ * - rc == ERROR_QMP_*: This is an error associated with the cmd to
+ * run, ev is Connected.
+ * - otherwise: An other error happend, ev is now Idle.
+ * 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 */
+ libxl__ao *ao;
+ libxl_domid domid;
+ libxl__ev_qmp_callback *callback;
+ int payload_fd; /* set to send a fd with the command, -1 otherwise */
+};
+
/*
* evgen structures, which are the state we use for generating
@@ -1897,7 +1969,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;
@@ -1910,7 +1982,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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (4 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 05/14] libxl: Design of an async API to issue QMP commands to QEMU Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 15:36 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure Anthony PERARD
` (9 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This patch implement the API libxl__ev_qmp documented in the previous
patch, "libxl: Design of an async API to issue QMP commands to QEMU".
Since this API is to interact with QEMU via the QMP protocol, it also
implement a QMP client. The specification for the QEMU Machine Protocol
(QMP) can be found in the QEMU repository at:
https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/interop/qmp-spec.txt
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
- Make use of the new `ao` field filled by the caller
(no more free(tx_buf))
- Have reworked the state table to be more accurate, easier to read and
with better desc of the waiting_reply state with sub-states as well as
better description of the state of the ev_fd `efd`.
- There is state transition changes, now we have cap.neg ->
waiting_reply (before it was cap.neg -> connected -> waiting_reply),
that makes the internal connected state the same as the external
Connected state.
- there are now 3 different id variable instead of a single one, the
currently sent one `id`, the next one `next_id`, and the one
associated with the caller's command `msg_id`, `id` will always be the
for the next expected message from QEMU.
- qmp_ev_fd_callback now check for POLLERR, and read async error with
getsockopt(SO_ERROR).
- qmp_ev_callback_readable takes care once again to also parse messages.
But it now first attempt to read messages that would be in the rx_buf
before reading from the socket.
- Some functions state changes have been updated,
qmp_ev_ensure_reading_writing, qmp_ev_set_state,
qmp_ev_callback_writable, qmp_ev_callback_readable
- Have cleaned up qmp_ev_ensure_reading_writing
- Have cleaned up qmp_error_class_to_libxl_error_code, and log unknown
error classes.
- In qmp_ev_callback_readable, the size increase of the buffer have been
reworked to update both rx_buf_size and rx_buf at the same time, and
remove the use of max()
- Return value of qmp_prepare_cmd calles is checked and logged on error.
- comments in RHS of struct
- New qmp_ev_tx_buf_clear func
- Add a link to the QMP spec.
- update callers of qmp_prepare_cmd, which doesn't provide the string
lenght anymore also remove ev->msg_len
- rename internal fields qmp_cfd, qmp_efd, qmp_state to cfd, efd and
state.
- change qmp_ev_connect to only allow disconnect on entry
- squash qmp_ev_prepare_cmd into libxl__ev_qmp_send
- reduce max rx buffer size to 1M
query-vcpus with 71 cpus active yield 14484 bytes.
- use JSON to print libxl__json_object
v6.2:
Add definition of the internal broken state
updated comments about states
v6.1:
Adding some comment about possible internal state changes
v6:
This is a squash of 7 commits on the previous version:
- libxl_qmp: Connect to QMP socket
- libxl_qmp: Implement fd callback and read data
- libxl_qmp: Parse JSON input from QMP
- 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
General rework of the implementation.
Added more comment, with a description of allowed internal states.
Check for EINPROGRESS after connect().
Read until EWOULDBLOCK.
Handle EWOULDBLOCK on write and sendmsg.
Using memmem instead of strstr.
Using memmove instead of having an offset in rx_buf.
Rework buffer allocation
Don't feed \r into json parser anymore
Add a check for a maximum RX buffer size
Added more error messages
New error code ERROR_PROTOCOL_ERROR_QMP
Rewrite conversion of QMP ErrorClass to libxl_error code
Added helpers: qmp_ev_ensure_reading_writing, qmp_ev_set_state
Split some functions, squash others
Added ev->msg* to store generated user command as tx_buf is used during
connection (for qmp_capabilities)
Remove qmp_state_greeting
Added qmp_state_waiting_reply
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.
some cleanup
remove read loop that only handled EINTR, simply return
initialize len and s at declaration time
remove old comment
rename buf_fd to send_fd
Adding default:abort() in qmp_ev_handle_message.
v4:
remove use of a linked list of receive buffer, and use realloc instead.
simplification of the patch due to use of a single allocated space for the
receive buffer.
tools/libxl/libxl_internal.h | 34 ++
tools/libxl/libxl_qmp.c | 740 +++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 6 +
3 files changed, 780 insertions(+)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1c7a3b22f4..056de9de2f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -412,6 +412,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,
+ /* qmp_capabilities command sent, waiting for reply */
+ qmp_state_capability_negotiation,
+ /* sending user's cmd and waiting for reply */
+ qmp_state_waiting_reply,
+ /* 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 */
@@ -419,6 +432,27 @@ struct libxl__ev_qmp {
libxl_domid domid;
libxl__ev_qmp_callback *callback;
int payload_fd; /* set to send a fd with the command, -1 otherwise */
+
+ /*
+ * remaining fields are private to libxl_ev_qmp_*
+ */
+
+ libxl__carefd *cfd;
+ libxl__ev_fd efd;
+ libxl__qmp_state state;
+ int id;
+ int next_id; /* next id to use */
+ /* receive buffer */
+ char *rx_buf;
+ size_t rx_buf_size; /* current allocated size */
+ size_t rx_buf_used; /* actual data in the buffer */
+ /* sending buffer */
+ char *tx_buf;
+ size_t tx_buf_len; /* tx_buf size */
+ size_t tx_buf_off; /* already sent */
+ /* The message to send when ready */
+ char *msg;
+ int msg_id;
};
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 73f2202b4f..3c2545922e 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -75,11 +75,18 @@
# 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
*/
#define QMP_RECEIVE_BUFFER_SIZE 4096
+#define QMP_MAX_SIZE_RX_BUF MB(1)
#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
/*
@@ -1307,6 +1314,739 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
return ret;
}
+/* ------------ Implementation of libxl__ev_qmp ---------------- */
+
+/*
+ * Possible internal state compared to qmp_state:
+ *
+ * qmp_state External cfd efd id rx_buf* tx_buf* msg*
+ * disconnected Idle NULL Idle reset free free free
+ * connecting Active open IN reset used free set
+ * cap.neg Active open IN|OUT sent used cap_neg set
+ * cap.neg Active open IN sent used free set
+ * connected Connected open IN any used free free
+ * waiting_reply Active open IN|OUT sent used free set
+ * waiting_reply Active open IN|OUT sent used user's free
+ * waiting_reply Active open IN sent used free free
+ * broken[1] none[2] any Active any any any any
+ *
+ * [1] When an internal function return an error, it can leave ev_qmp in a
+ * `broken` state but only if the caller is another internal function.
+ * That `broken` needs to be cleaned up, e.i. transitionned to the
+ * `disconnected` state, before the control of ev_qmp is released outsides
+ * of ev_qmp implementation.
+ *
+ * [2] This internal state should not be visible externally, see [1].
+ *
+ * Possible buffers states:
+ * - receiving buffer:
+ * free used
+ * rx_buf NULL allocated
+ * rx_buf_size 0 allocation size of `rx_buf`
+ * rx_buf_used 0 <= rx_buf_size, actual data in the buffer
+ * - transmitting buffer:
+ * free used
+ * tx_buf NULL contains data
+ * tx_buf_len 0 size of data
+ * tx_buf_off 0 <= tx_buf_len, data already sent
+ * - queued user command:
+ * free set
+ * msg NULL contains data
+ * msg_id 0 id assoctiated with the command in `msg`
+ *
+ * - Allowed internal state transition:
+ * disconnected -> connecting
+ * connection -> capability_negotiation
+ * capability_negotiation/connected -> waiting_reply
+ * waiting_reply -> connected
+ * any -> broken
+ * broken -> disconnected
+ * any -> disconnected
+ *
+ * The QEMU Machine Protocol (QMP) specification can be found in the QEMU
+ * repository:
+ * https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/interop/qmp-spec.txt
+ */
+
+/* prototypes */
+
+static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
+ int fd, short events, short revents);
+static int qmp_ev_callback_writable(libxl__gc *gc,
+ libxl__ev_qmp *ev, int fd);
+static int qmp_ev_callback_readable(libxl__egc *egc,
+ libxl__ev_qmp *ev, int fd);
+static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
+ libxl__json_object **o_r);
+static int qmp_ev_handle_message(libxl__egc *egc,
+ libxl__ev_qmp *ev,
+ const libxl__json_object *resp);
+
+/* helpers */
+
+static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
+ /* Update the state of `efd` to match the permited state */
+{
+ short events = POLLIN;
+
+ if (ev->tx_buf)
+ events |= POLLOUT;
+ else if ((ev->state == qmp_state_waiting_reply) && ev->msg)
+ events |= POLLOUT;
+
+ libxl__ev_fd_modify(gc, &ev->efd, events);
+}
+
+static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
+ libxl__qmp_state new_state)
+ /* on entry: !broken and !disconnected */
+{
+ switch (new_state) {
+ case qmp_state_disconnected:
+ break;
+ case qmp_state_connecting:
+ assert(ev->state == qmp_state_disconnected);
+ break;
+ case qmp_state_capability_negotiation:
+ assert(ev->state == qmp_state_connecting);
+ break;
+ case qmp_state_waiting_reply:
+ assert(ev->state == qmp_state_capability_negotiation ||
+ ev->state == qmp_state_connected);
+ break;
+ case qmp_state_connected:
+ assert(ev->state == qmp_state_waiting_reply);
+ break;
+ }
+
+ ev->state = new_state;
+}
+
+static void qmp_ev_tx_buf_clear(libxl__ev_qmp *ev)
+{
+ ev->tx_buf = NULL;
+ ev->tx_buf_len = 0;
+ ev->tx_buf_off = 0;
+}
+
+static int qmp_error_class_to_libxl_error_code(libxl__gc *gc,
+ const char *eclass)
+{
+ const libxl_enum_string_table *t = libxl_error_string_table;
+ const char skip[] = "QMP_";
+ const size_t skipl = sizeof(skip) - 1;
+
+ /* compare "QMP_GENERIC_ERROR" from libxl_error to "GenericError"
+ * generated by the QMP server */
+
+ for (; t->s; t++) {
+ const char *s = eclass;
+ const char *se = t->s;
+ if (strncasecmp(t->s, skip, skipl))
+ continue;
+ se += skipl;
+ while (*s && *se) {
+ /* skip underscores */
+ if (*se == '_') {
+ se++;
+ continue;
+ }
+ if (tolower(*s) != tolower(*se))
+ break;
+ s++, se++;
+ }
+ if (!*s && !*se)
+ return t->v;
+ }
+
+ LOG(ERROR, "Unknown QMP error class '%s'", eclass);
+ return ERROR_UNKNOWN_QMP_ERROR;
+}
+
+/* Setup connection */
+
+static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
+ /* disconnected -> connecting but with `msg` free
+ * on error: broken */
+{
+ int fd;
+ int rc, r;
+ struct sockaddr_un un;
+ const char *qmp_socket_path;
+
+ assert(ev->state == qmp_state_disconnected);
+
+ qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
+
+ LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
+
+ libxl__carefd_begin();
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ ev->cfd = libxl__carefd_opened(CTX, fd);
+ if (!ev->cfd) {
+ LOGED(ERROR, ev->domid, "socket() failed");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ rc = libxl_fd_set_nonblock(CTX, libxl__carefd_fd(ev->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->cfd),
+ (struct sockaddr *) &un, sizeof(un));
+ if (r && errno != EINPROGRESS) {
+ 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->efd, qmp_ev_fd_callback,
+ libxl__carefd_fd(ev->cfd), POLLIN);
+ if (rc)
+ goto out;
+
+ qmp_ev_set_state(gc, ev, qmp_state_connecting);
+
+ return 0;
+
+out:
+ return rc;
+}
+
+/* QMP FD callbacks */
+
+static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
+ int fd, short events, short revents)
+ /* On entry, ev_fd is (of course) Active. The ev_qmp may be in any
+ * state where this is permitted. qmp_ev_fd_callback will do the work
+ * necessary to make progress, depending on the current state, and make
+ * the appropriate state transitions and callbacks. */
+{
+ EGC_GC;
+ int rc;
+ libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, efd);
+
+ if (revents & (POLLHUP|POLLERR)) {
+ int r;
+ int error_val = 0;
+ socklen_t opt_len = sizeof(error_val);
+
+ r = getsockopt(fd, SOL_SOCKET, SO_ERROR, &error_val, &opt_len);
+ if (r)
+ LOGED(ERROR, ev->domid, "getsockopt failed");
+ if (!r && error_val) {
+ errno = error_val;
+ LOGED(ERROR, ev->domid, "error on QMP socket");
+ } else {
+ LOGD(ERROR, ev->domid,
+ "received POLLHUP|POLLERR from QMP socket");
+ }
+ rc = ERROR_PROTOCOL_ERROR_QMP;
+ goto error;
+ }
+
+ 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 error;
+ }
+
+ if (revents & POLLOUT) {
+ rc = qmp_ev_callback_writable(gc, ev, fd);
+ if (rc)
+ goto error;
+ }
+
+ if (revents & POLLIN) {
+ rc = qmp_ev_callback_readable(egc, ev, fd);
+ if (rc < 0)
+ goto error;
+ if (rc == 1) {
+ /* user callback has been called */
+ return;
+ }
+ }
+
+ qmp_ev_ensure_reading_writing(gc, ev);
+
+ return;
+
+error:
+ assert(rc);
+
+ LOGD(ERROR, ev->domid,
+ "Error happened with the QMP connection to QEMU");
+
+ /* On error, deallocate all private ressources */
+ libxl__ev_qmp_dispose(gc, ev);
+
+ /* And tell libxl__ev_qmp user about the error */
+ ev->callback(egc, ev, NULL, rc); /* must be last */
+}
+
+static int qmp_ev_callback_writable(libxl__gc *gc,
+ libxl__ev_qmp *ev, int fd)
+ /* on entry: !disconnected
+ * on return, one of these state transition:
+ * waiting_reply (with msg set) -> waiting_reply (with msg free)
+ * tx_buf set -> same state or tx_buf free
+ * tx_buf free -> no state change
+ * on error: broken */
+{
+ int rc;
+ ssize_t r;
+
+ if (ev->state == qmp_state_waiting_reply) {
+ if (ev->msg) {
+ assert(!ev->tx_buf);
+ ev->tx_buf = ev->msg;
+ ev->tx_buf_len = strlen(ev->msg);
+ ev->tx_buf_off = 0;
+ ev->id = ev->msg_id;
+ ev->msg = NULL;
+ ev->msg_id = 0;
+ }
+ }
+
+ assert(ev->tx_buf);
+ if (!ev->tx_buf)
+ return 0;
+
+ LOG_QMP("sending: '%.*s'", (int)ev->tx_buf_len, ev->tx_buf);
+
+ /*
+ * We will send a file descriptor associated with a command on the
+ * first byte of this command.
+ */
+ if (ev->state == qmp_state_waiting_reply &&
+ ev->payload_fd >= 0 &&
+ ev->tx_buf_off == 0) {
+
+ rc = libxl__sendmsg_fds(gc, fd, ev->tx_buf, 1,
+ 1, &ev->payload_fd, "QMP socket");
+ /* Check for EWOULDBLOCK, and return to try again later */
+ if (rc == ERROR_NOT_READY)
+ return 0;
+ if (rc)
+ return rc;
+ ev->tx_buf_off++;
+ }
+
+ while (ev->tx_buf_off < ev->tx_buf_len) {
+ r = write(fd, ev->tx_buf + ev->tx_buf_off,
+ ev->tx_buf_len - ev->tx_buf_off);
+ if (r < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ break;
+ LOGED(ERROR, ev->domid, "failed to write to QMP socket");
+ return ERROR_FAIL;
+ }
+ ev->tx_buf_off += r;
+ }
+
+ if (ev->tx_buf_off == ev->tx_buf_len)
+ qmp_ev_tx_buf_clear(ev);
+
+ return 0;
+}
+
+static int qmp_ev_callback_readable(libxl__egc *egc,
+ libxl__ev_qmp *ev, int fd)
+ /*
+ * Return values:
+ * < 0 libxl error code
+ * 0 success
+ * 1 success, but a user callback has been called,
+ * `ev` should not be used anymore.
+ *
+ * This function will update the rx buffer and possibly update
+ * ev->state:
+ * connecting -> capability_negotiation
+ * capability_negotiation -> waiting_reply
+ * waiting_reply -> connected
+ * on error: broken
+ */
+{
+ EGC_GC;
+ int rc;
+ ssize_t r;
+
+ while (1) {
+ while (1) {
+ libxl__json_object *o = NULL;
+
+ /* parse rx buffer to find one json object */
+ rc = qmp_ev_get_next_msg(egc, ev, &o);
+ if (rc == ERROR_NOTFOUND)
+ break;
+ else if (rc)
+ return rc;
+
+ /* Must be last and return when the user callback is called */
+ rc = qmp_ev_handle_message(egc, ev, o);
+ if (rc)
+ /* returns both rc values -ERROR_* and 1 */
+ return rc;
+ }
+
+ /* Check if the buffer still have space, or increase size */
+ if (ev->rx_buf_size - ev->rx_buf_used < QMP_RECEIVE_BUFFER_SIZE) {
+ size_t newsize = ev->rx_buf_size * 2 + QMP_RECEIVE_BUFFER_SIZE;
+
+ if (newsize > QMP_MAX_SIZE_RX_BUF) {
+ LOGD(ERROR, ev->domid,
+ "QMP receive buffer is too big (%ld > %lld)",
+ newsize, QMP_MAX_SIZE_RX_BUF);
+ return ERROR_BUFFERFULL;
+ }
+ ev->rx_buf_size = newsize;
+ ev->rx_buf = libxl__realloc(NOGC, ev->rx_buf, ev->rx_buf_size);
+ }
+
+ r = read(fd, ev->rx_buf + ev->rx_buf_used,
+ ev->rx_buf_size - ev->rx_buf_used);
+ if (r < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ break;
+ LOGED(ERROR, ev->domid, "error reading QMP socket");
+ return ERROR_FAIL;
+ }
+
+ if (r == 0) {
+ LOGD(ERROR, ev->domid, "Unexpected EOF on QMP socket");
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+
+ LOG_QMP("received %ldB: '%.*s'", r,
+ (int)r, ev->rx_buf + ev->rx_buf_used);
+
+ ev->rx_buf_used += r;
+ assert(ev->rx_buf_used <= ev->rx_buf_size);
+ }
+
+ return 0;
+}
+
+/* Handle messages received from QMP server */
+
+static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
+ libxl__json_object **o_r)
+ /* Find a JSON object and store it in o_r.
+ * return ERROR_NOTFOUND if no object is found.
+ * `o_r` is allocated within `egc`.
+ *
+ * !disconnected -> same state (with rx buffer updated)
+ */
+{
+ EGC_GC;
+ size_t len;
+ char *end = NULL;
+ const char eom[] = "\r\n";
+ const size_t eoml = sizeof(eom) - 1;
+ libxl__json_object *o = NULL;
+
+ if (!ev->rx_buf_used)
+ return ERROR_NOTFOUND;
+
+ /* Search for the end of a QMP message: "\r\n" */
+ end = memmem(ev->rx_buf, ev->rx_buf_used, eom, eoml);
+ if (!end)
+ return ERROR_NOTFOUND;
+ len = (end - ev->rx_buf) + eoml;
+
+ LOG_QMP("parsing %luB: '%.*s'", len, (int)len, ev->rx_buf);
+
+ /* Replace \r by \0 so that libxl__json_parse can use strlen */
+ ev->rx_buf[len - eoml] = '\0';
+ o = libxl__json_parse(gc, ev->rx_buf);
+
+ if (!o) {
+ LOGD(ERROR, ev->domid, "Parse error");
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+
+ ev->rx_buf_used -= len;
+ memmove(ev->rx_buf, ev->rx_buf + len, ev->rx_buf_used);
+
+ LOG_QMP("JSON object received: %s", JSON(o));
+
+ *o_r = o;
+
+ return 0;
+}
+
+static int qmp_ev_parse_error_messages(libxl__egc *egc,
+ libxl__ev_qmp *ev,
+ const libxl__json_object *resp)
+ /* no state change */
+{
+ EGC_GC;
+ int rc;
+ const char *s;
+ const libxl__json_object *o;
+ const libxl__json_object *err;
+
+ /*
+ * { "error": { "class": string, "desc": string } }
+ */
+
+ err = libxl__json_map_get("error", resp, JSON_MAP);
+
+ o = libxl__json_map_get("class", err, JSON_STRING);
+ if (!o) {
+ LOGD(ERROR, ev->domid,
+ "Protocol error: missing 'class' member in error message");
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+ s = libxl__json_object_get_string(o);
+ if (s)
+ rc = qmp_error_class_to_libxl_error_code(gc, s);
+ else
+ rc = ERROR_PROTOCOL_ERROR_QMP;
+
+ o = libxl__json_map_get("desc", err, JSON_STRING);
+ if (!o) {
+ LOGD(ERROR, ev->domid,
+ "Protocol error: missing 'desc' member in error message");
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+ s = libxl__json_object_get_string(o);
+ if (s)
+ LOGD(ERROR, ev->domid, "%s", s);
+ else
+ LOGD(ERROR, ev->domid, "Received unexpected error: %s",
+ JSON(resp));
+ return rc;
+}
+
+static int qmp_ev_handle_message(libxl__egc *egc,
+ libxl__ev_qmp *ev,
+ const libxl__json_object *resp)
+ /*
+ * This function will handle every messages sent by the QMP server.
+ * Return values:
+ * < 0 libxl error code
+ * 0 success
+ * 1 success, but a user callback has been called,
+ * `ev` should not be used anymore.
+ *
+ * Possible state changes:
+ * connecting -> capability_negotiation
+ * capability_negotiation -> waiting_reply
+ * waiting_reply -> waiting_reply/connected
+ *
+ * on error: broken
+ */
+{
+ EGC_GC;
+ int id;
+ char *buf;
+ int rc = 0;
+ const libxl__json_object *o;
+ const libxl__json_object *response;
+ libxl__qmp_message_type type = qmp_response_type(resp);
+
+ switch (type) {
+ case LIBXL__QMP_MESSAGE_TYPE_QMP:
+ /* greeting message */
+
+ if (ev->state != qmp_state_connecting) {
+ LOGD(ERROR, ev->domid,
+ "Unexpected greeting message received");
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+
+ /* Prepare next message to send */
+ assert(!ev->tx_buf);
+ ev->id = ev->next_id++;
+ buf = qmp_prepare_cmd(libxl__ao_inprogress_gc(ev->ao),
+ "qmp_capabilities", NULL, ev->id);
+ if (!buf) {
+ LOGD(ERROR, ev->domid,
+ "Failed to generate qmp_capabilities command");
+ return ERROR_FAIL;
+ }
+ ev->tx_buf = buf;
+ ev->tx_buf_len = strlen(buf);
+ ev->tx_buf_off = 0;
+ qmp_ev_set_state(gc, ev, qmp_state_capability_negotiation);
+
+ return 0;
+
+ case LIBXL__QMP_MESSAGE_TYPE_RETURN:
+ case LIBXL__QMP_MESSAGE_TYPE_ERROR:
+ /*
+ * Reply to a command (success/error) or server error
+ *
+ * In this cases, we are parsing two possibles responses:
+ * - success:
+ * { "return": json-value, "id": int }
+ * - error:
+ * { "error": { "class": string, "desc": string }, "id": int }
+ */
+
+ o = libxl__json_map_get("id", resp, JSON_INTEGER);
+ if (!o) {
+ /*
+ * If "id" isn't present, an error occur on the server before
+ * it has read the "id" provided by libxl.
+ *
+ * We deliberately squash all errors into
+ * ERROR_PROTOCOL_ERROR_QMP as qmp_ev_parse_error_messages may
+ * also return ERROR_QMP_* but those are reserved for errors
+ * return by the caller's command.
+ */
+ qmp_ev_parse_error_messages(egc, ev, resp);
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+
+ id = libxl__json_object_get_integer(o);
+
+ if (id != ev->id) {
+ LOGD(ERROR, ev->domid,
+ "Message from QEMU with unexpected id %d: %s",
+ id, JSON(resp));
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+
+ switch (ev->state) {
+ case qmp_state_capability_negotiation:
+ if (type != LIBXL__QMP_MESSAGE_TYPE_RETURN) {
+ LOGD(ERROR, ev->domid,
+ "Error during capability negotiation: %s",
+ JSON(resp));
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+ qmp_ev_set_state(gc, ev, qmp_state_waiting_reply);
+ return 0;
+ case qmp_state_waiting_reply:
+ if (type == LIBXL__QMP_MESSAGE_TYPE_RETURN) {
+ response = libxl__json_map_get("return", resp, JSON_ANY);
+ rc = 0;
+ } else {
+ /* error message */
+ response = NULL;
+ rc = qmp_ev_parse_error_messages(egc, ev, resp);
+ }
+ qmp_ev_set_state(gc, ev, qmp_state_connected);
+ ev->callback(egc, ev, response, rc); /* must be last */
+ return 1;
+ default:
+ LOGD(ERROR, ev->domid, "Unexpected message: %s", JSON(resp));
+ return ERROR_PROTOCOL_ERROR_QMP;
+ }
+ return 0;
+
+ case LIBXL__QMP_MESSAGE_TYPE_EVENT:
+ /* Events are ignored */
+ return 0;
+
+ case LIBXL__QMP_MESSAGE_TYPE_INVALID:
+ LOGD(ERROR, ev->domid, "Unexpected message received: %s",
+ JSON(resp));
+ return ERROR_PROTOCOL_ERROR_QMP;
+
+ default:
+ abort();
+ }
+
+ return 0;
+}
+
+/*
+ * libxl__ev_qmp_*
+ */
+
+void libxl__ev_qmp_init(libxl__ev_qmp *ev)
+ /* disconnected -> disconnected */
+{
+ /* Start with an message ID that is obviously generated by libxl
+ * "xlq\0" */
+ ev->next_id = 0x786c7100;
+
+ ev->cfd = NULL;
+ libxl__ev_fd_init(&ev->efd);
+ ev->state = qmp_state_disconnected;
+ ev->id = 0;
+
+ ev->rx_buf = NULL;
+ ev->rx_buf_size = ev->rx_buf_used = 0;
+ qmp_ev_tx_buf_clear(ev);
+
+ ev->msg = NULL;
+ ev->msg_id = 0;
+}
+
+int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
+ const char *cmd, libxl__json_object *args)
+ /* disconnected -> connecting
+ * connected -> waiting_reply (with msg set)
+ * on error: disconnected */
+{
+ int rc;
+
+ LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
+
+ assert(ev->state == qmp_state_disconnected ||
+ ev->state == qmp_state_connected);
+ assert(cmd);
+
+ /* Connect to QEMU if not already connected */
+ if (ev->state == qmp_state_disconnected) {
+ rc = qmp_ev_connect(gc, ev);
+ if (rc)
+ goto error;
+ }
+
+ /* Prepare user command */
+ ev->msg_id = ev->next_id++;
+ ev->msg = qmp_prepare_cmd(libxl__ao_inprogress_gc(ev->ao),
+ cmd, args, ev->msg_id);
+ if (!ev->msg) {
+ LOGD(ERROR, ev->domid, "Failed to generate caller's command %s",
+ cmd);
+ rc = ERROR_FAIL;
+ goto error;
+ }
+ if (ev->state == qmp_state_connected) {
+ qmp_ev_set_state(gc, ev, qmp_state_waiting_reply);
+ }
+
+ qmp_ev_ensure_reading_writing(gc, ev);
+
+ return 0;
+
+error:
+ libxl__ev_qmp_dispose(gc, ev);
+ return rc;
+}
+
+void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
+ /* * -> disconnected */
+{
+ LOGD(DEBUG, ev->domid, " ev %p", ev);
+
+ free(ev->rx_buf);
+
+ libxl__ev_fd_deregister(gc, &ev->efd);
+ libxl__carefd_close(ev->cfd);
+
+ libxl__ev_qmp_init(ev);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 51cf06a3a2..90340ab1bb 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -69,6 +69,12 @@ 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, "PROTOCOL_ERROR_QMP"),
+ (-27, "UNKNOWN_QMP_ERROR"),
+ (-28, "QMP_GENERIC_ERROR"), # unspecified qmp error
+ (-29, "QMP_COMMAND_NOT_FOUND"), # the requested command has not been found
+ (-30, "QMP_DEVICE_NOT_ACTIVE"), # a device has failed to be become active
+ (-31, "QMP_DEVICE_NOT_FOUND"), # the requested device has not been found
], value_namespace = "")
libxl_domain_type = Enumeration("domain_type", [
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (5 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_* Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 15:57 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state Anthony PERARD
` (8 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
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>
---
Notes:
v7:
call spawn_fail from libxl__spawn_initiate_failure.
modify spawn_fail to set ss->rc only once.
Fix description to tell caller of libxl__spawn_initiate_failure that
they have to log.
use an `egc` instead of `gc` with libxl__spawn_initiate_failure.
v6:
long line fix
typos fixed
fix leak of internal state into external doc
if the function is call multiple times, set rc only the first time.
v5:
fix typos
tools/libxl/libxl_exec.c | 11 ++++++++++-
tools/libxl/libxl_internal.h | 23 ++++++++++++++++++++++-
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_exec.c b/tools/libxl/libxl_exec.c
index 02e6c917f0..47c9c8f1ba 100644
--- a/tools/libxl/libxl_exec.c
+++ b/tools/libxl/libxl_exec.c
@@ -373,13 +373,22 @@ void libxl__spawn_initiate_detach(libxl__gc *gc, libxl__spawn_state *ss)
spawn_detach(gc, ss);
}
+void libxl__spawn_initiate_failure(libxl__egc *egc, libxl__spawn_state *ss,
+ int rc)
+/* The spawn state must be Attached on entry and will be Attached Failed
+ * on return. */
+{
+ spawn_fail(egc, ss, rc);
+}
+
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. */
{
EGC_GC;
assert(rc);
- ss->rc = rc;
+ if (!ss->rc)
+ ss->rc = rc;
spawn_detach(gc, ss);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 056de9de2f..b486ff9d50 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1553,7 +1553,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 set up.
*
* The user (in the parent) will be called back (confirm_cb) every
* time that xenstore path is modified.
@@ -1609,6 +1610,26 @@ _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, a failure will be reported.
+ *
+ * This is not synchronous: there will be a further callback when
+ * the detach is complete.
+ *
+ * Caller must have logged a failure reason.
+ *
+ * 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 first rc value from any source will take precedence.
+ */
+_hidden void libxl__spawn_initiate_failure(libxl__egc *egc,
+ 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (6 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:00 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU Anthony PERARD
` (7 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
These two new functions libxl__domain_build_state_{init,dispose} should
be called every time a new libxl__domain_build_state comes to existance.
There seems to be two of them, one with the domain creation machinery,
and one in the stub_dm_spawn.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
new patch
tools/libxl/libxl_create.c | 14 ++++++++++++--
tools/libxl/libxl_dm.c | 3 +++
tools/libxl/libxl_internal.h | 3 +++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fa573344bc..d1c0f009ea 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -761,6 +761,16 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
libxl_device_model_version_to_string(b_info->device_model_version));
}
+void libxl__domain_build_state_init(libxl__domain_build_state *state)
+{
+}
+
+void libxl__domain_build_state_dispose(libxl__domain_build_state *state)
+{
+ libxl__file_reference_unmap(&state->pv_kernel);
+ libxl__file_reference_unmap(&state->pv_ramdisk);
+}
+
/*----- main domain creation -----*/
/* We have a linear control flow; only one event callback is
@@ -820,6 +830,7 @@ static void initiate_domain_create(libxl__egc *egc,
const int restore_fd = dcs->restore_fd;
domid = dcs->domid_soft_reset;
+ libxl__domain_build_state_init(&dcs->build_state);
if (d_config->c_info.ssid_label) {
char *s = d_config->c_info.ssid_label;
@@ -1592,8 +1603,7 @@ static void domcreate_complete(libxl__egc *egc,
libxl_domain_config *const d_config = dcs->guest_config;
libxl_domain_config *d_config_saved = &dcs->guest_config_saved;
- libxl__file_reference_unmap(&dcs->build_state.pv_kernel);
- libxl__file_reference_unmap(&dcs->build_state.pv_ramdisk);
+ libxl__domain_build_state_dispose(&dcs->build_state);
if (!rc && d_config->b_info.exec_ssidref)
rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 9c47060473..cd53f9ae62 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1919,6 +1919,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
libxl__domain_build_state *const d_state = sdss->dm.build_state;
libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
+ libxl__domain_build_state_init(stubdom_state);
+
if (guest_config->b_info.device_model_version !=
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
ret = ERROR_INVAL;
@@ -2247,6 +2249,7 @@ static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
if (strcmp(p, "running"))
return;
out:
+ libxl__domain_build_state_dispose(&sdss->dm_state);
libxl__xswait_stop(gc, xswait);
sdss->callback(egc, &sdss->dm, rc);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b486ff9d50..77340677b8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1251,6 +1251,9 @@ typedef struct {
uint32_t clock_frequency;
} libxl__domain_build_state;
+_hidden void libxl__domain_build_state_init(libxl__domain_build_state *s);
+_hidden void libxl__domain_build_state_dispose(libxl__domain_build_state *s);
+
_hidden int libxl__build_pre(libxl__gc *gc, uint32_t domid,
libxl_domain_config * const d_config,
libxl__domain_build_state *state);
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (7 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:01 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 10/14] libxl: Add dmss_init/dispose for libxl__dm_spawn_state Anthony PERARD
` (6 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This patch moves the creation of the QMP unix socket from QEMU to libxl.
But libxl doesn't rely on this yet.
When starting QEMU with dm_restrict=1, pre-open the QMP socket before
exec QEMU. That socket will be useful to find out if QEMU is ready, and
pre-opening it means that libxl can connect to it without waiting for
QEMU to create it.
The pre-opening is conditional, 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 is available in QEMU 3.0.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
move domain_build_state init/dispose to a different patch
and close dm_monitor_fd in that new dispose function
assert that dm_monitor_fd should be -1 when using old qemu
v6:
move dm_monitor_fd into libxl_domain_build_info (or d_state)
-> move the creation of the socket into libxl__spawn_local_dm
instead of libxl__build_device_model_args
Use libxl_domid type instead of int for libxl__pre_open_qmp_socket()
Check function calls (bind and listen) return value in a separate statement.
typo and other coding style issue fixes
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_create.c | 5 +++
tools/libxl/libxl_dm.c | 70 +++++++++++++++++++++++++++++++++---
tools/libxl/libxl_internal.h | 1 +
3 files changed, 72 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d1c0f009ea..d5c13c0f35 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -763,12 +763,17 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
void libxl__domain_build_state_init(libxl__domain_build_state *state)
{
+ state->dm_monitor_fd = -1;
}
void libxl__domain_build_state_dispose(libxl__domain_build_state *state)
{
libxl__file_reference_unmap(&state->pv_kernel);
libxl__file_reference_unmap(&state->pv_ramdisk);
+ if (state->dm_monitor_fd >= 0) {
+ close(state->dm_monitor_fd);
+ state->dm_monitor_fd = -1;
+ }
}
/*----- main domain creation -----*/
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index cd53f9ae62..e511f9b527 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -432,6 +432,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
dm_args = flexarray_make(gc, 16, 1);
dm_envs = flexarray_make(gc, 16, 1);
+ assert(state->dm_monitor_fd == -1);
+
libxl__set_qemu_env_for_xsa_180(gc, dm_envs);
flexarray_vappend(dm_args, dm,
@@ -910,6 +912,51 @@ 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, libxl_domid domid,
+ int *fd_r)
+{
+ int rc, r;
+ 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;
+
+ r = bind(fd, (struct sockaddr *) &un, sizeof(un));
+ if (r < 0) {
+ LOGED(ERROR, domid, "bind('%s') failed", path);
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ r = listen(fd, 1);
+ if (r < 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,
@@ -944,10 +991,16 @@ 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 (state->dm_monitor_fd >= 0) {
+ flexarray_append(dm_args,
+ GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
+ state->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");
@@ -2306,6 +2359,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
rc = ERROR_FAIL;
goto out;
}
+ if (b_info->device_model_version
+ == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
+ libxl_defbool_val(b_info->dm_restrict)) {
+ /* If we have to use dm_restrict, QEMU needs to be new enough
+ * and will have the new interface where we can pre-open the
+ * QMP socket. */
+ rc = libxl__pre_open_qmp_socket(gc, domid, &state->dm_monitor_fd);
+ if (rc) goto out;
+ }
rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
&args, &envs, state,
&dm_state_fd);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 77340677b8..08442872fe 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1234,6 +1234,7 @@ typedef struct {
char *console_tty;
char *saved_state;
+ int dm_monitor_fd;
libxl__file_reference pv_kernel;
libxl__file_reference pv_ramdisk;
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 10/14] libxl: Add dmss_init/dispose for libxl__dm_spawn_state
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (8 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 11/14] libxl: QEMU startup sync based on QMP Anthony PERARD
` (5 subsequent siblings)
15 siblings, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
These two functions, dmss_init and dmss_dispose, need to be called to
initialise the private parts of a libxl__dm_spawn_state (dmss) as well
as dispose of them before giving back control to a caller.
There are 3 functions that can start using a dmss, the classic
libxl__spawn_local_dm, the one for stubdom libxl__spawn_stub_dm and
libxl__spawn_qdisk_backend. But there are only 2 exit path as
libxl__spawn_qdisk_backend is using libxl__spawn_local_dm functions.
These two new functions are empty but will be used shortly.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
new patch
tools/libxl/libxl_dm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e511f9b527..371b742b7f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1935,6 +1935,14 @@ retry_transaction:
return 0;
}
+static void dmss_init(libxl__dm_spawn_state *dmss)
+{
+}
+
+static void dmss_dispose(libxl__gc *gc, libxl__dm_spawn_state *dmss)
+{
+}
+
static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
libxl__dm_spawn_state *stubdom_dmss,
int rc);
@@ -1973,6 +1981,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
libxl__domain_build_state_init(stubdom_state);
+ dmss_init(&sdss->dm);
if (guest_config->b_info.device_model_version !=
LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
@@ -2304,6 +2313,7 @@ static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
out:
libxl__domain_build_state_dispose(&sdss->dm_state);
libxl__xswait_stop(gc, xswait);
+ dmss_dispose(gc, &sdss->dm);
sdss->callback(egc, &sdss->dm, rc);
}
@@ -2345,6 +2355,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
const char *dm;
int dm_state_fd = -1;
+ dmss_init(dmss);
+
if (libxl_defbool_val(b_info->device_model_stubdomain)) {
abort();
}
@@ -2550,6 +2562,7 @@ static void device_model_spawn_outcome(libxl__egc *egc,
}
out:
+ dmss_dispose(gc, dmss);
dmss->callback(egc, dmss, rc);
}
@@ -2562,6 +2575,8 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
int logfile_w, null = -1, rc;
uint32_t domid = dmss->guest_domid;
+ dmss_init(dmss);
+
/* Always use qemu-xen as device model */
dm = qemu_xen_path(gc);
@@ -2626,6 +2641,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
rc = 0;
out:
+ dmss_dispose(gc, dmss);
if (logfile_w >= 0) close(logfile_w);
if (null >= 0) close(null);
/* callback on error only, success goes via dmss->spawn.*_cb */
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 11/14] libxl: QEMU startup sync based on QMP
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (9 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 10/14] libxl: Add dmss_init/dispose for libxl__dm_spawn_state Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:03 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp Anthony PERARD
` (4 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This is only activated when dm_restrict=1, as explained in a 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:
v7:
fixed _dispose call in device_model_spawn_outcome
move qmp field in libxl__dm_spawn_state into the private section
added description about the new QEMU_API error value
v6:
invent ERROR_QEMU_API
return better rc: ERROR_QEMU_API or ERROR_NOT_READY
enhance log messages (debug and error)
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 | 53 ++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 1 +
tools/libxl/libxl_types.idl | 1 +
3 files changed, 55 insertions(+)
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 371b742b7f..80f72f36c7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1937,10 +1937,12 @@ retry_transaction:
static void dmss_init(libxl__dm_spawn_state *dmss)
{
+ libxl__ev_qmp_init(&dmss->qmp);
}
static void dmss_dispose(libxl__gc *gc, libxl__dm_spawn_state *dmss)
{
+ libxl__ev_qmp_dispose(gc, &dmss->qmp);
}
static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
@@ -2325,6 +2327,9 @@ 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,
@@ -2465,6 +2470,17 @@ retry_transaction:
spawn->failure_cb = device_model_startup_failed;
spawn->detached_cb = device_model_detached;
+ if (state->dm_monitor_fd >= 0) {
+ /* There is a valid QMP socket available now,
+ * use it to find out when QEMU is ready */
+ dmss->qmp.ao = ao;
+ dmss->qmp.callback = device_model_qmp_cb;
+ dmss->qmp.domid = domid;
+ dmss->qmp.payload_fd = -1;
+ 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;
@@ -2538,6 +2554,43 @@ 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(ERROR, ev->domid,
+ "Missing 'status' in response to 'query-status'");
+ LOGD(DEBUG, ev->domid, ".. instead, got: %s", JSON(response));
+ rc = ERROR_QEMU_API;
+ goto failed;
+ }
+ status = libxl__json_object_get_string(o);
+ if (strcmp(status, "running")) {
+ LOGD(ERROR, ev->domid, "Unexpected QEMU status: %s", status);
+ rc = ERROR_NOT_READY;
+ 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(egc, &dmss->spawn, rc);
+}
+
static void device_model_spawn_outcome(libxl__egc *egc,
libxl__dm_spawn_state *dmss,
int rc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 08442872fe..7c2e0edff8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3902,6 +3902,7 @@ typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
struct libxl__dm_spawn_state {
/* mixed - spawn.ao must be initialised by user; rest is private: */
libxl__spawn_state spawn;
+ libxl__ev_qmp qmp;
/* filled in by user, must remain valid: */
uint32_t guest_domid; /* domain being served */
libxl_domain_config *guest_config;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 90340ab1bb..0e12329064 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -75,6 +75,7 @@ libxl_error = Enumeration("error", [
(-29, "QMP_COMMAND_NOT_FOUND"), # the requested command has not been found
(-30, "QMP_DEVICE_NOT_ACTIVE"), # a device has failed to be become active
(-31, "QMP_DEVICE_NOT_FOUND"), # the requested device has not been found
+ (-32, "QEMU_API"), # QEMU's replies don't contains expected members
], value_namespace = "")
libxl_domain_type = Enumeration("domain_type", [
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (10 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 11/14] libxl: QEMU startup sync based on QMP Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:05 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async Anthony PERARD
` (3 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This will be used in a later patch.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Notes:
v7:
Add do{}while around the MACRO.
formating nits changes.
v6:
new local macro GRAB_VERSION
better definition of qemu_version field in libxl_internal.h
v5:
initialise qemu_version struct in libxl__ev_qmp_init
tools/libxl/libxl_internal.h | 8 ++++++++
tools/libxl/libxl_qmp.c | 25 +++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7c2e0edff8..64732bd16a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -433,6 +433,14 @@ struct libxl__ev_qmp {
libxl__ev_qmp_callback *callback;
int payload_fd; /* set to send a fd with the command, -1 otherwise */
+ /* read-only when Connected
+ * and not to be accessed by the caller otherwise */
+ struct {
+ int major;
+ int minor;
+ int micro;
+ } qemu_version;
+
/*
* remaining fields are private to libxl_ev_qmp_*
*/
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 3c2545922e..6954a909e5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -1869,6 +1869,27 @@ static int qmp_ev_handle_message(libxl__egc *egc,
return ERROR_PROTOCOL_ERROR_QMP;
}
+ /*
+ * Store advertised QEMU version
+ * { "QMP": { "version": {
+ * "qemu": { "major": int, "minor": int, "micro": int } } } }
+ */
+ 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);
+#define GRAB_VERSION(level) do { \
+ ev->qemu_version.level = libxl__json_object_get_integer( \
+ libxl__json_map_get(#level, o, JSON_INTEGER)); \
+ } while (0)
+ GRAB_VERSION(major);
+ GRAB_VERSION(minor);
+ GRAB_VERSION(micro);
+#undef GRAB_VERSION
+ LOGD(DEBUG, ev->domid, "QEMU version: %d.%d.%d",
+ ev->qemu_version.major,
+ ev->qemu_version.minor,
+ ev->qemu_version.micro);
+
/* Prepare next message to send */
assert(!ev->tx_buf);
ev->id = ev->next_id++;
@@ -1988,6 +2009,10 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
ev->msg = NULL;
ev->msg_id = 0;
+
+ ev->qemu_version.major = -1;
+ ev->qemu_version.minor = -1;
+ ev->qemu_version.micro = -1;
}
int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (11 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:09 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp Anthony PERARD
` (2 subsequent siblings)
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
This create an extra step for the two call sites of the function.
libxl__domain_suspend_device_model() in this patch gets an extra error
variable (there is ret and rc), but ret goes away in the next patch.
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
libxl_domain_soft_reset() haven't been tested, as it doesn't appear to
possible to call the function from xl.
---
Notes:
v6:
fix multiple way to report errors,
libxl__domain_suspend_device_model will now only report via
callbacks, and return void
add rc in libxl__domain_suspend_device_model (ret isn't a proper rc
as libxl__qmp_save don't return one)
tools/libxl/libxl_create.c | 35 +++++++++++++++++++++-----------
tools/libxl/libxl_dom_suspend.c | 36 ++++++++++++++++++++-------------
tools/libxl/libxl_internal.h | 7 +++++--
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d5c13c0f35..232e94b85b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1771,6 +1771,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,
@@ -1853,11 +1856,24 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
goto out;
}
- rc = libxl__domain_suspend_device_model(gc, &dss->dsps);
- if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to suspend device model.");
- goto out;
- }
+ dss->dsps.ao = ao;
+ dss->dsps.callback_device_model_done = soft_reset_dm_suspended;
+ libxl__domain_suspend_device_model(egc, &dss->dsps); /* must be last */
+
+ 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
@@ -1865,18 +1881,13 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
* 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..f8ff5cf0c5 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -68,10 +68,12 @@ out:
/*----- callbacks, called by xc_domain_save -----*/
-int libxl__domain_suspend_device_model(libxl__gc *gc,
+void libxl__domain_suspend_device_model(libxl__egc *egc,
libxl__domain_suspend_state *dsps)
{
+ STATE_AO_GC(dsps->ao);
int ret = 0;
+ int rc = 0;
uint32_t const domid = dsps->domid;
const char *const filename = dsps->dm_savefile;
@@ -83,18 +85,29 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
break;
}
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
- if (libxl__qmp_stop(gc, domid))
- return ERROR_FAIL;
+ ret = libxl__qmp_stop(gc, domid);
+ if (ret) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
/* Save DM state into filename */
ret = libxl__qmp_save(gc, domid, filename, dsps->live);
- if (ret)
+ if (ret) {
+ rc = ERROR_FAIL;
unlink(filename);
+ goto out;
+ }
break;
default:
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
- return ret;
+out:
+ if (rc)
+ LOGD(ERROR, dsps->domid,
+ "failed to suspend device model, rc=%d", rc);
+ dsps->callback_device_model_done(egc, dsps, rc); /* must be last */
}
static void domain_suspend_common_wait_guest(libxl__egc *egc,
@@ -371,20 +384,15 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
libxl__domain_suspend_state *dsps)
{
STATE_AO_GC(dsps->ao);
- int rc;
libxl__ev_evtchn_cancel(gc, &dsps->guest_evtchn);
libxl__ev_xswatch_deregister(gc, &dsps->guest_watch);
libxl__ev_time_deregister(gc, &dsps->guest_timeout);
if (dsps->type == LIBXL_DOMAIN_TYPE_HVM) {
- rc = libxl__domain_suspend_device_model(gc, dsps);
- if (rc) {
- LOGD(ERROR, dsps->domid,
- "libxl__domain_suspend_device_model failed ret=%d", rc);
- domain_suspend_common_done(egc, dsps, rc);
- return;
- }
+ dsps->callback_device_model_done = domain_suspend_common_done;
+ libxl__domain_suspend_device_model(egc, dsps); /* must be last */
+ return;
}
domain_suspend_common_done(egc, dsps, 0);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 64732bd16a..9dcc474b5b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3423,6 +3423,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 rc);
void (*callback_common_done)(libxl__egc*,
struct libxl__domain_suspend_state*, int ok);
};
@@ -4044,8 +4046,9 @@ static inline bool libxl__save_helper_inuse(const libxl__save_helper_state *shs)
return libxl__ev_child_inuse(&shs->child);
}
-/* 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,
+/* Each time the dm needs to be saved, we must call suspend and then save
+ * calls dsps->callback_device_model_done when done */
+_hidden void 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (12 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async Anthony PERARD
@ 2018-11-23 13:53 ` Anthony PERARD
2018-12-21 16:12 ` Ian Jackson
2018-11-23 13:57 ` [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
2018-12-21 16:13 ` Ian Jackson
15 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Ian Jackson, Wei Liu
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:
v7:
use libxl__remove_file instead of unlink.
comment that libxl__qmp_suspend_save can callback synchronously
v6:
extract open call from libxl__carefd_opened to respect coding style
libxl__qmp_suspend_save now always report success/error
via dsps->callback_device_model_done
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 | 19 +---
tools/libxl/libxl_internal.h | 11 ++-
tools/libxl/libxl_qmp.c | 160 +++++++++++++++++++++++++-------
3 files changed, 139 insertions(+), 51 deletions(-)
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index f8ff5cf0c5..d1af3a6573 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,6 @@ void libxl__domain_suspend_device_model(libxl__egc *egc,
libxl__domain_suspend_state *dsps)
{
STATE_AO_GC(dsps->ao);
- int ret = 0;
int rc = 0;
uint32_t const domid = dsps->domid;
const char *const filename = dsps->dm_savefile;
@@ -85,19 +85,9 @@ void libxl__domain_suspend_device_model(libxl__egc *egc,
break;
}
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
- ret = libxl__qmp_stop(gc, domid);
- if (ret) {
- rc = ERROR_FAIL;
- goto out;
- }
- /* Save DM state into filename */
- ret = libxl__qmp_save(gc, domid, filename, dsps->live);
- if (ret) {
- rc = ERROR_FAIL;
- unlink(filename);
- goto out;
- }
- break;
+ /* calls dsps->callback_device_model_done when done */
+ libxl__qmp_suspend_save(egc, dsps); /* must be last */
+ return;
default:
rc = ERROR_INVAL;
goto out;
@@ -406,6 +396,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 9dcc474b5b..f766359fee 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1951,13 +1951,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 */
@@ -3421,6 +3416,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*,
@@ -3432,6 +3428,11 @@ int libxl__domain_suspend_init(libxl__egc *egc,
libxl__domain_suspend_state *dsps,
libxl_domain_type type);
+/* calls dsps->callback_device_model_done when done
+ * may synchronously calls this callback */
+_hidden void libxl__qmp_suspend_save(libxl__egc *egc,
+ 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 6954a909e5..32331d67e5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -405,12 +405,12 @@ static int qmp_handle_response(libxl__gc *gc, libxl__qmp_handler *qmp,
* = 0 if qemu's version == asked version
* > 0 if qemu's version > asked version
*/
-static int qmp_qemu_compare_version(libxl__qmp_handler *qmp, int major,
- int minor, int micro)
+static int qmp_ev_qemu_compare_version(libxl__ev_qmp *ev, int major,
+ int minor, int micro)
{
#define CHECK_VERSION(level) do { \
- if (qmp->version.level > (level)) return +1; \
- if (qmp->version.level < (level)) return -1; \
+ if (ev->qemu_version.level > (level)) return +1; \
+ if (ev->qemu_version.level < (level)) return -1; \
} while (0)
CHECK_VERSION(major);
@@ -1019,29 +1019,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_compare_version(qmp, 2, 11, 0) >= 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;
@@ -1070,11 +1047,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);
@@ -1314,6 +1286,130 @@ int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
return ret;
}
+
+/*
+ * Functions 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);
+
+/* calls dsps->callback_device_model_done when done */
+void libxl__qmp_suspend_save(libxl__egc *egc,
+ libxl__domain_suspend_state *dsps)
+{
+ EGC_GC;
+ int rc;
+ libxl__ev_qmp *ev = &dsps->qmp;
+
+ ev->ao = dsps->ao;
+ ev->domid = dsps->domid;
+ ev->callback = dm_stopped;
+ ev->payload_fd = -1;
+
+ rc = libxl__ev_qmp_send(gc, ev, "stop", NULL);
+ if (rc)
+ goto error;
+
+ return;
+
+error:
+ dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+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;
+
+ if (rc)
+ goto error;
+
+ ev->payload_fd = open(filename, O_WRONLY | O_CREAT, 0600);
+ if (ev->payload_fd < 0) {
+ LOGED(ERROR, ev->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->payload_fd >= 0) {
+ close(ev->payload_fd);
+ libxl__remove_file(gc, filename);
+ ev->payload_fd = -1;
+ }
+ 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;
+ int fdset;
+ const libxl__json_object *o;
+ libxl__json_object *args = NULL;
+ libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
+
+ close(ev->payload_fd);
+ ev->payload_fd = -1;
+
+ if (rc)
+ goto error;
+
+ o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
+ if (!o) {
+ rc = ERROR_QEMU_API;
+ goto error;
+ }
+ fdset = libxl__json_object_get_integer(o);
+
+ ev->callback = dm_state_saved;
+
+ /* The `live` parameter was added to QEMU 2.11. It signals QEMU that
+ * the save operation is for a live migration rather than for taking a
+ * snapshot. */
+ if (qmp_ev_qemu_compare_version(ev, 2, 11, 0) >= 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:
+ assert(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)
+ libxl__remove_file(gc, dsps->dm_savefile);
+
+ dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+
/* ------------ Implementation of libxl__ev_qmp ---------------- */
/*
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (13 preceding siblings ...)
2018-11-23 13:53 ` [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp Anthony PERARD
@ 2018-11-23 13:57 ` Anthony PERARD
2018-12-21 16:13 ` Ian Jackson
15 siblings, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2018-11-23 13:57 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu
On Fri, Nov 23, 2018 at 01:53:41PM +0000, Anthony PERARD wrote:
> Patch series available in this git branch:
> https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-ev-qmp-v
That should have read: br.libxl-ev-qmp-v7
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK
2018-11-23 13:53 ` [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK Anthony PERARD
@ 2018-12-21 14:30 ` Ian Jackson
2019-01-04 12:07 ` Anthony PERARD
0 siblings, 1 reply; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 14:30 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK"):
> This patch change the behavior of libxl__sendmsg_fds to retry sendmsg on
> EINTR error.
>
> This patch also allow a caller of libxl__sendmsg_fds to deal with
> EWOULDBLOCK. The function now requires to send only 1 byte of data so
> that when dealing with non-blocking fds a EWOULDBLOCK error would mean
> that the fds haven't been sent yet. Current caller already send only 1
> byte.
Even with a blocking fd, sendmsg may in principle report a short
write. (So the commit message should talk about short writes in
general.)
> Notes:
> v7:
> always assert datalen == 1, but only fail when sendmsg haven't send
> everything (r != datalen)
> check sendmsg return value on success as well (check for short write)
Rather than having a function which takes an argument which
mandatorily takes the value 1, how about simply deleting that
argument ?
You can do that in a followup patch if you like. In the meantime:
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare
2018-11-23 13:53 ` [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare Anthony PERARD
@ 2018-12-21 14:32 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 14:32 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare"):
> .. to be able to re-use qmp_prepare_cmd with libxl__ev_qmp.
>
> This patch also add the QMP end of command '\r\n' into the generated
> string as every caller will needs this.
>
> There should be no functional change.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON
2018-11-23 13:53 ` [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON Anthony PERARD
@ 2018-12-21 14:33 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 14:33 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON"):
> That wrapper is going to be used to safely log a json_object, as
> libxl__json_object_to_json return NULL on error. In the error case,
> JSON() will return an invalid json string.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2018-11-23 13:53 ` [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_* Anthony PERARD
@ 2018-12-21 15:36 ` Ian Jackson
2018-12-21 15:59 ` Ian Jackson
2019-01-03 12:40 ` Anthony PERARD
0 siblings, 2 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 15:36 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> This patch implement the API libxl__ev_qmp documented in the previous
> patch, "libxl: Design of an async API to issue QMP commands to QEMU".
>
> Since this API is to interact with QEMU via the QMP protocol, it also
> implement a QMP client. The specification for the QEMU Machine Protocol
> (QMP) can be found in the QEMU repository at:
> https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/interop/qmp-spec.txt
Thanks.
I have only fairly minor comments now. The biggest one remaining is
about the use of EGC_GC which I think probably wants to become
STATE_AO_GC throughout. See below...
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 1c7a3b22f4..056de9de2f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -412,6 +412,19 @@ _hidden int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> + /* receive buffer */
> + char *rx_buf;
rx_buf needs a comment saying it comes from NOGC since otherwise one
would assume it came from the ao gc like the other buffers.
(Could it come from the ao gc instead?)
> +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* Update the state of `efd` to match the permited state */
> +{
This function is legal only in states other than disconnected.
Needs to be documented.
> +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> + libxl__qmp_state new_state)
> + /* on entry: !broken and !disconnected */
> +{
> + switch (new_state) {
> + case qmp_state_disconnected:
> + break;
> + case qmp_state_connecting:
> + assert(ev->state == qmp_state_disconnected);
> + break;
> + case qmp_state_capability_negotiation:
> + assert(ev->state == qmp_state_connecting);
> + break;
> + case qmp_state_waiting_reply:
> + assert(ev->state == qmp_state_capability_negotiation ||
> + ev->state == qmp_state_connected);
> + break;
> + case qmp_state_connected:
> + assert(ev->state == qmp_state_waiting_reply);
> + break;
> + }
> +
> + ev->state = new_state;
I think this function needs to update efd ? What am I missing ?
The comment doesn't say what the output state is but naturally I
assume that it is precisely new_state, and not some transitional
mixture. If it is intended to produce a transitional mixture that
ought to be documented.
For a concrete example: if on entry, with new_state==disconnected, we
are `connecting' then: efd will be looking for POLLIN. But it needs
to become Idle.
> +/* Setup connection */
> +
> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* disconnected -> connecting but with `msg` free
> + * on error: broken */
> +{
This function looks fine to me. However, earlier I wrote this:
Contrary to the state description, this function does not
transition rx_buf from free to used. However, I think this
would probably be more easily remedied by changing the
definition of `used' to permit NULL/0/0. You might want to use
a different word to `used', `inuse' perhaps ?
This is still true. That is, your state description for `connecting'
says that rx_buf is `used'. And your description lower about what
rx_buf being `used' means says that rx_buf must be `allocated'.
I think this would probably be best resolved by writing:
* free used
- * rx_buf NULL allocated
+ * rx_buf NULL NULL or allocated
* rx_buf_size 0 allocation size of `rx_buf`
* rx_buf_used 0 <= rx_buf_size, actual data in the
* buffer
Ie just to change the internal spec. I am going to assume for the
rest of the review that the code is right and the internal spec will
be updated. (I don't think it is necessary to change the descriptions
of rx_buf_size and rx_buf_used; it will be clear that the `allocation
size' of a NULL must be 0.)
> +/* QMP FD callbacks */
> +
> +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
...
> +static int qmp_ev_callback_writable(libxl__gc *gc,
> + libxl__ev_qmp *ev, int fd)
> + /* on entry: !disconnected
> + * on return, one of these state transition:
> + * waiting_reply (with msg set) -> waiting_reply (with msg free)
> + * tx_buf set -> same state or tx_buf free
> + * tx_buf free -> no state change
> + * on error: broken */
> +{
...
> + assert(ev->tx_buf);
> + if (!ev->tx_buf)
> + return 0;
I think the if is vestigial.
> + while (ev->tx_buf_off < ev->tx_buf_len) {
> + r = write(fd, ev->tx_buf + ev->tx_buf_off,
> + ev->tx_buf_len - ev->tx_buf_off);
> + if (r < 0) {
> + if (errno == EINTR)
> + continue;
> + if (errno == EWOULDBLOCK)
> + break;
> + LOGED(ERROR, ev->domid, "failed to write to QMP socket");
> + return ERROR_FAIL;
> + }
> + ev->tx_buf_off += r;
Can you assert that the value of r was within range ? (Perhaps this
is paranoia on my part, but, still.)
> +static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
> + libxl__json_object **o_r)
> + /* Find a JSON object and store it in o_r.
> + * return ERROR_NOTFOUND if no object is found.
> + * `o_r` is allocated within `egc`.
Why not allocate o_r from within AO_GC ?
ISTM that taking it from egc is a beartrap. If you do want to
allocate it from egc, this should definitely be documented in the
internal public api for libxl__ev_qmp_callback. Right now a caller
might well reasonably assume that the libxl__json_object *response
they get is useable for the whole ao. Indeed future callers might
even need that semantic.
To do this, use STATE_AO_GC instead of EGC_GC.
TBH I think you should probably do that throughout.
> +static int qmp_ev_parse_error_messages(libxl__egc *egc,
> + libxl__ev_qmp *ev,
> + const libxl__json_object *resp)
Personally I would have preferred to read this and
qmp_ev_handle_message in the other order, so I would swap them. Up to
you.
> + /* no state change */
> +{
> + EGC_GC;
> + int rc;
> + const char *s;
> + const libxl__json_object *o;
> + const libxl__json_object *err;
> +
> + /*
> + * { "error": { "class": string, "desc": string } }
> + */
> +
> + err = libxl__json_map_get("error", resp, JSON_MAP);
> +
> + o = libxl__json_map_get("class", err, JSON_STRING);
I wondered: surely err can be NULL ? I didn't find any docs saying
that it couldn't; nor that it tolerated NULL for o on input.
However, reading the implementation I see that libxl__json_map_get
calls libxl__json_object_is_map which does indeed handle o==0.
Could you perhaps add a comment (in libxl_internal.h near
libxl__json_map_get) et al about this ?
> +static int qmp_ev_handle_message(libxl__egc *egc,
> + libxl__ev_qmp *ev,
> + const libxl__json_object *resp)
...
> + /* Prepare next message to send */
> + assert(!ev->tx_buf);
> + ev->id = ev->next_id++;
> + buf = qmp_prepare_cmd(libxl__ao_inprogress_gc(ev->ao),
> + "qmp_capabilities", NULL, ev->id);
Erk, I don't like the open coded call to libxl__ao_inprogress_gc. I'm
becoming more convinced you just wanted AO_GC or STATE_AO_GC
everywhere.
> +void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* * -> disconnected */
> +{
> + LOGD(DEBUG, ev->domid, " ev %p", ev);
> +
> + free(ev->rx_buf);
> +
> + libxl__ev_fd_deregister(gc, &ev->efd);
> + libxl__carefd_close(ev->cfd);
> +
> + libxl__ev_qmp_init(ev);
> +}
It's a small point, but it would be nicer to move the free of rx_buf
nearer the call to libxl__ev_qmp_init which zeroes it.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure
2018-11-23 13:53 ` [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure Anthony PERARD
@ 2018-12-21 15:57 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 15:57 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 07/14] 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.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2018-12-21 15:36 ` Ian Jackson
@ 2018-12-21 15:59 ` Ian Jackson
2019-01-03 12:40 ` Anthony PERARD
1 sibling, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 15:59 UTC (permalink / raw)
To: Anthony PERARD, xen-devel, Wei Liu
Ian Jackson writes ("Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> I have only fairly minor comments now. The biggest one remaining is
> about the use of EGC_GC which I think probably wants to become
> STATE_AO_GC throughout. See below...
I realise that I should state explicitly that libxl__ev_qmp_callback
must still get an egc.
The egc should be passed through all the layers, but its gc should
generally not be used.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state
2018-11-23 13:53 ` [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state Anthony PERARD
@ 2018-12-21 16:00 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:00 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state"):
> These two new functions libxl__domain_build_state_{init,dispose} should
> be called every time a new libxl__domain_build_state comes to existance.
>
> There seems to be two of them, one with the domain creation machinery,
> and one in the stub_dm_spawn.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU
2018-11-23 13:53 ` [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU Anthony PERARD
@ 2018-12-21 16:01 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:01 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU"):
> This patch moves the creation of the QMP unix socket from QEMU to libxl.
> But libxl doesn't rely on this yet.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 11/14] libxl: QEMU startup sync based on QMP
2018-11-23 13:53 ` [PATCH v7 11/14] libxl: QEMU startup sync based on QMP Anthony PERARD
@ 2018-12-21 16:03 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:03 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 11/14] libxl: QEMU startup sync based on QMP"):
> This is only activated when dm_restrict=1, as explained in a 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>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp
2018-11-23 13:53 ` [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp Anthony PERARD
@ 2018-12-21 16:05 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:05 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp"):
> This will be used in a later patch.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> + 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);
> +#define GRAB_VERSION(level) do { \
> + ev->qemu_version.level = libxl__json_object_get_integer( \
> + libxl__json_map_get(#level, o, JSON_INTEGER)); \
> + } while (0)
> + GRAB_VERSION(major);
> + GRAB_VERSION(minor);
> + GRAB_VERSION(micro);
Earlier I wrote:
I would prefer the indentation to be such that the statement inside
the macro is indented like the ones outside.
Ie like this:
+#define GRAB_VERSION(level) do { \
+ ev->qemu_version.level = libxl__json_object_get_integer( \
+ libxl__json_map_get(#level, o, JSON_INTEGER)); \
+ } while (0)
+ GRAB_VERSION(major);
But up to you. My ack stands either way.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async
2018-11-23 13:53 ` [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async Anthony PERARD
@ 2018-12-21 16:09 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:09 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async"):
> This create an extra step for the two call sites of the function.
>
> libxl__domain_suspend_device_model() in this patch gets an extra error
> variable (there is ret and rc), but ret goes away in the next patch.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp
2018-11-23 13:53 ` [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp Anthony PERARD
@ 2018-12-21 16:12 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:12 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 14/14] 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.
>
> 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.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
` (14 preceding siblings ...)
2018-11-23 13:57 ` [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
@ 2018-12-21 16:13 ` Ian Jackson
15 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2018-12-21 16:13 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("[PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_*"):
> Patch series available in this git branch:
> https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.libxl-ev-qmp-v
Thanks. Sorry for being so slow to review this. It is very nearly
ready and I hope it will make 4.12.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2018-12-21 15:36 ` Ian Jackson
2018-12-21 15:59 ` Ian Jackson
@ 2019-01-03 12:40 ` Anthony PERARD
2019-01-04 11:14 ` Anthony PERARD
2019-01-04 11:21 ` Ian Jackson
1 sibling, 2 replies; 35+ messages in thread
From: Anthony PERARD @ 2019-01-03 12:40 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu
On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> > This patch implement the API libxl__ev_qmp documented in the previous
> > patch, "libxl: Design of an async API to issue QMP commands to QEMU".
> >
> > Since this API is to interact with QEMU via the QMP protocol, it also
> > implement a QMP client. The specification for the QEMU Machine Protocol
> > (QMP) can be found in the QEMU repository at:
> > https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/interop/qmp-spec.txt
>
> Thanks.
>
> I have only fairly minor comments now. The biggest one remaining is
> about the use of EGC_GC which I think probably wants to become
> STATE_AO_GC throughout. See below...
>
>
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 1c7a3b22f4..056de9de2f 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -412,6 +412,19 @@ _hidden int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
>
> > + /* receive buffer */
> > + char *rx_buf;
>
> rx_buf needs a comment saying it comes from NOGC since otherwise one
> would assume it came from the ao gc like the other buffers.
>
> (Could it come from the ao gc instead?)
I guess it's fine to have the rx_buf comes from ao gc. The buffer isn't
that big (maybe 100kB in worse cases).
>
> > +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> > + /* Update the state of `efd` to match the permited state */
> > +{
>
> This function is legal only in states other than disconnected.
> Needs to be documented.
Will do.
> > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > + libxl__qmp_state new_state)
> > + /* on entry: !broken and !disconnected */
> > +{
> > + switch (new_state) {
> > + case qmp_state_disconnected:
> > + break;
> > + case qmp_state_connecting:
> > + assert(ev->state == qmp_state_disconnected);
> > + break;
> > + case qmp_state_capability_negotiation:
> > + assert(ev->state == qmp_state_connecting);
> > + break;
> > + case qmp_state_waiting_reply:
> > + assert(ev->state == qmp_state_capability_negotiation ||
> > + ev->state == qmp_state_connected);
> > + break;
> > + case qmp_state_connected:
> > + assert(ev->state == qmp_state_waiting_reply);
> > + break;
> > + }
> > +
> > + ev->state = new_state;
>
> I think this function needs to update efd ? What am I missing ?
Yes, I think it's fine to call qmp_ev_ensure_reading_writing here ( or
even inline it) and qmp_ev_ensure_reading_writing won't needs to be
call from other places.
> The comment doesn't say what the output state is but naturally I
> assume that it is precisely new_state, and not some transitional
> mixture. If it is intended to produce a transitional mixture that
> ought to be documented.
>
> For a concrete example: if on entry, with new_state==disconnected, we
> are `connecting' then: efd will be looking for POLLIN. But it needs
> to become Idle.
Once I update efd with this function, I think qmp_ev_set_state should
always output precisely new_state. But new_state alone might not be
enough in few cases (waiting_reply) to describe a full state, but it
will be one of the possible internal state as describe in the state
documentation of the implementation.
>
> > +/* Setup connection */
> > +
> > +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> > + /* disconnected -> connecting but with `msg` free
> > + * on error: broken */
> > +{
>
> This function looks fine to me. However, earlier I wrote this:
>
> Contrary to the state description, this function does not
> transition rx_buf from free to used. However, I think this
> would probably be more easily remedied by changing the
> definition of `used' to permit NULL/0/0. You might want to use
> a different word to `used', `inuse' perhaps ?
>
> This is still true. That is, your state description for `connecting'
> says that rx_buf is `used'. And your description lower about what
> rx_buf being `used' means says that rx_buf must be `allocated'.
>
> I think this would probably be best resolved by writing:
>
> * free used
> - * rx_buf NULL allocated
> + * rx_buf NULL NULL or allocated
> * rx_buf_size 0 allocation size of `rx_buf`
> * rx_buf_used 0 <= rx_buf_size, actual data in the
> * buffer
I'll make this change.
> Ie just to change the internal spec. I am going to assume for the
> rest of the review that the code is right and the internal spec will
> be updated. (I don't think it is necessary to change the descriptions
> of rx_buf_size and rx_buf_used; it will be clear that the `allocation
> size' of a NULL must be 0.)
>
>
> > +/* QMP FD callbacks */
> > +
> > +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> ...
>
> > +static int qmp_ev_callback_writable(libxl__gc *gc,
> > + libxl__ev_qmp *ev, int fd)
> > + /* on entry: !disconnected
> > + * on return, one of these state transition:
> > + * waiting_reply (with msg set) -> waiting_reply (with msg free)
> > + * tx_buf set -> same state or tx_buf free
> > + * tx_buf free -> no state change
> > + * on error: broken */
> > +{
> ...
> > + assert(ev->tx_buf);
> > + if (!ev->tx_buf)
> > + return 0;
>
> I think the if is vestigial.
I'll remove it.
> > + while (ev->tx_buf_off < ev->tx_buf_len) {
> > + r = write(fd, ev->tx_buf + ev->tx_buf_off,
> > + ev->tx_buf_len - ev->tx_buf_off);
> > + if (r < 0) {
> > + if (errno == EINTR)
> > + continue;
> > + if (errno == EWOULDBLOCK)
> > + break;
> > + LOGED(ERROR, ev->domid, "failed to write to QMP socket");
> > + return ERROR_FAIL;
> > + }
> > + ev->tx_buf_off += r;
>
> Can you assert that the value of r was within range ? (Perhaps this
> is paranoia on my part, but, still.)
I do assert the value of tx_buf_off which does include r.
But I can add
`assert(r > 0 && r <= (ev->rx_buf_size - ev->rx_buf_used));'.
> > +static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
> > + libxl__json_object **o_r)
> > + /* Find a JSON object and store it in o_r.
> > + * return ERROR_NOTFOUND if no object is found.
> > + * `o_r` is allocated within `egc`.
>
> Why not allocate o_r from within AO_GC ?
>
> ISTM that taking it from egc is a beartrap. If you do want to
> allocate it from egc, this should definitely be documented in the
> internal public api for libxl__ev_qmp_callback. Right now a caller
> might well reasonably assume that the libxl__json_object *response
> they get is useable for the whole ao. Indeed future callers might
> even need that semantic.
Indeed, that can be an issue. I'll make the changes to use ao gc instead
of egc.
> To do this, use STATE_AO_GC instead of EGC_GC.
> TBH I think you should probably do that throughout.
>
> > +static int qmp_ev_parse_error_messages(libxl__egc *egc,
> > + libxl__ev_qmp *ev,
> > + const libxl__json_object *resp)
> > + /* no state change */
> > +{
> > + EGC_GC;
> > + int rc;
> > + const char *s;
> > + const libxl__json_object *o;
> > + const libxl__json_object *err;
> > +
> > + /*
> > + * { "error": { "class": string, "desc": string } }
> > + */
> > +
> > + err = libxl__json_map_get("error", resp, JSON_MAP);
> > +
> > + o = libxl__json_map_get("class", err, JSON_STRING);
>
> I wondered: surely err can be NULL ? I didn't find any docs saying
> that it couldn't; nor that it tolerated NULL for o on input.
>
> However, reading the implementation I see that libxl__json_map_get
> calls libxl__json_object_is_map which does indeed handle o==0.
> Could you perhaps add a comment (in libxl_internal.h near
> libxl__json_map_get) et al about this ?
I'll add the comments.
> > +static int qmp_ev_handle_message(libxl__egc *egc,
> > + libxl__ev_qmp *ev,
> > + const libxl__json_object *resp)
> ...
> > + /* Prepare next message to send */
> > + assert(!ev->tx_buf);
> > + ev->id = ev->next_id++;
> > + buf = qmp_prepare_cmd(libxl__ao_inprogress_gc(ev->ao),
> > + "qmp_capabilities", NULL, ev->id);
>
> Erk, I don't like the open coded call to libxl__ao_inprogress_gc. I'm
> becoming more convinced you just wanted AO_GC or STATE_AO_GC
> everywhere.
>
> > +void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
> > + /* * -> disconnected */
> > +{
> > + LOGD(DEBUG, ev->domid, " ev %p", ev);
> > +
> > + free(ev->rx_buf);
> > +
> > + libxl__ev_fd_deregister(gc, &ev->efd);
> > + libxl__carefd_close(ev->cfd);
> > +
> > + libxl__ev_qmp_init(ev);
> > +}
>
> It's a small point, but it would be nicer to move the free of rx_buf
> nearer the call to libxl__ev_qmp_init which zeroes it.
As rx_buf is probably going to be allocated from ao gc, the free won't
be needed anymore. Or, I could realloc with a new size of 0:
libxl__realloc(maybe_gc, ev->rx_buf, 0);
and that would free the memory. I've check realloc, libxl__realloc and
libxl__free_all, and they all seems to do the right things when the new
size is 0.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2019-01-03 12:40 ` Anthony PERARD
@ 2019-01-04 11:14 ` Anthony PERARD
2019-01-04 11:21 ` Ian Jackson
1 sibling, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2019-01-04 11:14 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu
On Thu, Jan 03, 2019 at 12:40:18PM +0000, Anthony PERARD wrote:
> On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> > Anthony PERARD writes ("[PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> > > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > > + libxl__qmp_state new_state)
> > > + /* on entry: !broken and !disconnected */
> > > +{
[...]
> > > +
> > > + ev->state = new_state;
> >
> > I think this function needs to update efd ? What am I missing ?
>
> Yes, I think it's fine to call qmp_ev_ensure_reading_writing here ( or
> even inline it) and qmp_ev_ensure_reading_writing won't needs to be
> call from other places.
Actually, I still need to update efd after having transmit data to QEMU,
as ev->state isn't updated in that case. So I'll have efd update when
qmp_ev_set_state is called, and when the tx_buf is maybe sent.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2019-01-03 12:40 ` Anthony PERARD
2019-01-04 11:14 ` Anthony PERARD
@ 2019-01-04 11:21 ` Ian Jackson
2019-01-04 11:41 ` Anthony PERARD
1 sibling, 1 reply; 35+ messages in thread
From: Ian Jackson @ 2019-01-04 11:21 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
(Skipping the parts where you were just agreeing with me...)
Anthony PERARD writes ("Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > > + libxl__qmp_state new_state)
> > > + /* on entry: !broken and !disconnected */
> > > +{
...
> > I think this function needs to update efd ? What am I missing ?
>
> Yes, I think it's fine to call qmp_ev_ensure_reading_writing here ( or
> even inline it) and qmp_ev_ensure_reading_writing won't needs to be
> call from other places.
Sure.
> > The comment doesn't say what the output state is but naturally I
> > assume that it is precisely new_state, and not some transitional
> > mixture. If it is intended to produce a transitional mixture that
> > ought to be documented.
> >
> > For a concrete example: if on entry, with new_state==disconnected, we
> > are `connecting' then: efd will be looking for POLLIN. But it needs
> > to become Idle.
>
> Once I update efd with this function, I think qmp_ev_set_state should
> always output precisely new_state. But new_state alone might not be
> enough in few cases (waiting_reply) to describe a full state, but it
> will be one of the possible internal state as describe in the state
> documentation of the implementation.
That SGTM.
> > > + while (ev->tx_buf_off < ev->tx_buf_len) {
> > > + r = write(fd, ev->tx_buf + ev->tx_buf_off,
> > > + ev->tx_buf_len - ev->tx_buf_off);
> > > + if (r < 0) {
> > > + if (errno == EINTR)
> > > + continue;
> > > + if (errno == EWOULDBLOCK)
> > > + break;
> > > + LOGED(ERROR, ev->domid, "failed to write to QMP socket");
> > > + return ERROR_FAIL;
> > > + }
> > > + ev->tx_buf_off += r;
> >
> > Can you assert that the value of r was within range ? (Perhaps this
> > is paranoia on my part, but, still.)
>
> I do assert the value of tx_buf_off which does include r.
> But I can add
> `assert(r > 0 && r <= (ev->rx_buf_size - ev->rx_buf_used));'.
Yes. As I say worrying that write() might return something insane is
perhaps paranoia.
Do you mean `ev->tx_buf_len - ev->tx_buf_off' like in the write call ?
I would factor the expression out into a variable so you only have to
write it once.
> > Why not allocate o_r from within AO_GC ?
> >
> > ISTM that taking it from egc is a beartrap. If you do want to
> > allocate it from egc, this should definitely be documented in the
> > internal public api for libxl__ev_qmp_callback. Right now a caller
> > might well reasonably assume that the libxl__json_object *response
> > they get is useable for the whole ao. Indeed future callers might
> > even need that semantic.
>
> Indeed, that can be an issue. I'll make the changes to use ao gc instead
> of egc.
Thanks.
> > > +void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
> > > + /* * -> disconnected */
> > > +{
> > > + LOGD(DEBUG, ev->domid, " ev %p", ev);
> > > +
> > > + free(ev->rx_buf);
> > > +
> > > + libxl__ev_fd_deregister(gc, &ev->efd);
> > > + libxl__carefd_close(ev->cfd);
> > > +
> > > + libxl__ev_qmp_init(ev);
> > > +}
> >
> > It's a small point, but it would be nicer to move the free of rx_buf
> > nearer the call to libxl__ev_qmp_init which zeroes it.
>
> As rx_buf is probably going to be allocated from ao gc, the free won't
> be needed anymore.
Yay, less code.
> Or, I could realloc with a new size of 0:
> libxl__realloc(maybe_gc, ev->rx_buf, 0);
> and that would free the memory. I've check realloc, libxl__realloc and
> libxl__free_all, and they all seems to do the right things when the new
> size is 0.
We can probably afford to leak it for the duration of the ao.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
2019-01-04 11:21 ` Ian Jackson
@ 2019-01-04 11:41 ` Anthony PERARD
0 siblings, 0 replies; 35+ messages in thread
From: Anthony PERARD @ 2019-01-04 11:41 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu
On Fri, Jan 04, 2019 at 11:21:11AM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*"):
> > On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> > > > + while (ev->tx_buf_off < ev->tx_buf_len) {
> > > > + r = write(fd, ev->tx_buf + ev->tx_buf_off,
> > > > + ev->tx_buf_len - ev->tx_buf_off);
> > > > + if (r < 0) {
> > > > + if (errno == EINTR)
> > > > + continue;
> > > > + if (errno == EWOULDBLOCK)
> > > > + break;
> > > > + LOGED(ERROR, ev->domid, "failed to write to QMP socket");
> > > > + return ERROR_FAIL;
> > > > + }
> > > > + ev->tx_buf_off += r;
> > >
> > > Can you assert that the value of r was within range ? (Perhaps this
> > > is paranoia on my part, but, still.)
> >
> > I do assert the value of tx_buf_off which does include r.
> > But I can add
> > `assert(r > 0 && r <= (ev->rx_buf_size - ev->rx_buf_used));'.
>
> Yes. As I say worrying that write() might return something insane is
> perhaps paranoia.
>
> Do you mean `ev->tx_buf_len - ev->tx_buf_off' like in the write call ?
I was looking at the wrong function :(.
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK
2018-12-21 14:30 ` Ian Jackson
@ 2019-01-04 12:07 ` Anthony PERARD
2019-01-04 15:58 ` Ian Jackson
0 siblings, 1 reply; 35+ messages in thread
From: Anthony PERARD @ 2019-01-04 12:07 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu
On Fri, Dec 21, 2018 at 02:30:05PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK"):
> > This patch change the behavior of libxl__sendmsg_fds to retry sendmsg on
> > EINTR error.
> >
> > This patch also allow a caller of libxl__sendmsg_fds to deal with
> > EWOULDBLOCK. The function now requires to send only 1 byte of data so
> > that when dealing with non-blocking fds a EWOULDBLOCK error would mean
> > that the fds haven't been sent yet. Current caller already send only 1
> > byte.
>
> Even with a blocking fd, sendmsg may in principle report a short
> write. (So the commit message should talk about short writes in
> general.)
I have rewrite the commit message like this:
This patch change the behavior of libxl__sendmsg_fds to retry sendmsg on
EINTR error and return an error on short writes.
This patch allow a caller of libxl__sendmsg_fds to deal with EWOULDBLOCK
and short writes. The function now requires to send only 1 byte of data
so that when dealing with non-blocking fds a EWOULDBLOCK error would
mean that the fds haven't been sent yet. Current caller already send
only 1 byte.
> > Notes:
> > v7:
> > always assert datalen == 1, but only fail when sendmsg haven't send
> > everything (r != datalen)
> > check sendmsg return value on success as well (check for short write)
>
> Rather than having a function which takes an argument which
> mandatorily takes the value 1, how about simply deleting that
> argument ?
I'll add a new patch to do that at the end of the series.
> You can do that in a followup patch if you like. In the meantime:
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK
2019-01-04 12:07 ` Anthony PERARD
@ 2019-01-04 15:58 ` Ian Jackson
0 siblings, 0 replies; 35+ messages in thread
From: Ian Jackson @ 2019-01-04 15:58 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Wei Liu
Anthony PERARD writes ("Re: [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK"):
> On Fri, Dec 21, 2018 at 02:30:05PM +0000, Ian Jackson wrote:
> > Even with a blocking fd, sendmsg may in principle report a short
> > write. (So the commit message should talk about short writes in
> > general.)
>
> I have rewrite the commit message like this:
>
> This patch change the behavior of libxl__sendmsg_fds to retry sendmsg on
> EINTR error and return an error on short writes.
>
> This patch allow a caller of libxl__sendmsg_fds to deal with EWOULDBLOCK
> and short writes. The function now requires to send only 1 byte of data
> so that when dealing with non-blocking fds a EWOULDBLOCK error would
> mean that the fds haven't been sent yet. Current caller already send
> only 1 byte.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-01-04 15:58 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23 13:53 [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 01/14] libxl: Enhance libxl__sendmsg_fds to deal with EINTR and EWOULDBLOCK Anthony PERARD
2018-12-21 14:30 ` Ian Jackson
2019-01-04 12:07 ` Anthony PERARD
2019-01-04 15:58 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 02/14] libxl_qmp: Separate QMP message generation from qmp_send_prepare Anthony PERARD
2018-12-21 14:32 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 03/14] libxl_qmp: Change qmp_qemu_check_version to compare version Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 04/14] libxl: Add wrapper around libxl__json_object_to_json JSON Anthony PERARD
2018-12-21 14:33 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 05/14] libxl: Design of an async API to issue QMP commands to QEMU Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_* Anthony PERARD
2018-12-21 15:36 ` Ian Jackson
2018-12-21 15:59 ` Ian Jackson
2019-01-03 12:40 ` Anthony PERARD
2019-01-04 11:14 ` Anthony PERARD
2019-01-04 11:21 ` Ian Jackson
2019-01-04 11:41 ` Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 07/14] libxl_exec: Add libxl__spawn_initiate_failure Anthony PERARD
2018-12-21 15:57 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 08/14] libxl: Add init/dispose of for libxl__domain_build_state Anthony PERARD
2018-12-21 16:00 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 09/14] libxl_dm: Pre-open QMP socket for QEMU Anthony PERARD
2018-12-21 16:01 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 10/14] libxl: Add dmss_init/dispose for libxl__dm_spawn_state Anthony PERARD
2018-11-23 13:53 ` [PATCH v7 11/14] libxl: QEMU startup sync based on QMP Anthony PERARD
2018-12-21 16:03 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 12/14] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp Anthony PERARD
2018-12-21 16:05 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 13/14] libxl: Change libxl__domain_suspend_device_model() to be async Anthony PERARD
2018-12-21 16:09 ` Ian Jackson
2018-11-23 13:53 ` [PATCH v7 14/14] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp Anthony PERARD
2018-12-21 16:12 ` Ian Jackson
2018-11-23 13:57 ` [PATCH v7 00/14] libxl: Enable save/restore/migration of a restricted QEMU + libxl__ev_qmp_* Anthony PERARD
2018-12-21 16:13 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.