Hi On Wed, Sep 19, 2018 at 7:06 PM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > Hi, > > On Wed, Sep 19, 2018 at 05:51:13PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Wed, Sep 19, 2018 at 3:26 PM Victor Toso <victortoso@xxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > Tested the qmp part, so far, so good. Some comments/suggestions > > > bellow. > > > > > > On Fri, Aug 10, 2018 at 03:44:19PM +0200, marcandre.lureau@xxxxxxxxxx wrote: > > > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > > > > > > > > Add a few helper functions to deal with a QMP port channel, in order > > > > to ease json handling, and wrapping a few commands. > > > > > > > > (by convention, the port should have the name > > > > "org.qemu.monitor.qmp.0", but it's not strictly required) > > > > > > > > This helper is put into use in the virt-viewer "Add QEMU-like UI: VT > > > > console & basic VM status" series. > > > > > > > > Note: this adds a strong dependency on json-glib for > > > > spice-client-glib, a widely available and fairly small > > > > library. > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > > > > --- > > > > configure.ac | 2 + > > > > > > Could you also add it to meson? > > > > yes, that was already in my tree ;) > > > > > > > > > doc/reference/spice-gtk-docs.xml | 1 + > > > > doc/reference/spice-gtk-sections.txt | 27 ++ > > > > src/Makefile.am | 6 + > > > > src/map-file | 9 + > > > > src/qmp-port.c | 571 +++++++++++++++++++++++++++ > > > > src/qmp-port.h | 122 ++++++ > > > > src/spice-client.h | 1 + > > > > src/spice-glib-sym-file | 9 + > > > > 9 files changed, 748 insertions(+) > > > > create mode 100644 src/qmp-port.c > > > > create mode 100644 src/qmp-port.h > > > > > > > > diff --git a/configure.ac b/configure.ac > > > > index 7b32e29..e686d76 100644 > > > > --- a/configure.ac > > > > +++ b/configure.ac > > > > @@ -173,6 +173,8 @@ PKG_CHECK_MODULES(CAIRO, cairo >= 1.2.0) > > > > > > > > PKG_CHECK_MODULES(GTHREAD, gthread-2.0) > > > > > > > > +PKG_CHECK_MODULES(JSON, json-glib-1.0) > > > > + > > > > AC_ARG_ENABLE([webdav], > > > > AS_HELP_STRING([--enable-webdav=@<:@auto/yes/no@:>@], > > > > [Enable webdav support @<:@default=auto@:>@]), > > > > diff --git a/doc/reference/spice-gtk-docs.xml b/doc/reference/spice-gtk-docs.xml > > > > index db5dd3d..696c375 100644 > > > > --- a/doc/reference/spice-gtk-docs.xml > > > > +++ b/doc/reference/spice-gtk-docs.xml > > > > @@ -52,6 +52,7 @@ > > > > <xi:include href="xml/spice-audio.xml"/> > > > > <xi:include href="xml/smartcard-manager.xml"/> > > > > <xi:include href="xml/usb-device-manager.xml"/> > > > > + <xi:include href="xml/qmp-port.xml"/> > > > > <xi:include href="xml/spice-util.xml"/> > > > > <xi:include href="xml/spice-version.xml"/> > > > > <xi:include href="xml/spice-uri.xml"/> > > > > diff --git a/doc/reference/spice-gtk-sections.txt b/doc/reference/spice-gtk-sections.txt > > > > index a85df7a..a0336aa 100644 > > > > --- a/doc/reference/spice-gtk-sections.txt > > > > +++ b/doc/reference/spice-gtk-sections.txt > > > > @@ -494,6 +494,33 @@ SPICE_PORT_CHANNEL_GET_CLASS > > > > SpicePortChannelPrivate > > > > </SECTION> > > > > > > > > +<SECTION> > > > > +<FILE>qmp-port</FILE> > > > > +<TITLE>SpiceQmpPort</TITLE> > > > > + > > > > +<SUBSECTION> > > > > +SpiceQmpPort > > > > +SpiceQmpPortVmAction > > > > +SpiceQmpStatus > > > > +<SUBSECTION> > > > > +spice_qmp_port_get > > > > +spice_qmp_port_vm_action_async > > > > +spice_qmp_port_vm_action_finish > > > > +spice_qmp_port_query_status_async > > > > +spice_qmp_port_query_status_finish > > > > +spice_qmp_status_ref > > > > +spice_qmp_status_unref > > > > +<SUBSECTION Standard> > > > > +SPICE_IS_QMP_PORT > > > > +SPICE_IS_QMP_PORT_CLASS > > > > +SPICE_QMP_PORT > > > > +SPICE_QMP_PORT_CLASS > > > > +SPICE_QMP_PORT_GET_CLASS > > > > +SPICE_TYPE_QMP_PORT > > > > +spice_qmp_port_get_type > > > > +spice_qmp_status_get_type > > > > +</SECTION> > > > > + > > > > <SECTION> > > > > <FILE>spice-uri</FILE> > > > > spice_uri_get_scheme > > > > diff --git a/src/Makefile.am b/src/Makefile.am > > > > index afad922..8f8ccdf 100644 > > > > --- a/src/Makefile.am > > > > +++ b/src/Makefile.am > > > > @@ -77,6 +77,7 @@ SPICE_COMMON_CPPFLAGS = \ > > > > $(GLIB2_CFLAGS) \ > > > > $(GIO_CFLAGS) \ > > > > $(GOBJECT2_CFLAGS) \ > > > > + $(JSON_CFLAGS) \ > > > > $(SSL_CFLAGS) \ > > > > $(SASL_CFLAGS) \ > > > > $(GSTAUDIO_CFLAGS) \ > > > > @@ -181,6 +182,7 @@ libspice_client_glib_2_0_la_LIBADD = \ > > > > $(GIO_LIBS) \ > > > > $(GOBJECT2_LIBS) \ > > > > $(JPEG_LIBS) \ > > > > + $(JSON_LIBS) \ > > > > $(Z_LIBS) \ > > > > $(LZ4_LIBS) \ > > > > $(PIXMAN_LIBS) \ > > > > @@ -242,6 +244,8 @@ libspice_client_glib_2_0_la_SOURCES = \ > > > > channel-smartcard.c \ > > > > channel-usbredir.c \ > > > > channel-usbredir-priv.h \ > > > > + qmp-port.c \ > > > > + qmp-port.h \ > > > > smartcard-manager.c \ > > > > smartcard-manager-priv.h \ > > > > spice-uri.c \ > > > > @@ -292,6 +296,7 @@ libspice_client_glibinclude_HEADERS = \ > > > > channel-smartcard.h \ > > > > channel-usbredir.h \ > > > > channel-webdav.h \ > > > > + qmp-port.h \ > > > > usb-device-manager.h \ > > > > smartcard-manager.h \ > > > > spice-file-transfer-task.h \ > > > > @@ -525,6 +530,7 @@ glib_introspection_files = \ > > > > channel-record.c \ > > > > channel-smartcard.c \ > > > > channel-usbredir.c \ > > > > + qmp-port.c \ > > > > smartcard-manager.c \ > > > > usb-device-manager.c \ > > > > $(NULL) > > > > diff --git a/src/map-file b/src/map-file > > > > index cdb81c3..500683c 100644 > > > > --- a/src/map-file > > > > +++ b/src/map-file > > > > @@ -117,6 +117,15 @@ spice_port_channel_write_finish; > > > > spice_port_event; > > > > spice_port_write_async; > > > > spice_port_write_finish; > > > > +spice_qmp_port_get; > > > > +spice_qmp_port_get_type; > > > > +spice_qmp_port_query_status_async; > > > > +spice_qmp_port_query_status_finish; > > > > +spice_qmp_port_vm_action_async; > > > > +spice_qmp_port_vm_action_finish; > > > > +spice_qmp_status_get_type; > > > > +spice_qmp_status_ref; > > > > +spice_qmp_status_unref; > > > > spice_record_channel_get_type; > > > > spice_record_channel_send_data; > > > > spice_record_send_data; > > > > diff --git a/src/qmp-port.c b/src/qmp-port.c > > > > new file mode 100644 > > > > index 0000000..04ebc98 > > > > --- /dev/null > > > > +++ b/src/qmp-port.c > > > > @@ -0,0 +1,571 @@ > > > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > > > > +/* > > > > + Copyright (C) 2018 Red Hat, Inc. > > > > + > > > > + This library is free software; you can redistribute it and/or > > > > + modify it under the terms of the GNU Lesser General Public > > > > + License as published by the Free Software Foundation; either > > > > + version 2.1 of the License, or (at your option) any later version. > > > > + > > > > + This library is distributed in the hope that it will be useful, > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > + Lesser General Public License for more details. > > > > + > > > > + You should have received a copy of the GNU Lesser General Public > > > > + License along with this library; if not, see <http://www.gnu.org/licenses/>. > > > > +*/ > > > > +#include "config.h" > > > > + > > > > +#define _GNU_SOURCE > > > > +#include <string.h> > > > > +#include <json-glib/json-glib.h> > > > > +#include "spice-client.h" > > > > + > > > > +/** > > > > + * SECTION:qmp-port > > > > + * @short_description: QMP port helper > > > > + * @title: QMP port channel helper > > > > + * @section_id: > > > > + * @see_also: #SpicePortChannel > > > > + * @stability: Stable > > > > + * @include: spice-client.h > > > > + * > > > > + * A helper to handle QMP messages over a %SpicePortChannel. > > > > + * > > > > + * Since: 0.36 > > > > + */ > > > > + > > > > +typedef struct _SpiceQmpPortPrivate SpiceQmpPortPrivate; > > > > + > > > > +struct _SpiceQmpPortPrivate > > > > +{ > > > > + SpicePortChannel *channel; > > > > + gboolean ready; > > > > + > > > > + gint id_counter; > > > > > > This is only used in qmp() to get task's id. I'd say a helper > > > like get_task_id() with static gint id_counter would be nicer but > > > not too strong opnion about it. The spice-file-transfer-task uses > > > static xfer_id on its _task_new() function. > > > > It is a counter per channel, so it's better left here. > > > > > > > > > + GString *qmp_data; > > > > + JsonParser *qmp_parser; > > > > + GHashTable *qmp_tasks; > > > > +}; > > > > + > > > > +struct _SpiceQmpPort > > > > +{ > > > > + GObject parent; > > > > + > > > > + SpiceQmpPortPrivate *priv; > > > > +}; > > > > + > > > > +struct _SpiceQmpPortClass > > > > +{ > > > > + GObjectClass parent_class; > > > > +}; > > > > + > > > > +enum { > > > > + PROP_CHANNEL = 1, > > > > + PROP_READY, > > > > + > > > > + PROP_LAST, > > > > +}; > > > > + > > > > +enum { > > > > + SIGNAL_EVENT, > > > > + > > > > + SIGNAL_LAST, > > > > +}; > > > > + > > > > +static guint signals[SIGNAL_LAST]; > > > > +static GParamSpec *props[PROP_LAST] = { NULL, }; > > > > + > > > > +G_DEFINE_TYPE_WITH_PRIVATE(SpiceQmpPort, spice_qmp_port, G_TYPE_OBJECT) > > > > + > > > > +G_DEFINE_BOXED_TYPE(SpiceQmpStatus, spice_qmp_status, spice_qmp_status_ref, spice_qmp_status_unref) > > > > + > > > > +typedef void (QMPCb)(GTask *task, JsonNode *node); > > > > + > > > > +static void > > > > +qmp_error_return(GTask *task, const gchar *desc) > > > > +{ > > > > + g_task_return_new_error(task, SPICE_CLIENT_ERROR, > > > > + SPICE_CLIENT_ERROR_FAILED, "%s", desc); > > > > + g_object_unref(task); > > > > +} > > > > + > > > > +static void > > > > +spice_qmp_dispatch_message(SpiceQmpPort *self) > > > > +{ > > > > + JsonObject *obj = json_node_get_object(json_parser_get_root(self->priv->qmp_parser)); > > > > + JsonNode *node; > > > > + GTask *task; > > > > + const gchar *event; > > > > > > More like suggestions than anything else. > > > > > > > + > > > > + if (!self->priv->ready && json_object_get_member(obj, "QMP")) { > > > > > > maybe: > > > if (json_object_get_member(obj, "QMP") != NULL) { > > > g_warn_if_fail(!self->priv->ready); > > > > ok > > > > > > > > > + g_debug("QMP greeting received"); > > > > + } else if ((node = json_object_get_member(obj, "error"))) { > > > > + gint id = json_object_get_int_member(obj, "id"); > > > > + const gchar *desc = json_object_get_string_member(obj, "desc"); > > > > + > > > > + g_debug("QMP return error: %s, id:%d", desc, id); > > > > + if ((task = g_hash_table_lookup(self->priv->qmp_tasks, GINT_TO_POINTER(id)))) { > > > > + g_hash_table_steal(self->priv->qmp_tasks, GINT_TO_POINTER(id)); > > > > + qmp_error_return(task, desc); > > > > + } > > > > > > Should warn if task not found? > > > > ok > > > > > > > > > + } else if ((node = json_object_get_member(obj, "return"))) { > > > > + gint id = json_object_get_int_member(obj, "id"); > > > > + > > > > + g_debug("QMP return id:%d", id); > > > > + if (!self->priv->ready && id == 0) { > > > > + self->priv->ready = TRUE; > > > > + g_object_notify(G_OBJECT(self), "ready"); > > > > + } > > > > + > > > > + if (!self->priv->ready) { > > > > + g_warning("Invalid QMP return, not ready"); > > > > + } else if ((task = g_hash_table_lookup(self->priv->qmp_tasks, GINT_TO_POINTER(id)))) { > > > > + QMPCb *cb = g_task_get_task_data(task); > > > > + g_hash_table_steal(self->priv->qmp_tasks, GINT_TO_POINTER(id)); > > > > + cb(task, node); > > > > + } else { > > > > + g_debug("No task associated with return"); > > > > > > I did not see this happening so I wonder if it shoudn't be a > > > warning instead? > > > > ok > > > > > > > > > + } > > > > + } else if ((event = json_object_get_string_member(obj, "event"))) { > > > > + g_debug("QMP event %s", event); > > > > + g_signal_emit(G_OBJECT(self), signals[SIGNAL_EVENT], 0, event, > > > > + json_object_get_member(obj, "data")); > > > > + } else { > > > > + g_debug("unhandled JSON %s", self->priv->qmp_data->str); > > > > > > If caller, spice_qmp_data, can be dealing with several messages > > > (not sure yet, see my query on "\r\n"), maybe it is better to > > > return false and let the caller print the unhandled message (that > > > would be only part of, not all qmp_data->str) > > > > The caller stops at message boundaries: *crlf = '\0'; so one message > > is processed. > > But we print from qmp_data->str instead of current str position > in spice_qmp_data(), so, if we have 3 qmp messages being read in > that while(), and the second message is unhandled above, we would > print the first message, if got it right. > > My suggestion to return false and let spice_qmp_data() print the > current str message that was not handled in that case. Oh I see, my bad, done > > > > > > > > > > + } > > > > +} > > > > + > > > > +#define QMP_MAX_RESPONSE (10 * 1024 * 1024) > > > > + > > > > +static void > > > > +spice_qmp_data(SpiceQmpPort *self, gpointer data, > > > > > > It is a callback for 'port-data' so I'd rename it to > > > spice_qmp_handle_port_data or something like that. > > > > ok > > > > > > > > > + int size G_GNUC_UNUSED, > > > > + SpicePortChannel *port G_GNUC_UNUSED) > > > > +{ > > > > + GString *qmp = self->priv->qmp_data; > > > > + gchar *str, *crlf; > > > > + > > > > + g_string_append_len(qmp, data, size); > > > > + if (qmp->len > QMP_MAX_RESPONSE) { > > > > + g_warning("QMP response is too large, over %d bytes, truncating", > > > > + QMP_MAX_RESPONSE); > > > > + g_string_set_size(qmp, 0); > > > > + return; > > > > + } > > > > + > > > > + str = qmp->str; > > > > + while ((crlf = memmem(str, qmp->len - (str - qmp->str), "\r\n", 2))) { > > > > > > I take that "\r\n" should be the end of a valid qmp message? Does > > > the while help recover in any situation? > > > > We can be receiving mulitple messages in the same read(). So > > while is necessary. > > Right > > > > > > > > > > + GError *err = NULL; > > > > + > > > > + *crlf = '\0'; > > > > + json_parser_load_from_data(self->priv->qmp_parser, str, crlf - str, &err); > > > > + if (err) { > > > > + g_warning("JSON parsing error: %s", err->message); > > > > + g_error_free(err); > > > > + } else { > > > > + spice_qmp_dispatch_message(self); > > > > + } > > > > + str = crlf + 2; > > > > + } > > > > + > > > > + g_string_erase(qmp, 0, str - qmp->str); > > > > +} > > > > + > > > > +static void qmp_task_cancelled_cb(gpointer data) > > > > +{ > > > > + qmp_error_return(G_TASK(data), "Task got cancelled"); > > > > +} > > > > + > > > > +static void spice_qmp_port_init(SpiceQmpPort *self) > > > > +{ > > > > + self->priv = spice_qmp_port_get_instance_private(self); > > > > + self->priv->qmp_data = g_string_sized_new(256); > > > > + self->priv->qmp_parser = json_parser_new(); > > > > + self->priv->qmp_tasks = g_hash_table_new_full(g_direct_hash, g_direct_equal, > > > > + NULL, qmp_task_cancelled_cb); > > > > > > More like task is terminated as this is only called when > > > SpiceQmpPort is on dispose() > > > > so we cancel the pending tasks, not a big difference to make here. Or > > do you have a better suggestion? > > I don't see a strong reason to cancel the pending tasks unless > that could make a different around spice_port_channel_write_async > - my comment was more about that we are not really cancelling. > The source object is being disposed of. Hmm, it's not cancelled as with GCancellable, but it's still cancelled from a caller point of view, as we won't get reply/return from it. I don't think we need to bother about the distinction here, but I'll change it to task "disposed" if that may help. > Not really important, it was a comment after looking into the > usage of qmp_tasks hashtable as every GTask is retrieved with > g_hash_table_steal(). > > > > > > > > +} > > > > + > > > > +static void spice_qmp_port_dispose(GObject *gobject) > > > > +{ > > > > + SpiceQmpPort *self = SPICE_QMP_PORT(gobject); > > > > + > > > > + g_string_free(self->priv->qmp_data, TRUE); > > > > + g_object_unref(self->priv->qmp_parser); > > > > + g_hash_table_unref(self->priv->qmp_tasks); > > > > + > > > > + if (G_OBJECT_CLASS(spice_qmp_port_parent_class)->dispose) > > > > + G_OBJECT_CLASS(spice_qmp_port_parent_class)->dispose(gobject); > > > > +} > > > > + > > > > +static void spice_qmp_port_constructed(GObject *gobject) > > > > +{ > > > > + SpiceQmpPort *self = SPICE_QMP_PORT(gobject); > > > > + > > > > + g_object_set_data_full(G_OBJECT(self->priv->channel), > > > > + "spice-qmp-port", self, g_object_unref); > > > > > > I think you should g_object_ref(self) here, no? > > > > Good question, I haven't thought about that much. I'll use a weak > > pointer to clear the property on dispose > > Not sure if I understood. My concern is having the SpiceQmpPort's > property (port channel) to unref it without holding a reference > to it like on spice_qmp_port_set_property() SpiceQmpPort will hold a strong reference on the PortChannel. To avoid cyclic references, PortChannel will have a weak reference to SpiceQmpPort. nstead of the set_data_full with unref (bug), or a weak_ref I suggested, I'll just set_data() on construct to set the pointer and unset it on dispose. > > > > > + spice_g_signal_connect_object(self->priv->channel, > > > > + "port-data", G_CALLBACK(spice_qmp_data), > > > > + self, G_CONNECT_SWAPPED); > > > > + > > > > + if (G_OBJECT_CLASS(spice_qmp_port_parent_class)->constructed) > > > > + G_OBJECT_CLASS(spice_qmp_port_parent_class)->constructed(gobject); > > > > +} > > > > + > > > > +static void > > > > +spice_qmp_port_set_property(GObject *object, > > > > + guint property_id, > > > > + const GValue *value, > > > > + GParamSpec *pspec) > > > > +{ > > > > + SpiceQmpPort *self = SPICE_QMP_PORT(object); > > > > + > > > > + switch (property_id) { > > > > + case PROP_CHANNEL: > > > > + g_clear_object(&self->priv->channel); > > > > + self->priv->channel = g_value_dup_object(value); > > > > + break; > > > > + > > > > + default: > > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec); > > > > + break; > > > > + } > > > > +} > > > > + > > > > +static void > > > > +spice_qmp_port_get_property(GObject *object, > > > > + guint property_id, > > > > + GValue *value, > > > > + GParamSpec *pspec) > > > > +{ > > > > + SpiceQmpPort *self = SPICE_QMP_PORT(object); > > > > + > > > > + switch (property_id) { > > > > + case PROP_CHANNEL: > > > > + g_value_set_object(value, self->priv->channel); > > > > + break; > > > > + > > > > + case PROP_READY: > > > > + g_value_set_boolean(value, self->priv->ready); > > > > + break; > > > > + > > > > + default: > > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec); > > > > + break; > > > > + } > > > > +} > > > > + > > > > +static void spice_qmp_port_class_init(SpiceQmpPortClass *klass) > > > > +{ > > > > + GObjectClass *gobject_class = G_OBJECT_CLASS (klass); > > > > + > > > > + gobject_class->dispose = spice_qmp_port_dispose; > > > > + gobject_class->get_property = spice_qmp_port_get_property; > > > > + gobject_class->set_property = spice_qmp_port_set_property; > > > > + gobject_class->constructed = spice_qmp_port_constructed; > > > > + > > > > + /** > > > > + * SpiceQmpPort::event: > > > > + * @self: the #SpiceQmpPort that emitted the signal > > > > + * @name: the QMP event name > > > > + * @node: the event data json-node, or NULL > > > > + * > > > > + * Event emitted whenever a QMP event is received. > > > > + * > > > > + * Since: 0.36 > > > > + */ > > > > + signals[SIGNAL_EVENT] = > > > > + g_signal_new("event", > > > > + G_OBJECT_CLASS_TYPE(gobject_class), > > > > + G_SIGNAL_RUN_FIRST, > > > > + 0, > > > > + NULL, NULL, NULL, > > > > + G_TYPE_NONE, > > > > + 2, G_TYPE_STRING, G_TYPE_POINTER); > > > > + > > > > + props[PROP_CHANNEL] = > > > > + g_param_spec_object("channel", > > > > + "Channel", > > > > + "Associated port channel", > > > > + SPICE_TYPE_PORT_CHANNEL, > > > > + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); > > > > + > > > > + props[PROP_READY] = > > > > + g_param_spec_boolean("ready", > > > > + "Ready", > > > > + "Whether the QMP port is ready", > > > > + FALSE, > > > > + G_PARAM_READABLE | G_PARAM_STATIC_STRINGS); > > > > + > > > > + g_object_class_install_properties(gobject_class, PROP_LAST, props); > > > > + } > > > > + > > > > +static void > > > > +qmp_empty_return_cb(GTask *task, JsonNode *node) > > > > > > G_GNUC_UNUSED on node > > > > ok > > > > > > > > > +{ > > > > + g_task_return_boolean(task, TRUE); > > > > + g_object_unref(task); > > > > +} > > > > + > > > > +static void > > > > +spice_qmp_port_write_finished(GObject *source_object, > > > > + GAsyncResult *res, > > > > + gpointer t) > > > > +{ > > > > + SpicePortChannel *port = SPICE_PORT_CHANNEL(source_object); > > > > + GTask *task = G_TASK(t); > > > > + SpiceQmpPort *self = g_task_get_source_object(task); > > > > + gint id = GPOINTER_TO_INT(g_object_get_data(G_OBJECT(task), "qmp-id")); > > > > + GError *err = NULL; > > > > + > > > > + spice_port_channel_write_finish(port, res, &err); > > > > + if (err) { > > > > + g_hash_table_steal(self->priv->qmp_tasks, GINT_TO_POINTER(id)); > > > > + qmp_error_return(task, err->message); > > > > + g_error_free(err); > > > > + } > > > > +} > > > > + > > > > +static void > > > > +qmp(SpiceQmpPort *self, GTask *task, > > > > + const char *cmd, const gchar *args) > > > > +{ > > > > + GString *str = g_string_sized_new(256); > > > > + gsize len; > > > > + gchar *data; > > > > + gint id = self->priv->id_counter; > > > > + > > > > + g_string_append_printf(str, "{ 'execute': '%s'", cmd); > > > > + if (args) > > > > + g_string_append_printf(str, ", 'arguments': { %s }", args); > > > > + g_string_append_printf(str, ", 'id': %d", id); > > > > > > Interesting, you can send an id and you should receive the same > > > value back? I did not find this in man qemu-qmp-ref > > > > https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l108 > > Cool, please add this link to commit log ok > > > > > + g_string_append(str, " }"); > > > > + > > > > + g_hash_table_insert(self->priv->qmp_tasks, GINT_TO_POINTER(id), task); > > > > > > Although args are always null in this code, thinking that we > > > might add new commands later on... Is there any length limit to > > > qmp protocol? If yes, we would need to check it here. > > > > No strict limit afaik (but qemu has a limit of 20mb iirc internally) > > ok > > > > > > > + > > > > + len = str->len; > > > > + data = g_string_free(str, FALSE); > > > > + spice_port_channel_write_async(self->priv->channel, data, len, > > > > + g_task_get_cancellable(task), > > > > + spice_qmp_port_write_finished, task); > > > > + g_object_set_data_full(G_OBJECT(task), "qmp-data", data, g_free); > > > > + g_object_set_data(G_OBJECT(task), "qmp-id", GINT_TO_POINTER(id)); > > > > + > > > > + self->priv->id_counter++; > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_port_vm_action_finish: > > > > + * @self: a qmp port helper > > > > + * @result: The async #GAsyncResult result > > > > + * @error: a #GError pointer, or %NULL > > > > + * > > > > + * Finishes asynchronous VM action and returns the result. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +gboolean spice_qmp_port_vm_action_finish(SpiceQmpPort *self, > > > > + GAsyncResult *result, > > > > + GError **error) > > > > +{ > > > > + g_return_val_if_fail(SPICE_IS_QMP_PORT(self), FALSE); > > > > + g_return_val_if_fail(g_task_is_valid(result, self), FALSE); > > > > + > > > > + return g_task_propagate_boolean(G_TASK(result), error); > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_port_vm_action_async: > > > > + * @self: a qmp port helper > > > > + * @action: a VM action > > > > + * @cancellable: a #GCancellable, or %NULL > > > > + * @callback: callback to call when the action is complete > > > > + * @user_data: the data to pass to the callback function > > > > + * > > > > + * Request the VM to perform an action. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +void spice_qmp_port_vm_action_async(SpiceQmpPort *self, > > > > + SpiceQmpPortVmAction action, > > > > + GCancellable *cancellable, > > > > + GAsyncReadyCallback callback, > > > > + gpointer user_data) > > > > +{ > > > > + GTask *task; > > > > + const gchar *cmd; > > > > + > > > > + g_return_if_fail(SPICE_IS_QMP_PORT(self)); > > > > + g_return_if_fail(cancellable == NULL || G_IS_CANCELLABLE(cancellable)); > > > > + g_return_if_fail(self->priv->ready); > > > > + g_return_if_fail(action >= 0 && action < SPICE_QMP_PORT_VM_ACTION_LAST); > > > > + > > > > + task = g_task_new(self, cancellable, callback, user_data); > > > > + g_task_set_task_data(task, qmp_empty_return_cb, NULL); > > > > > > g_task_set_check_cancellable() or add > > > g_task_return_error_if_cancelled() on some of those callbacks > > > > We don't do heavy / long task in the helper itself, and the > > cancellable is passed down spice_port_channel_write(). > > True. > > > That wouldn't give us much to add check_cancellable(). Where > > would you add it? > > My initial thought was that, if there is ongoing request that is > taking too long, client might timeout/cancel the request. > > Not sure what should be done in qmp-port anyway, e.g: cancel a > shutdown request that was already sent :) ohoh troubles.., fwiw there is not really concurrent requests support in qmp (or libvirt) at the moment. > > > > > + switch (action) { > > > > + case SPICE_QMP_PORT_VM_ACTION_QUIT: > > > > + cmd = "quit"; > > > > + break; > > > > + case SPICE_QMP_PORT_VM_ACTION_RESET: > > > > + cmd = "system_reset"; > > > > + break; > > > > + case SPICE_QMP_PORT_VM_ACTION_POWER_DOWN: > > > > + cmd = "system_powerdown"; > > > > + break; > > > > + case SPICE_QMP_PORT_VM_ACTION_PAUSE: > > > > + cmd = "stop"; > > > > + break; > > > > + case SPICE_QMP_PORT_VM_ACTION_CONTINUE: > > > > + cmd = "cont"; > > > > + break; > > > > + default: > > > > + g_return_if_reached(); > > > > + } > > > > + > > > > + qmp(self, task, cmd, NULL); > > > > +} > > > > + > > > > +static void > > > > +qmp_capabilities_cb(GTask *task, JsonNode *node) > > > > +{ > > > > + g_task_return_boolean(task, TRUE); > > > > + g_object_unref(task); > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_port_get: > > > > + * @channel: the QMP port channel > > > > + * > > > > + * Associate a QMP port helper to the given port channel. If there is > > > > + * already a helper associated with the channel, it is simply returned. > > > > + * > > > > + * Returns: (transfer none): a weak reference to the associated SpiceQmpPort > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +SpiceQmpPort *spice_qmp_port_get(SpicePortChannel *channel) > > > > +{ > > > > + GObject *self; > > > > + > > > > + g_return_val_if_fail(SPICE_IS_PORT_CHANNEL(channel), NULL); > > > > + > > > > + self = g_object_get_data(G_OBJECT(channel), "spice-qmp-port"); > > > > + > > > > + if (self == NULL) { > > > > + GTask *task; > > > > + > > > > + self = g_object_new(SPICE_TYPE_QMP_PORT, "channel", channel, NULL); > > > > + task = g_task_new(self, NULL, NULL, NULL); > > > > + g_task_set_task_data(task, qmp_capabilities_cb, NULL); > > > > + qmp(SPICE_QMP_PORT(self), task, "qmp_capabilities", NULL); > > > > + } > > > > + > > > > + return SPICE_QMP_PORT(self); > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_status_ref: > > > > + * @status: a #SpiceQmpStatus > > > > + * > > > > + * References a @status. > > > > + * > > > > + * Returns: The same @status > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +SpiceQmpStatus * > > > > +spice_qmp_status_ref(SpiceQmpStatus *status) > > > > +{ > > > > + g_return_val_if_fail(status != NULL, NULL); > > > > + > > > > + status->ref++; > > > > + > > > > + return status; > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_status_unref: > > > > + * @status: a #SpiceQmpStatus > > > > + * > > > > + * Removes a reference from the given @status. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +void spice_qmp_status_unref(SpiceQmpStatus *status) > > > > +{ > > > > + if (status && status->ref-- == 0) { > > > > + g_free(status->status); > > > > + g_free(status); > > > > + } > > > > +} > > > > + > > > > +static void > > > > +qmp_query_status_return_cb(GTask *task, JsonNode *node) > > > > +{ > > > > + SpiceQmpStatus *status = g_new0(SpiceQmpStatus, 1); > > > > + JsonObject *obj = json_node_get_object(node); > > > > + > > > > + status->version = 1; > > > > + status->ref = 1; > > > > + status->running = json_object_get_boolean_member(obj, "running"); > > > > + status->singlestep = json_object_get_boolean_member(obj, "singlestep"); > > > > + status->status = g_strdup(json_object_get_string_member(obj, "status")); > > > > + > > > > + g_task_return_pointer(task, status, (GDestroyNotify)spice_qmp_status_unref); > > > > + g_object_unref(task); > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_port_query_status_async: > > > > + * @self: A #SpiceQmpPort > > > > + * @cancellable: A #GCancellable > > > > + * @callback: The async callback. > > > > + * @user_data: The async callback user data. > > > > + * > > > > + * Query the run status of all VCPUs. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +void spice_qmp_port_query_status_async(SpiceQmpPort *self, > > > > + GCancellable *cancellable, > > > > + GAsyncReadyCallback callback, > > > > + gpointer user_data) > > > > +{ > > > > + GTask *task; > > > > + > > > > + g_return_if_fail(SPICE_IS_QMP_PORT(self)); > > > > + g_return_if_fail(cancellable == NULL || G_IS_CANCELLABLE(cancellable)); > > > > + g_return_if_fail(self->priv->ready); > > > > + > > > > + task = g_task_new(self, cancellable, callback, user_data); > > > > + g_task_set_task_data(task, qmp_query_status_return_cb, NULL); > > > > + > > > > + qmp(self, task, "query-status", NULL); > > > > +} > > > > + > > > > +/** > > > > + * spice_qmp_port_query_status_finish: > > > > + * @self: A #SpiceQmpPort > > > > + * @result: The async #GAsyncResult result > > > > + * @error: a #GError pointer, or %NULL > > > > + * > > > > + * Finish the asynchronous status query. > > > > + * > > > > + * Returns: The #SpiceQmpStatus result or %NULL, in which case @error > > > > + * will be set. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +SpiceQmpStatus * > > > > +spice_qmp_port_query_status_finish(SpiceQmpPort *self, > > > > + GAsyncResult *result, > > > > + GError **error) > > > > +{ > > > > + g_return_val_if_fail(SPICE_IS_QMP_PORT(self), NULL); > > > > + g_return_val_if_fail(g_task_is_valid(result, self), NULL); > > > > + > > > > + return g_task_propagate_pointer(G_TASK(result), error); > > > > +} > > > > diff --git a/src/qmp-port.h b/src/qmp-port.h > > > > new file mode 100644 > > > > index 0000000..a8a4e30 > > > > --- /dev/null > > > > +++ b/src/qmp-port.h > > > > @@ -0,0 +1,122 @@ > > > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */ > > > > +/* > > > > + Copyright (C) 2018 Red Hat, Inc. > > > > + > > > > + This library is free software; you can redistribute it and/or > > > > + modify it under the terms of the GNU Lesser General Public > > > > + License as published by the Free Software Foundation; either > > > > + version 2.1 of the License, or (at your option) any later version. > > > > + > > > > + This library is distributed in the hope that it will be useful, > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > + Lesser General Public License for more details. > > > > + > > > > + You should have received a copy of the GNU Lesser General Public > > > > + License along with this library; if not, see <http://www.gnu.org/licenses/>. > > > > +*/ > > > > +#ifndef QMP_PORT_H_ > > > > +#define QMP_PORT_H_ > > > > + > > > > +#if !defined(__SPICE_CLIENT_H_INSIDE__) && !defined(SPICE_COMPILATION) > > > > +#warning "Only <spice-client.h> can be included directly" > > > > +#endif > > > > + > > > > +#include <glib-object.h> > > > > +#include "channel-port.h" > > > > + > > > > +G_BEGIN_DECLS > > > > + > > > > +#define SPICE_TYPE_QMP_PORT (spice_qmp_port_get_type ()) > > > > +#define SPICE_QMP_PORT(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), SPICE_TYPE_QMP_PORT, SpiceQmpPort)) > > > > +#define SPICE_QMP_PORT_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), SPICE_TYPE_QMP_PORT, SpiceQmpPortClass)) > > > > +#define SPICE_IS_QMP_PORT(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), SPICE_TYPE_QMP_PORT)) > > > > +#define SPICE_IS_QMP_PORT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), SPICE_TYPE_QMP_PORT)) > > > > +#define SPICE_QMP_PORT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), SPICE_TYPE_QMP_PORT, SpiceQmpPortClass)) > > > > + > > > > +/** > > > > + * SpiceQmpPort: > > > > + * > > > > + * Opaque data structure. > > > > + * Since: 0.36 > > > > + */ > > > > +typedef struct _SpiceQmpPort SpiceQmpPort; > > > > +typedef struct _SpiceQmpPortClass SpiceQmpPortClass; > > > > + > > > > +/** > > > > + * SpiceQmpPortVmAction: > > > > + * @SPICE_QMP_PORT_VM_ACTION_QUIT: This command will cause the VM process to exit gracefully. > > > > + * @SPICE_QMP_PORT_VM_ACTION_RESET: Performs a hard reset of the VM. > > > > + * @SPICE_QMP_PORT_VM_ACTION_POWER_DOWN: Performs a power down operation. > > > > + * @SPICE_QMP_PORT_VM_ACTION_PAUSE: Stop all VCPU execution. > > > > + * @SPICE_QMP_PORT_VM_ACTION_CONTINUE: Resume all VCPU execution. > > > > + * @SPICE_QMP_PORT_VM_ACTION_LAST: the last enum value. > > > > + * > > > > + * An action to perform on the VM. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +typedef enum SpiceQmpPortVmAction { > > > > + SPICE_QMP_PORT_VM_ACTION_QUIT, > > > > + SPICE_QMP_PORT_VM_ACTION_RESET, > > > > + SPICE_QMP_PORT_VM_ACTION_POWER_DOWN, > > > > + SPICE_QMP_PORT_VM_ACTION_PAUSE, > > > > + SPICE_QMP_PORT_VM_ACTION_CONTINUE, > > > > + > > > > + SPICE_QMP_PORT_VM_ACTION_LAST, > > > > +} SpiceQmpPortVmAction; > > > > + > > > > +/** > > > > + * SpiceQmpStatus: > > > > + * @version: the structure version > > > > + * @running: true if all VCPUs are runnable, false if not runnable > > > > + * @singlestep: true if VCPUs are in single-step mode > > > > + * @status: the virtual machine run state > > > > + * > > > > + * Information about VCPU run state. > > > > + * > > > > + * Since: 0.36 > > > > + **/ > > > > +typedef struct _SpiceQmpStatus { > > > > + /*< private >*/ > > > > + gint ref; > > > > > > g_rc_box_new0() would work well.. but too fresh for us, I guess. > > > > yes > > > > > > > > > + > > > > + /*< public >*/ > > > > + gint version; > > > > + > > > > + gboolean running; > > > > + gboolean singlestep; > > > > + gchar *status; > > > > +} SpiceQmpStatus; > > > > + > > > > +GType spice_qmp_port_get_type(void); > > > > + > > > > +SpiceQmpPort *spice_qmp_port_get(SpicePortChannel *channel); > > > > + > > > > +void spice_qmp_port_vm_action_async(SpiceQmpPort *self, > > > > + SpiceQmpPortVmAction action, > > > > + GCancellable *cancellable, > > > > + GAsyncReadyCallback callback, > > > > + gpointer user_data); > > > > + > > > > +gboolean spice_qmp_port_vm_action_finish(SpiceQmpPort *self, > > > > + GAsyncResult *result, > > > > + GError **error); > > > > + > > > > +GType spice_qmp_status_get_type(void); > > > > + > > > > +SpiceQmpStatus *spice_qmp_status_ref(SpiceQmpStatus *status); > > > > +void spice_qmp_status_unref(SpiceQmpStatus *status); > > > > + > > > > +void spice_qmp_port_query_status_async(SpiceQmpPort *self, > > > > + GCancellable *cancellable, > > > > + GAsyncReadyCallback callback, > > > > + gpointer user_data); > > > > + > > > > +SpiceQmpStatus *spice_qmp_port_query_status_finish(SpiceQmpPort *self, > > > > + GAsyncResult *result, > > > > + GError **error); > > > > + > > > > +G_END_DECLS > > > > + > > > > +#endif /* QMP_PORT_H_ */ > > > > diff --git a/src/spice-client.h b/src/spice-client.h > > > > index 32b79ea..77362b9 100644 > > > > --- a/src/spice-client.h > > > > +++ b/src/spice-client.h > > > > @@ -51,6 +51,7 @@ > > > > #include "usb-device-manager.h" > > > > #include "spice-audio.h" > > > > #include "spice-file-transfer-task.h" > > > > +#include "qmp-port.h" > > > > > > > > G_BEGIN_DECLS > > > > > > > > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file > > > > index b19844c..2df1cc0 100644 > > > > --- a/src/spice-glib-sym-file > > > > +++ b/src/spice-glib-sym-file > > > > @@ -96,6 +96,15 @@ spice_port_channel_write_finish > > > > spice_port_event > > > > spice_port_write_async > > > > spice_port_write_finish > > > > +spice_qmp_port_get > > > > +spice_qmp_port_get_type > > > > +spice_qmp_port_query_status_async > > > > +spice_qmp_port_query_status_finish > > > > +spice_qmp_port_vm_action_async > > > > +spice_qmp_port_vm_action_finish > > > > +spice_qmp_status_get_type > > > > +spice_qmp_status_ref > > > > +spice_qmp_status_unref > > > > spice_record_channel_get_type > > > > spice_record_channel_send_data > > > > spice_record_send_data > > > > -- > > > > 2.18.0.547.g1d89318c48 > > > > > > > > _______________________________________________ > > > > Spice-devel mailing list > > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > _______________________________________________ > > > Spice-devel mailing list > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > thanks for the review > > > > -- > > Marc-André Lureau > > Cheers, > Victor I am sending v2, thanks -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel