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 <marcandre.lur...@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.
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
