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

[Marc-André Lureau]

hi

----- Original Message -----
> 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/

ok

> 
> > information on all instances.
> > 
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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.

and kind is removed is later patches "char: remove class kind field"

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

mux_chr_add_watch() also checks if the underlying driver has chr_add_watch(). 
qemu_chr_fe_add_watch() returns 0 in both cases then.


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

I did place it where it felt right at the time I did the change. I don't think 
we need to care much because all this is going away in the series. No strong 
feeling 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.

thanks