Re: [Qemu-devel] [PATCH 07/20] char: move callbacks in CharDriver

[Eric Blake]

On 01/05/2017 10:53 AM, Marc-André Lureau wrote:
> This makes the code more declarative, and avoids duplicating the
> information on all instances.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/sysemu/char.h |  46 ++---
>  backends/baum.c       |  11 +-
>  backends/msmouse.c    |  11 +-
>  backends/testdev.c    |   8 +-
>  gdbstub.c             |   7 +-
>  hw/bt/hci-csr.c       |   8 +-
>  qemu-char.c           | 489 
> +++++++++++++++++++++++++++++---------------------
>  spice-qemu-char.c     |  32 ++--
>  ui/console.c          |  28 +--
>  ui/gtk.c              |  11 +-
>  10 files changed, 381 insertions(+), 270 deletions(-)
> 

> +++ b/gdbstub.c
> @@ -1730,6 +1730,10 @@ int gdbserver_start(const char *device)
>      CharDriverState *chr = NULL;
>      CharDriverState *mon_chr;
>      ChardevCommon common = { 0 };
> +    static const CharDriver driver = {
> +        .kind = -1,
> +        .chr_write = gdb_monitor_write

Missing a trailing comma.

> @@ -2624,7 +2639,7 @@ static CharDriverState *qemu_chr_open_stdio(const char 
> *id,
>      int                is_console = 0;
>      ChardevCommon *common = qapi_ChardevStdio_base(backend->u.stdio.data);
>  
> -    chr   = qemu_chr_alloc(common, errp);
> +    chr   = qemu_chr_alloc(driver, common, errp);

Minor, but it looks like there's not much to align to anymore, and we
could trim it to one space while touching it.  But I don't care if it
stays the same, either, given the weird alignment on is_console a bit
earlier.

> +static const CharDriver ringbuf_driver = {
> +    .kind = CHARDEV_BACKEND_KIND_RINGBUF,
> +    .parse = qemu_chr_parse_ringbuf,
> +    .create = qemu_chr_open_ringbuf,
> +    .chr_write = ringbuf_chr_write,
> +    .chr_free = ringbuf_chr_free,
> +};
> +
> +/* Bug-compatibility: */
> +static const CharDriver memory_driver = {
> +    .kind = CHARDEV_BACKEND_KIND_MEMORY,
> +    .parse = qemu_chr_parse_ringbuf,
> +    .create = qemu_chr_open_ringbuf,
> +    .chr_write = ringbuf_chr_write,
> +    .chr_free = ringbuf_chr_free,
> +};

Again, depending on what we decide to do about the QAPI, we could delete
the 'memory' value from QMP and use .alias to still support it from the
command line, for just one driver instead of two here, if desired.  But
not a show-stopper if we don't make that decision before this patch, though.

> -#if defined HAVE_CHARDEV_SERIAL
> -        {
> -            .kind = CHARDEV_BACKEND_KIND_SERIAL,
> -            .alias = "tty",
> -            .parse = qemu_chr_parse_serial,
> -            .create = qmp_chardev_open_serial,
> -        },
> -        {
> -            .kind = CHARDEV_BACKEND_KIND_SERIAL,
> -            .parse = qemu_chr_parse_serial,
> -            .create = qmp_chardev_open_serial,
> -        },
> +    static const CharDriver *drivers[] = {
> +        &null_driver,
> +        &socket_driver,
> +        &udp_driver,
> +        &ringbuf_driver,
> +        &file_driver,
> +        &stdio_driver,
> +#ifdef HAVE_CHARDEV_SERIAL
> +        &serial_driver,
>  #endif

Will be some rebase churn here as you deal with my comments on earlier
patches, but they should be obvious enough that it shouldn't affect my
review.

With nits fixed,
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature