[Spice-devel] [PATCH spice-gtk] glib: add SpiceQmpPort helper
Marc-André Lureau
marcandre.lureau at gmail.com
Wed Sep 19 13:51:13 UTC 2018
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.
>
> > + }
> > +}
> > +
> > +#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.
>
> > + 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?
>
> > +}
> > +
> > +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
>
> > +
> > + 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
>
> > + 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)
> > +
> > + 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().
That wouldn't give us much to add check_cancellable(). Where would you add it?
> > +
> > + 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
More information about the Spice-devel
mailing list