[Spice-devel] [PATCH spice-gtk] glib: add SpiceQmpPort helper
Victor Toso
victortoso at redhat.com
Wed Sep 19 15:06:23 UTC 2018
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 at redhat.com> 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 at redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > 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 at redhat.com>
> > > ---
> > > 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.
>
> >
> > > + }
> > > +}
> > > +
> > > +#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.
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()
> > > + 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
> > > + 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 :)
> > > + 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> thanks for the review
>
> --
> Marc-André Lureau
Cheers,
Victor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180919/c844710d/attachment-0001.sig>
More information about the Spice-devel
mailing list