[Qemu-devel] Re: [RfC PATCH] spice: qmp windup: connection events & info command.

[Luiz Capitulino]

On Thu, 11 Nov 2010 13:49:32 +0100
Gerd Hoffmann <address@hidden> wrote:

>   Hi,
> 
> Looking for comments especially from Luiz and Daniel (aka qmp and
> libvirt masters) ...
> 
> This patch adds support for connection events to spice.  The events are
> quite simliar to the vnc events.  Unlike vnc spice uses multiple tcp
> channels though.  qemu will report every single tcp connection (aka
> spice channel).  If you want track spice sessions only you can filter
> for the main channel (channel-type == 1).
> 
> The patch also adds a 'query-spice' monitor command which returns
> informations about the spice server configuration and also a list of
> channel connections.

The first thing we have to check with Daniel is whether or not we're
providing the info they would need/expect, apart from that I have the
following general comments:

 1. It's missing documentation in QMP/qmp-events.txt and qmp-commands.hx
    (yeah, docs are far from code, hope to fix soon)
 2. Can you please split this in two patches? One adding the events and
    the other adding the query command

Some small comments follow.

> 
> cheers,
>   Gerd
> 
> Cc: address@hidden
> Cc: address@hidden
> ---
>  monitor.c       |   43 +++++++++++++++
>  monitor.h       |    3 +
>  ui/qemu-spice.h |    2 +
>  ui/spice-core.c |  160 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 208 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 8cee35d..4f18cbc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -59,6 +59,7 @@
>  #ifdef CONFIG_SIMPLE_TRACE
>  #include "trace.h"
>  #endif
> +#include "ui/qemu-spice.h"
>  
>  //#define DEBUG
>  //#define DEBUG_COMPLETION
> @@ -459,6 +460,15 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> *data)
>          case QEVENT_WATCHDOG:
>              event_name = "WATCHDOG";
>              break;
> +        case QEVENT_SPICE_CONNECTED:
> +            event_name = "SPICE_CONNECTED";
> +            break;
> +        case QEVENT_SPICE_INITIALIZED:
> +            event_name = "SPICE_INITIALIZED";
> +            break;
> +        case QEVENT_SPICE_DISCONNECTED:
> +            event_name = "SPICE_DISCONNECTED";
> +            break;
>          default:
>              abort();
>              break;
> @@ -640,6 +650,19 @@ static void user_async_info_handler(Monitor *mon, const 
> mon_cmd_t *cmd)
>      }
>  }
>  
> +/*
> + * generic print handler for hmp 'info $what'
> + * simply pretty-print the josn representation
> + */
> +#if defined(CONFIG_SPICE) /* because 'info spice' is the only user */
> +static void do_info_generic_print(Monitor *mon, const QObject *data)
> +{
> +    QString *json = qobject_to_json_pretty(data);
> +    monitor_printf(mon, "%s\n", qstring_get_str(json));
> +    QDECREF(json);
> +}
> +#endif

We definitely need a generic print handler, but I don't that stringifying
JSON makes a minimal good user interface.

