Re: [Qemu-devel] [PATCH 5/9] spice: core bits

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/19/2010 07:40 AM, Gerd Hoffmann wrote:
> Add -spice command line switch.  Has support setting passwd and port for
> now.  With this patch applied the spice client can successfully connect
> to qemu.  You can't do anything useful yet though.
>
> Signed-off-by: Gerd Hoffmann<kraxel@redhat.com>
> ---
>   Makefile.objs   |    2 +
>   qemu-config.c   |   23 ++++++++
>   qemu-config.h   |    1 +
>   qemu-options.hx |    8 +++
>   qemu-spice.h    |   22 ++++++++
>   spice.c         |  151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   vl.c            |   15 ++++++
>   7 files changed, 222 insertions(+), 0 deletions(-)
>   create mode 100644 qemu-spice.h
>   create mode 100644 spice.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index edfca87..021067b 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -88,6 +88,8 @@ common-obj-y += pflib.o
>   common-obj-$(CONFIG_BRLAPI) += baum.o
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
>
> +common-obj-$(CONFIG_SPICE) += spice.o
> +
>   audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>   audio-obj-$(CONFIG_SDL) += sdlaudio.o
>   audio-obj-$(CONFIG_OSS) += ossaudio.o
> diff --git a/qemu-config.c b/qemu-config.c
> index 95abe61..4cf3b53 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -342,6 +342,26 @@ QemuOptsList qemu_cpudef_opts = {
>       },
>   };
>
> +#ifdef CONFIG_SPICE
>    

Now's probably a good time to make vm_config_groups a list that can be 
dynamically added to so that you can register the option group from 
within spice.c.

This would require a new qemu_opts_parse() that took a name of an 
QemuOptsList instead of the pointer to it but then you can return 
failure if spice is unsupported.

The result would be that we wouldn't need an #ifdef here, -spice would 
always be a command line option, but if we didn't support spice, we'd 
return an error if we tried to use it.

> diff --git a/qemu-config.h b/qemu-config.h
> index dca69d4..3a90213 100644
> --- a/qemu-config.h
> +++ b/qemu-config.h
> @@ -14,6 +14,7 @@ extern QemuOptsList qemu_rtc_opts;
>   extern QemuOptsList qemu_global_opts;
>   extern QemuOptsList qemu_mon_opts;
>   extern QemuOptsList qemu_cpudef_opts;
> +extern QemuOptsList qemu_spice_opts;
>
>   QemuOptsList *qemu_find_opts(const char *group);
>   int qemu_set_option(const char *str);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index db86feb..c05c219 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -674,6 +674,14 @@ STEXI
>   Enable SDL.
>   ETEXI
>
> +#ifdef CONFIG_SPICE
> +DEF("spice", HAS_ARG, QEMU_OPTION_spice,
> +    "-spice<args>    use spice\n", QEMU_ARCH_ALL)
> +STEXI
> +Use Spice.
> +ETEXI
> +#endif
> +
>    

As mentioned above, I prefer run time failure instead of build time 
failure.  One reason is that we build documentation based on this file 
and it's awkward to have different documentation based on what's 
available in your build.

For instance, if I don't have libspice installed and I regenerate the 
doc on qemu.org, no one will be able to find out about spice from the help.

BTW, you could have better documentation than 'Use Spice.' :-)

>   DEF("portrait", 0, QEMU_OPTION_portrait,
>       "-portrait       rotate graphical output 90 deg left (only PXA LCD)\n",
>       QEMU_ARCH_ALL)
> diff --git a/qemu-spice.h b/qemu-spice.h
> new file mode 100644
> index 0000000..5597576
> --- /dev/null
> +++ b/qemu-spice.h
> @@ -0,0 +1,22 @@
> +#ifndef QEMU_SPICE_H
> +#define QEMU_SPICE_H
> +
> +#ifdef CONFIG_SPICE
> +
> +#include<spice.h>
> +
> +#include "qemu-option.h"
> +#include "qemu-config.h"
> +
> +extern SpiceServer *spice_server;
> +extern int using_spice;
> +
> +void qemu_spice_init(void);
>    

