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.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.
>
> +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
signature.asc
Description: OpenPGP digital signature