>  static void do_info(Monitor *mon, const QDict *qdict)
>  {
>      const mon_cmd_t *cmd;
> @@ -2533,6 +2556,16 @@ static const mon_cmd_t info_cmds[] = {
>          .user_print = do_info_vnc_print,
>          .mhandler.info_new = do_info_vnc,
>      },
> +#if defined(CONFIG_SPICE)
> +    {
> +        .name       = "spice",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the spice server status",
> +        .user_print = do_info_generic_print,
> +        .mhandler.info_new = do_info_spice,
> +    },
> +#endif
>      {
>          .name       = "name",
>          .args_type  = "",
> @@ -2720,6 +2753,16 @@ static const mon_cmd_t qmp_query_cmds[] = {
>          .user_print = do_info_vnc_print,
>          .mhandler.info_new = do_info_vnc,
>      },
> +#if defined(CONFIG_SPICE)
> +    {
> +        .name       = "spice",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the spice server status",
> +        .user_print = do_info_generic_print,
> +        .mhandler.info_new = do_info_spice,
> +    },
> +#endif
>      {
>          .name       = "name",
>          .args_type  = "",
> diff --git a/monitor.h b/monitor.h
> index 2d36bba..4f2d328 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -32,6 +32,9 @@ typedef enum MonitorEvent {
>      QEVENT_BLOCK_IO_ERROR,
>      QEVENT_RTC_CHANGE,
>      QEVENT_WATCHDOG,
> +    QEVENT_SPICE_CONNECTED,
> +    QEVENT_SPICE_INITIALIZED,
> +    QEVENT_SPICE_DISCONNECTED,
>      QEVENT_MAX,
>  } MonitorEvent;
>  
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index 0e3ad9b..33c1e71 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -33,6 +33,8 @@ void qemu_spice_audio_init(void);
>  void qemu_spice_display_init(DisplayState *ds);
>  int qemu_spice_add_interface(SpiceBaseInstance *sin);
>  
> +void do_info_spice(Monitor *mon, QObject **ret_data);
> +
>  #else  /* CONFIG_SPICE */
>  
>  #define using_spice 0
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index b7fa031..d2c4a70 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -18,16 +18,24 @@
>  #include <spice.h>
>  #include <spice-experimental.h>
>  
> +#include <netdb.h>
> +
>  #include "qemu-common.h"
>  #include "qemu-spice.h"
>  #include "qemu-timer.h"
>  #include "qemu-queue.h"
>  #include "qemu-x509.h"
> +#include "qemu_socket.h"
> +#include "qint.h"
> +#include "qbool.h"
> +#include "qstring.h"
> +#include "qjson.h"
>  #include "monitor.h"
>  
>  /* core bits */
>  
>  static SpiceServer *spice_server;
> +static const char *auth = "spice";
>  int using_spice = 0;
>  
>  struct SpiceTimer {
> @@ -121,6 +129,118 @@ static void watch_remove(SpiceWatch *watch)
>      qemu_free(watch);
>  }
>  
> +#if SPICE_INTERFACE_CORE_MINOR >= 3
> +
> +typedef struct ChannelList ChannelList;
> +struct ChannelList {
> +    SpiceChannelEventInfo *info;
> +    QTAILQ_ENTRY(ChannelList) link;
> +};
> +static QTAILQ_HEAD(, ChannelList) channel_list = 
> QTAILQ_HEAD_INITIALIZER(channel_list);
> +
> +static void channel_list_add(SpiceChannelEventInfo *info)
> +{
> +    ChannelList *item;
> +
> +    item = qemu_mallocz(sizeof(*item));
> +    item->info = info;
> +    QTAILQ_INSERT_TAIL(&channel_list, item, link);
> +}
> +
> +static void channel_list_del(SpiceChannelEventInfo *info)
> +{
> +    ChannelList *item;
> +
> +    QTAILQ_FOREACH(item, &channel_list, link) {
> +        if (item->info != info) {
> +            continue;
> +        }
> +        QTAILQ_REMOVE(&channel_list, item, link);
> +        qemu_free(item);
> +        return;
> +    }
> +}
> +
> +static void add_addr_info(QDict *dict, struct sockaddr *addr, int len)
> +{
> +    char host[NI_MAXHOST], port[NI_MAXSERV];
> +    const char *family;
> +
> +    getnameinfo(addr, len, host, sizeof(host), port, sizeof(port),
> +                NI_NUMERICHOST | NI_NUMERICSERV);
> +    family = inet_strfamily(addr->sa_family);
> +
> +    qdict_put(dict, "host", qstring_from_str(host));
> +    qdict_put(dict, "port", qstring_from_str(port));
> +    qdict_put(dict, "family", qstring_from_str(family));
> +}
> +
> +static void add_channel_info(QDict *dict, SpiceChannelEventInfo *info)
> +{
> +    int tls = info->flags & SPICE_CHANNEL_EVENT_FLAG_TLS;
> +
> +    qdict_put(dict, "connection-id", qint_from_int(info->connection_id));
> +    qdict_put(dict, "channel-type", qint_from_int(info->type));
> +    qdict_put(dict, "channel-id", qint_from_int(info->id));
> +    qdict_put(dict, "tls", qbool_from_int(tls));
> +}
> +
> +static QList *channel_list_get(void)
> +{
> +    ChannelList *item;
> +    QList *list;
> +    QDict *dict;
> +
> +    list = qlist_new();
> +    QTAILQ_FOREACH(item, &channel_list, link) {
> +        dict = qdict_new();
> +        add_addr_info(dict, &item->info->paddr, item->info->plen);
> +        add_channel_info(dict, item->info);
> +        qlist_append_obj(list, QOBJECT(dict));

You can use qlist_append() and drop the QOBJECT() usage.

> +    }
> +    return list;
> +}
> +
> +static void channel_event(int event, SpiceChannelEventInfo *info)
> +{
> +    static const int qevent[] = {
> +        [ SPICE_CHANNEL_EVENT_CONNECTED    ] = QEVENT_SPICE_CONNECTED,
> +        [ SPICE_CHANNEL_EVENT_INITIALIZED  ] = QEVENT_SPICE_INITIALIZED,
> +        [ SPICE_CHANNEL_EVENT_DISCONNECTED ] = QEVENT_SPICE_DISCONNECTED,
> +    };
> +    QDict *server, *client;
> +    QObject *data;
> +
> +    client = qdict_new();
> +    add_addr_info(client, &info->paddr, info->plen);
> +
> +    server = qdict_new();
> +    add_addr_info(server, &info->laddr, info->llen);
> +
> +    if (event == SPICE_CHANNEL_EVENT_INITIALIZED) {
> +        qdict_put(server, "auth", qstring_from_str(auth));
> +        add_channel_info(client, info);
> +        channel_list_add(info);
> +    }
> +    if (event == SPICE_CHANNEL_EVENT_DISCONNECTED) {
> +        channel_list_del(info);
> +    }
> +
> +    data = qobject_from_jsonf("{ 'client': %p, 'server': %p }",
> +                              QOBJECT(client), QOBJECT(server));
> +    monitor_protocol_event(qevent[event], data);
> +    qobject_decref(data);
> +}
> +
> +#else /* SPICE_INTERFACE_CORE_MINOR >= 3 */
> +
> +static QList *channel_list_get(void)
> +{
> +    return NULL;
> +}
> +
> +#endif /* SPICE_INTERFACE_CORE_MINOR >= 3 */
> +
>  static SpiceCoreInterface core_interface = {
>      .base.type          = SPICE_INTERFACE_CORE,
>      .base.description   = "qemu core services",
> @@ -135,6 +255,10 @@ static SpiceCoreInterface core_interface = {
>      .watch_add          = watch_add,
>      .watch_update_mask  = watch_update_mask,
>      .watch_remove       = watch_remove,
> +
> +#if SPICE_INTERFACE_CORE_MINOR >= 3
> +    .channel_event      = channel_event,
> +#endif
>  };
>  
>  /* config string parsing */
> @@ -204,6 +328,41 @@ static const char *wan_compression_names[] = {
>  
>  /* functions for the rest of qemu */
>  
> +void do_info_spice(Monitor *mon, QObject **ret_data)
> +{
> +    QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> +    QDict *server;
> +    QList *clist;
> +    const char *addr;
> +    int port, tls_port;
> +
> +    if (!spice_server) {
> +        *ret_data = qobject_from_jsonf("{ 'enabled': false }");
> +        return;
> +    }
> +
> +    addr = qemu_opt_get(opts, "addr");
> +    port = qemu_opt_get_number(opts, "port", 0);
> +    tls_port = qemu_opt_get_number(opts, "tls-port", 0);
> +    clist = channel_list_get();
> +
> +    server = qdict_new();
> +    qdict_put(server, "enabled", qbool_from_int(true));
> +    qdict_put(server, "auth", qstring_from_str(auth));
> +    qdict_put(server, "host", qstring_from_str(addr ? addr : "0.0.0.0"));
> +    if (port) {
> +        qdict_put(server, "port", qint_from_int(port));
> +    }
> +    if (tls_port) {
> +        qdict_put(server, "tls-port", qint_from_int(tls_port));
> +    }
> +    if (clist) {
> +        qdict_put(server, "channels", clist);
> +    }
> +
> +    *ret_data = QOBJECT(server);
> +}
> +
>  static int add_channel(const char *name, const char *value, void *opaque)
>  {
>      int security = 0;
> @@ -316,6 +475,7 @@ void qemu_spice_init(void)
>          spice_server_set_ticket(spice_server, password, 0, 0, 0);
>      }
>      if (qemu_opt_get_bool(opts, "disable-ticketing", 0)) {
> +        auth = "none";
>          spice_server_set_noauth(spice_server);
>      }
>