Could you avoid this with a module_init()?

Please make sure to have copyright/licenses in all files too.

> +
> +#else  /* CONFIG_SPICE */
> +
> +#define using_spice 0
>    

Please make this at least a macro.

> +#endif /* CONFIG_SPICE */
> +
> +#endif /* QEMU_SPICE_H */
> diff --git a/spice.c b/spice.c
> new file mode 100644
> index 0000000..50fa5ca
> --- /dev/null
> +++ b/spice.c
> @@ -0,0 +1,151 @@
>    

Copyrights.

> +#include<stdlib.h>
> +#include<stdio.h>
> +#include<string.h>
>    

This all comes in qemu-common.h

> +#include<spice.h>
> +#include<spice-experimental.h>
> +
> +#include "qemu-common.h"
> +#include "qemu-spice.h"
> +#include "qemu-timer.h"
> +#include "qemu-queue.h"
> +#include "monitor.h"
> +
> +/* core bits */
> +
> +SpiceServer *spice_server;
> +int using_spice = 0;
> +
> +struct SpiceTimer {
> +    QEMUTimer *timer;
> +    QTAILQ_ENTRY(SpiceTimer) next;
> +};
> +static QTAILQ_HEAD(, SpiceTimer) timers = QTAILQ_HEAD_INITIALIZER(timers);
> +
> +static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
> +{
> +    SpiceTimer *timer;
> +
> +    timer = qemu_mallocz(sizeof(*timer));
> +    timer->timer = qemu_new_timer(rt_clock, func, opaque);
> +    QTAILQ_INSERT_TAIL(&timers, timer, next);
> +    return timer;
> +}
> +
> +static void timer_start(SpiceTimer *timer, uint32_t ms)
> +{
> +    qemu_mod_timer(timer->timer, qemu_get_clock(rt_clock) + ms);
> +}
> +
> +static void timer_cancel(SpiceTimer *timer)
> +{
> +    qemu_del_timer(timer->timer);
> +}
> +
> +static void timer_remove(SpiceTimer *timer)
> +{
> +    qemu_del_timer(timer->timer);
> +    qemu_free_timer(timer->timer);
> +    QTAILQ_REMOVE(&timers, timer, next);
> +    free(timer);
> +}
>    
qemu_malloc mixed with free().

This can probably be a non-Spice function too.  I want to propose a new 
Timer wrapper that behaves much like what you have above.  There's a 
couple things I'd suggest to do though.  Let me post a quick patch as a 
follow up.

> +struct SpiceWatch {
> +    int fd;
> +    int event_mask;
> +    SpiceWatchFunc func;
> +    void *opaque;
> +    QTAILQ_ENTRY(SpiceWatch) next;
> +};
> +static QTAILQ_HEAD(, SpiceWatch) watches = QTAILQ_HEAD_INITIALIZER(watches);
> +
> +static void watch_read(void *opaque)
> +{
> +    SpiceWatch *watch = opaque;
> +    watch->func(watch->fd, SPICE_WATCH_EVENT_READ, watch->opaque);
> +}
> +
> +static void watch_write(void *opaque)
> +{
> +    SpiceWatch *watch = opaque;
> +    watch->func(watch->fd, SPICE_WATCH_EVENT_WRITE, watch->opaque);
> +}
> +
> +static void watch_update_mask(SpiceWatch *watch, int event_mask)
> +{
> +    IOHandler *on_read = NULL;
> +    IOHandler *on_write = NULL;
> +
> +    watch->event_mask = event_mask;
> +    if (watch->event_mask&  SPICE_WATCH_EVENT_READ)
> +        on_read = watch_read;
> +    if (watch->event_mask&  SPICE_WATCH_EVENT_WRITE)
> +        on_read = watch_write;
> +    qemu_set_fd_handler(watch->fd, on_read, on_write, watch);
> +}
> +
> +static SpiceWatch *watch_add(int fd, int event_mask, SpiceWatchFunc func, void *opaque)
> +{
> +    SpiceWatch *watch;
> +
> +    watch = qemu_mallocz(sizeof(*watch));
> +    watch->fd     = fd;
> +    watch->func   = func;
> +    watch->opaque = opaque;
> +    QTAILQ_INSERT_TAIL(&watches, watch, next);
> +
> +    watch_update_mask(watch, event_mask);
> +    return watch;
> +}
> +
> +static void watch_remove(SpiceWatch *watch)
> +{
> +    watch_update_mask(watch, 0);
> +    QTAILQ_REMOVE(&watches, watch, next);
> +    qemu_free(watch);
> +}
>    

