Re: [Qemu-devel] [PATCH 04/54] char: move callbacks in CharDriver

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6887 bytes --]

On 12/12/2016 04:42 PM, Marc-André Lureau wrote:
> This makes the code more declarative, and avoids to duplicate the

s/to duplicate/duplicating/

> information on all instances.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  backends/baum.c       |  13 +-
>  backends/msmouse.c    |  13 +-
>  backends/testdev.c    |  10 +-
>  gdbstub.c             |   7 +-
>  hw/bt/hci-csr.c       |   8 +-
>  qemu-char.c           | 427 +++++++++++++++++++++++++++++++-------------------
>  spice-qemu-char.c     |  36 +++--
>  ui/console.c          |  26 +--
>  ui/gtk.c              |  11 +-
>  include/sysemu/char.h |  46 +++---

Again, seeing the .h changes first makes a huge difference on code
review.  I'm manually reformatting:

>  10 files changed, 370 insertions(+), 227 deletions(-)

> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index c8750ede21..09e40ef9b8 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -85,24 +85,11 @@ typedef struct CharBackend {
>      int fe_open;
>  } CharBackend;
>
> +typedef struct CharDriver CharDriver;
> +
>  struct CharDriverState {
> +    const CharDriver *driver;
>      QemuMutex chr_write_lock;
> -    int (*chr_write)(struct CharDriverState *s, const uint8_t *buf,
int len);

So all the callbacks are moved into the CharDriver struct...

> @@ -125,7 +112,8 @@ struct CharDriverState {
>   *
>   * Returns: a newly allocated CharDriverState, or NULL on error.
>   */
> -CharDriverState *qemu_chr_alloc(ChardevCommon *backend, Error **errp);
> +CharDriverState *qemu_chr_alloc(const CharDriver *driver,
> +                                ChardevCommon *backend, Error **errp);

...and all the entry points are now passed that struct as a new parameter.

>
>  /**
>   * @qemu_chr_new_from_opts:
> @@ -473,15 +461,33 @@ void qemu_chr_set_feature(CharDriverState *chr,
>                            CharDriverFeature feature);
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>
> -typedef struct CharDriver {
> +struct CharDriver {

...the struct already existed, but is now more useful.

Looks worthwhile. I suspect the rest of the patch is mechanical.

> @@ -688,7 +686,10 @@ static void register_types(void)
>  {
>      static const CharDriver driver = {
>          .kind = CHARDEV_BACKEND_KIND_BRAILLE,
> -        .parse = NULL, .create = chr_baum_init
> +        .parse = NULL, .create = chr_baum_init,

And now you see why I asked for trailing commas in 2/54 :)

> +        .chr_write = baum_write,
> +        .chr_accept_input = baum_accept_input,
> +        .chr_free = baum_free,
>      };
>  

> +++ 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

Trailing comma.

Interesting that this is a new use of CharDriver with an out-of-range
.kind. But I think your code in 3/54 was careful to explicitly handle a
.kind that does not map to one of the public types, so that you are able
to use this as an internal-only driver.


>  
> +static const CharDriver null_driver = {
> +    .kind = CHARDEV_BACKEND_KIND_NULL, .create = qemu_chr_open_null,

One initializer per line is fine.

> +    .chr_write = null_chr_write
> +};
> +

> 
> @@ -864,14 +880,6 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
>  
>      chr->opaque = d;
>      d->focus = -1;
> -    chr->chr_free = mux_chr_free;
> -    chr->chr_write = mux_chr_write;
> -    chr->chr_accept_input = mux_chr_accept_input;
> -    /* Frontend guest-open / -close notification is not support with muxes */
> -    chr->chr_set_fe_open = NULL;
> -    if (drv->chr_add_watch) {
> -        chr->chr_add_watch = mux_chr_add_watch;
> -    }

Here, the callback was only conditionally registered...

> +static const CharDriver mux_driver = {
> +    .kind = CHARDEV_BACKEND_KIND_MUX,
> +    .parse = qemu_chr_parse_mux, .create = qemu_chr_open_mux,
> +    .chr_free = mux_chr_free,
> +    .chr_write = mux_chr_write,
> +    .chr_accept_input = mux_chr_accept_input,
> +    .chr_add_watch = mux_chr_add_watch,
> +};

...but here, it is always registered.  Is that an unintentional semantic
change?


> +#ifdef HAVE_CHARDEV_SERIAL
> +static const CharDriver serial_driver = {
> +    .alias = "tty", .kind = CHARDEV_BACKEND_KIND_SERIAL,
> +    .parse = qemu_chr_parse_serial, .create = qmp_chardev_open_serial,
> +#ifdef _WIN32
> +    .chr_write = win_chr_write,
> +    .chr_free = win_chr_free,
> +#else
> +    .chr_add_watch = fd_chr_add_watch,
> +    .chr_write = fd_chr_write,
> +    .chr_update_read_handler = fd_chr_update_read_handler,
> +    .chr_ioctl = tty_serial_ioctl,
> +    .chr_free = qemu_chr_free_tty,
> +#endif
> +};
> +#endif


> @@ -4910,49 +5037,33 @@ void qemu_chr_cleanup(void)
>  
>  static void register_types(void)
>  {
> -    int i;
> -    static const CharDriver drivers[] = {
> -        { .kind = CHARDEV_BACKEND_KIND_NULL, .parse = NULL,
> -          .create = qemu_chr_open_null },
> -        { .kind = CHARDEV_BACKEND_KIND_SOCKET,
> -          .parse = qemu_chr_parse_socket, .create = qmp_chardev_open_socket },
> -        { .kind = CHARDEV_BACKEND_KIND_UDP, .parse = qemu_chr_parse_udp,
> -          .create = qmp_chardev_open_udp },
> -        { .kind = CHARDEV_BACKEND_KIND_RINGBUF,
> -          .parse = qemu_chr_parse_ringbuf, .create = qemu_chr_open_ringbuf },
> -        { .kind = CHARDEV_BACKEND_KIND_FILE,
> -          .parse = qemu_chr_parse_file_out, .create = qmp_chardev_open_file },
> -        { .kind = CHARDEV_BACKEND_KIND_STDIO,
> -          .parse = qemu_chr_parse_stdio, .create = qemu_chr_open_stdio },
> -#if defined HAVE_CHARDEV_SERIAL
> -        { .kind = CHARDEV_BACKEND_KIND_SERIAL, .alias = "tty",
> -          .parse = qemu_chr_parse_serial, .create = qmp_chardev_open_serial },

It feels like some code motion between 3/54 and 4/54 (you are moving
where the CharDriver is declared); is it worth tweaking the series to
avoid the code motion by declaring the structs in the right place to
begin with?  Not necessarily a show-stopper to the series, though.

> +    static const CharDriver *drivers[] = {
> +        &null_driver,
> +        &socket_driver,
> +        &udp_driver,
> +        &ringbuf_driver,
> +        &file_driver,
> +        &stdio_driver,
> +#ifdef HAVE_CHARDEV_SERIAL
> +        &serial_driver,
>  #endif

Overall impression is that I still like where this is headed.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]