Re: [Qemu-devel] [PATCH] spice: add chardev (v3)

[Anthony Liguori <anthony@codemonkey.ws>]

On 12/17/2010 07:39 AM, Alon Levy wrote:
> Adding a chardev backend for spice, for usage by spice vdagent in
> conjunction with a properly named virtio-serial device.
>
> Example usage:
>   qemu -device virtio-serial -chardev spicevmc,name=vdagent,id=vdagent -devic
>
> This is equivalent to the old:
>   qemu -device virtio-serial -device spicevmc,subtype=vdagent
>
> longer to write, but generated by libvirt, and requires one less device.
>
> v1->v3 changes: (v2 had a wrong commit message)
>   * removed spice-qemu-char.h, folded into ui/qemu-spice.h
>   * removed dead IOCTL code
>   * removed comment
>   * removed ifdef CONFIG_SPICE from qemu-config.c and qemu-options.hx help.
>    

What doe this channel do?

I really don't feel comfortable with this.  This is not connecting QEMU 
to an existing interface that happens to fit our model.

This is clearly a "library" that provides thin wrappers around internal 
QEMU interfaces to implement code that belongs in QEMU outside of QEMU.

It's essentially a static plugin.  It's the same problem with QXL.  I 
don't think we should be in the business of having thin shims to 
external libraries when the only reason to have the external library is 
to keep code out of QEMU.

Regards,

Anthony Liguori

> ---
>   Makefile.objs     |    2 +-
>   qemu-char.c       |    4 +
>   qemu-config.c     |    6 ++
>   qemu-options.hx   |   16 ++++-
>   spice-qemu-char.c |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ui/qemu-spice.h   |    3 +
>   6 files changed, 214 insertions(+), 2 deletions(-)
>   create mode 100644 spice-qemu-char.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index cebb945..320b2a9 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -102,7 +102,7 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
>   common-obj-$(CONFIG_WIN32) += version.o
>
> -common-obj-$(CONFIG_SPICE) += ui/spice-core.o ui/spice-input.o ui/spice-display.o
> +common-obj-$(CONFIG_SPICE) += ui/spice-core.o ui/spice-input.o ui/spice-display.o spice-qemu-char.o
>
>   audio-obj-y = audio.o noaudio.o wavaudio.o mixeng.o
>   audio-obj-$(CONFIG_SDL) += sdlaudio.o
> diff --git a/qemu-char.c b/qemu-char.c
> index edc9ad6..acc7130 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -97,6 +97,7 @@
>   #endif
>
>   #include "qemu_socket.h"
> +#include "ui/qemu-spice.h"
>
>   #define READ_BUF_LEN 4096
>
> @@ -2495,6 +2496,9 @@ static const struct {
>       || defined(__FreeBSD_kernel__)
>       { .name = "parport",   .open = qemu_chr_open_pp },
>   #endif
> +#ifdef CONFIG_SPICE
> +    { .name = "spicevmc",     .open = qemu_chr_open_spice },
> +#endif
>   };
>
>   CharDriverState *qemu_chr_open_opts(QemuOpts *opts,
> diff --git a/qemu-config.c b/qemu-config.c
> index 965fa46..323d3c2 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -146,6 +146,12 @@ static QemuOptsList qemu_chardev_opts = {
>           },{
>               .name = "signal",
>               .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "debug",
> +            .type = QEMU_OPT_NUMBER,
>           },
>           { /* end of list */ }
>       },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 4d99a58..5c13f0f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1357,6 +1357,9 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>   #if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__)
>       "-chardev parport,id=id,path=path[,mux=on|off]\n"
>   #endif
> +#if defined(CONFIG_SPICE)
> +    "-chardev spicevmc,id=id,debug=debug,name=name\n"
> +#endif
>       , QEMU_ARCH_ALL
>   )
>
> @@ -1381,7 +1384,8 @@ Backend is one of:
>   @option{stdio},
>   @option{braille},
>   @option{tty},
> -@option{parport}.
> +@option{parport}
> +@option{spicevmc}.
>   The specific backend will determine the applicable options.
>
>   All devices must have an id, which can be any string up to 127 characters long.
> @@ -1557,6 +1561,16 @@ Connect to a local parallel port.
>   @option{path} specifies the path to the parallel port device. @option{path} is
>   required.
>
> +#if defined(CONFIG_SPICE)
> +@item -chardev spicevmc ,id=@var{id} ,debug=@var{debug}, name=@var{name}
> +
> +@option{debug} debug level for spicevmc
> +
> +@option{name} name of spice channel to connect to
> +
> +Connect to a spice virtual machine channel, such as vdiport.
> +#endif
> +
>   @end table
>   ETEXI
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> new file mode 100644
> index 0000000..0ffa674
> --- /dev/null
> +++ b/spice-qemu-char.c
> @@ -0,0 +1,185 @@
> +#include "config-host.h"
> +#include "ui/qemu-spice.h"
> +#include<spice.h>
> +#include<spice-experimental.h>
> +
> +#include "osdep.h"
> +
> +#define dprintf(_scd, _level, _fmt, ...)                                \
> +    do {                                                                \
> +        static unsigned __dprintf_counter = 0;                          \
> +        if (_scd->debug>= _level) {                                    \
> +            fprintf(stderr, "scd: %3d: " _fmt, ++__dprintf_counter, ## __VA_ARGS__);\
> +        }                                                               \
> +    } while (0)
> +
> +#define VMC_MAX_HOST_WRITE    2048
> +
> +typedef struct SpiceCharDriver {
> +    CharDriverState*      chr;
> +    SpiceCharDeviceInstance     sin;
> +    char                  *subtype;
> +    bool                  active;
> +    uint8_t               *buffer;
> +    uint8_t               *datapos;
> +    ssize_t               bufsize, datalen;
> +    uint32_t              debug;
> +} SpiceCharDriver;
> +
> +static int vmc_write(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len)
> +{
> +    SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
> +    ssize_t out = 0;
> +    ssize_t last_out;
> +    uint8_t* p = (uint8_t*)buf;
> +
> +    while (len>  0) {
> +        last_out = MIN(len, VMC_MAX_HOST_WRITE);
> +        qemu_chr_read(scd->chr, p, last_out);
> +        if (last_out>  0) {
> +            out += last_out;
> +            len -= last_out;
> +            p += last_out;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    dprintf(scd, 3, "%s: %lu/%zd\n", __func__, out, len + out);
> +    return out;
> +}
> +
> +static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
> +{
> +    SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
> +    int bytes = MIN(len, scd->datalen);
> +
> +    dprintf(scd, 2, "%s: %p %d/%d/%zd\n", __func__, scd->datapos, len, bytes, scd->datalen);
> +    if (bytes>  0) {
> +        memcpy(buf, scd->datapos, bytes);
> +        scd->datapos += bytes;
> +        scd->datalen -= bytes;
> +        assert(scd->datalen>= 0);
> +        if (scd->datalen == 0) {
> +            scd->datapos = 0;
> +        }
> +    }
> +    return bytes;
> +}
> +
> +static SpiceCharDeviceInterface vmc_interface = {
> +    .base.type          = SPICE_INTERFACE_CHAR_DEVICE,
> +    .base.description   = "spice virtual channel char device",
> +    .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
> +    .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
> +    .write              = vmc_write,
> +    .read               = vmc_read,
> +};
> +
> +
> +static void vmc_register_interface(SpiceCharDriver *scd)
> +{
> +    if (scd->active) {
> +        return;
> +    }
> +    dprintf(scd, 1, "%s\n", __func__);
> +    scd->sin.base.sif =&vmc_interface.base;
> +    qemu_spice_add_interface(&scd->sin.base);
> +    scd->active = true;
> +}
> +
> +static void vmc_unregister_interface(SpiceCharDriver *scd)
> +{
> +    if (!scd->active) {
> +        return;
> +    }
> +    dprintf(scd, 1, "%s\n", __func__);
> +    spice_server_remove_interface(&scd->sin.base);
> +    scd->active = false;
> +}
> +
> +
> +static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    SpiceCharDriver *s = chr->opaque;
> +
> +    dprintf(s, 2, "%s: %d\n", __func__, len);
> +    vmc_register_interface(s);
> +    assert(s->datalen == 0);
> +    if (s->bufsize<  len) {
> +        s->bufsize = len;
> +        s->buffer = qemu_realloc(s->buffer, s->bufsize);
> +    }
> +    memcpy(s->buffer, buf, len);
> +    s->datapos = s->buffer;
> +    s->datalen = len;
> +    spice_server_char_device_wakeup(&s->sin);
> +    return len;
> +}
> +
> +static void spice_chr_close(struct CharDriverState *chr)
> +{
> +    SpiceCharDriver *s = chr->opaque;
> +
> +    printf("%s\n", __func__);
> +    vmc_unregister_interface(s);
> +    qemu_free(s);
> +}
> +
> +static void print_allowed_subtypes(void)
> +{
> +    const char** psubtype;
> +    int i;
> +
> +    fprintf(stderr, "allowed names: ");
> +    for(i=0, psubtype = spice_server_char_device_recognized_subtypes();
> +        *psubtype != NULL; ++psubtype, ++i) {
> +        if (i == 0) {
> +            fprintf(stderr, "%s", *psubtype);
> +        } else {
> +            fprintf(stderr, ", %s", *psubtype);
> +        }
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> +{
> +    CharDriverState *chr;
> +    SpiceCharDriver *s;
> +    const char* name = qemu_opt_get(opts, "name");
> +    uint32_t debug = qemu_opt_get_number(opts, "debug", 0);
> +    const char** psubtype = spice_server_char_device_recognized_subtypes();
> +    const char *subtype = NULL;
> +
> +    if (name == NULL) {
> +        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> +        print_allowed_subtypes();
> +        return NULL;
> +    }
> +    for(;*psubtype != NULL; ++psubtype) {
> +        if (strcmp(name, *psubtype) == 0) {
> +            subtype = *psubtype;
> +            break;
> +        }
> +    }
> +    if (subtype == NULL) {
> +        fprintf(stderr, "spice-qemu-char: unsupported name\n");
> +        print_allowed_subtypes();
> +        return NULL;
> +    }
> +
> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    s = qemu_mallocz(sizeof(SpiceCharDriver));
> +    s->chr = chr;
> +    s->debug = debug;
> +    s->active = false;
> +    s->sin.subtype = subtype;
> +    chr->opaque = s;
> +    chr->chr_write = spice_chr_write;
> +    chr->chr_close = spice_chr_close;
> +
> +    qemu_chr_generic_open(chr);
> +
> +    return chr;
> +}
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index 0e3ad9b..e06af17 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -24,6 +24,7 @@
>
>   #include "qemu-option.h"
>   #include "qemu-config.h"
> +#include "qemu-char.h"
>
>   extern int using_spice;
>
> @@ -33,6 +34,8 @@ void qemu_spice_audio_init(void);
>   void qemu_spice_display_init(DisplayState *ds);
>   int qemu_spice_add_interface(SpiceBaseInstance *sin);
>
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
> +
>   #else  /* CONFIG_SPICE */
>
>   #define using_spice 0
>