I understand the value of these wrappers but it should be common code IMHO.

> +static SpiceCoreInterface core_interface = {
> +    .base.type          = SPICE_INTERFACE_CORE,
> +    .base.description   = "qemu core services",
> +    .base.major_version = SPICE_INTERFACE_CORE_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_CORE_MINOR,
> +
> +    .timer_add          = timer_add,
> +    .timer_start        = timer_start,
> +    .timer_cancel       = timer_cancel,
> +    .timer_remove       = timer_remove,
> +
> +    .watch_add          = watch_add,
> +    .watch_update_mask  = watch_update_mask,
> +    .watch_remove       = watch_remove,
> +};
>    

Ah, the interface is dictated by libspice.  Okay, ignore the bit about 
common code.

> +/* functions for the rest of qemu */
> +
> +void qemu_spice_init(void)
> +{
> +    QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
> +    const char *password;
> +    int port;
>    

Perhaps qemu_spice_init() should be passed the option list instead of 
relying on the global here.

> +    if (!opts)
> +        return;
> +    port = qemu_opt_get_number(opts, "port", 0);
> +    if (!port)
> +        return;
> +    password = qemu_opt_get(opts, "password");
> +
> +    spice_server = spice_server_new();
> +    spice_server_set_port(spice_server, port);
> +    if (password)
> +        spice_server_set_ticket(spice_server, password, 0, 0, 0);
> +    if (qemu_opt_get_bool(opts, "disable-ticketing", 0))
> +        spice_server_set_noauth(spice_server);
> +
> +    /* TODO: make configurable via cmdline */
> +    spice_server_set_image_compression(spice_server, SPICE_IMAGE_COMPRESS_AUTO_GLZ);
> +
> +    spice_server_init(spice_server,&core_interface);
> +    using_spice = 1;
> +}
> diff --git a/vl.c b/vl.c
> index b3e3676..7f391c0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -161,6 +161,8 @@ int main(int argc, char **argv)
>   #include "cpus.h"
>   #include "arch_init.h"
>
> +#include "qemu-spice.h"
> +
>   //#define DEBUG_NET
>   //#define DEBUG_SLIRP
>
> @@ -2600,6 +2602,15 @@ int main(int argc, char **argv, char **envp)
>                       }
>                       break;
>                   }
> +#ifdef CONFIG_SPICE
> +            case QEMU_OPTION_spice:
> +                opts = qemu_opts_parse(&qemu_spice_opts, optarg, 0);
> +                if (!opts) {
> +                    fprintf(stderr, "parse error: %s\n", optarg);
> +                    exit(1);
> +                }
> +                break;
> +#endif
>               case QEMU_OPTION_writeconfig:
>                   {
>                       FILE *fp;
> @@ -2868,6 +2879,10 @@ int main(int argc, char **argv, char **envp)
>       }
>       qemu_add_globals();
>
> +#ifdef CONFIG_SPICE
> +    qemu_spice_init();
> +#endif
>    

I think there's two paths we can take here.  We can either use 
module_init() with a new class for this location or we can be more 
explicit and pass the option list here.

I think I'd be happy with either one.  In any event, I'd like to avoid 
an #ifdef here.

Regards,

Anthony Liguori