On 12/12/2016 04:42 PM, Marc-André Lureau wrote: > Turn Chardev into Object. > > qemu_chr_alloc() is replaced by the qemu_chardev_new() constructor. It > will call qemu_char_open() to open/intialize the chardev with the > ChardevCommon *backend settings. > > The CharDriver::create() callback is turned into a ChardevClass::open() > which is called from the newly introduced qemu_chardev_open(). > > "chardev-gdb" and "chardev-hci" are internal chardev and aren't > creatable directly with -chardev. Use a new internal flag to disable > them. We may want to use TYPE_USER_CREATABLE interface instead, or > perhaps allow -chardev usage. > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > backends/baum.c | 70 +-- > backends/msmouse.c | 57 ++- > backends/testdev.c | 34 +- > gdbstub.c | 33 +- > hw/bt/hci-csr.c | 54 +- > monitor.c | 2 +- > qemu-char.c | 1334 > ++++++++++++++++++++++++++----------------------- > spice-qemu-char.c | 144 +++--- > ui/console.c | 73 +-- > ui/gtk.c | 51 +- > vl.c | 2 + > include/sysemu/char.h | 107 ++-- > include/ui/console.h | 2 + > 13 files changed, 1084 insertions(+), 879 deletions(-)
Big, but probably not possible to break it into many more chunks.
Rearranging the reply, to put the .h first:
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 384f3ce9b7..1ee8aa4325 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -10,6 +10,7 @@
> #include "qapi/qmp/qstring.h"
> #include "qemu/main-loop.h"
> #include "qemu/bitmap.h"
> +#include "qom/object.h"
>
> /* character device */
>
> @@ -90,7 +91,8 @@ typedef struct CharBackend {
> typedef struct CharDriver CharDriver;
>
> struct Chardev {
> - const CharDriver *driver;
> + Object parent_obj;
> +
> QemuMutex chr_write_lock;
> CharBackend *be;
> char *label;
> @@ -102,18 +104,6 @@ struct Chardev {
> QTAILQ_ENTRY(Chardev) next;
> };
>
>
> +#define TYPE_CHARDEV "chardev"
> +#define CHARDEV(obj) OBJECT_CHECK(Chardev, (obj), TYPE_CHARDEV)
> +#define CHARDEV_CLASS(klass) \
> + OBJECT_CLASS_CHECK(ChardevClass, (klass), TYPE_CHARDEV)
> +#define CHARDEV_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(ChardevClass, (obj), TYPE_CHARDEV)
> +
> +#define TYPE_CHARDEV_NULL "chardev-null"
> +#define TYPE_CHARDEV_MUX "chardev-mux"
> +#define TYPE_CHARDEV_RINGBUF "chardev-ringbuf"
> +#define TYPE_CHARDEV_PTY "chardev-pty"
> +#define TYPE_CHARDEV_CONSOLE "chardev-console"
> +#define TYPE_CHARDEV_STDIO "chardev-stdio"
> +#define TYPE_CHARDEV_PIPE "chardev-pipe"
> +#define TYPE_CHARDEV_MEMORY "chardev-memory"
> +#define TYPE_CHARDEV_PARALLEL "chardev-parallel"
> +#define TYPE_CHARDEV_FILE "chardev-file"
> +#define TYPE_CHARDEV_SERIAL "chardev-serial"
> +#define TYPE_CHARDEV_SOCKET "chardev-socket"
> +#define TYPE_CHARDEV_UDP "chardev-udp"
> +
> +#define CHARDEV_IS_MUX(chr) \
> + object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
> +#define CHARDEV_IS_RINGBUF(chr) \
> + object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_RINGBUF)
> +#define CHARDEV_IS_PTY(chr) \
> + object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_PTY)
> +
> +typedef struct ChardevClass {
> + ObjectClass parent_class;
> +
> + bool internal; /* TODO: eventually use TYPE_USER_CREATABLE */
> +
> + void (*open)(Chardev *chr, ChardevBackend *backend,
> + bool *be_opened, Error **errp);
> +
> + int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
> + int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
> + GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
> + void (*chr_update_read_handler)(Chardev *s, GMainContext *context);
> + int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
> + int (*get_msgfds)(Chardev *s, int* fds, int num);
> + int (*set_msgfds)(Chardev *s, int *fds, int num);
> + int (*chr_add_client)(Chardev *chr, int fd);
> + int (*chr_wait_connected)(Chardev *chr, Error **errp);
> + void (*chr_free)(Chardev *chr);
> + void (*chr_disconnect)(Chardev *chr);
> + void (*chr_accept_input)(Chardev *chr);
> + void (*chr_set_echo)(Chardev *chr, bool echo);
> + void (*chr_set_fe_open)(Chardev *chr, int fe_open);
> +} ChardevClass;
Looks nice.
On to the .c:
>
> diff --git a/backends/baum.c b/backends/baum.c
> index 7fd1ebc557..80103b6098 100644
> --- a/backends/baum.c
> +++ b/backends/baum.c
> @@ -102,6 +102,9 @@ typedef struct {
> QEMUTimer *cellCount_timer;
> } BaumChardev;
>
> +#define TYPE_CHARDEV_BRAILLE "chardev-braille"
> +#define BAUM_CHARDEV(obj) OBJECT_CHECK(BaumChardev, (obj),
> TYPE_CHARDEV_BRAILLE)
As mentioned earlier, OBJECT_CHECK() is a nice wrapper around
container_of(), so this indeed resolves my earlier concerns on casts.
> +
> /* Let's assume NABCC by default */
> enum way {
> DOTS2ASCII,
> @@ -268,9 +271,9 @@ static int baum_deferred_init(BaumChardev *baum)
> }
>
> /* The serial port can receive more of our data */
> -static void baum_accept_input(struct Chardev *chr)
> +static void baum_chr_accept_input(struct Chardev *chr)
Why the rename?
> @@ -485,9 +488,9 @@ static int baum_eat_packet(BaumChardev *baum, const
> uint8_t *buf, int len)
> }
>
> /* The other end is writing some data. Store it and try to interpret */
> -static int baum_write(Chardev *chr, const uint8_t *buf, int len)
> +static int baum_chr_write(Chardev *chr, const uint8_t *buf, int len)
and again
> @@ -544,7 +547,7 @@ static void baum_send_key2(BaumChardev *baum, uint8_t
> type, uint8_t value,
> /* We got some data on the BrlAPI socket */
> static void baum_chr_read(void *opaque)
> {
If it is for consistency in the baum callback names, maybe a separate
patch is warranted for that part.
> @@ -515,33 +485,98 @@ static void remove_fd_in_watch(Chardev *chr);
> static void mux_chr_set_handlers(Chardev *chr, GMainContext *context);
> static void mux_set_focus(MuxChardev *d, int focus);
>
> -static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
> +static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
> + bool *be_opened, Error **errp)
> {
> - return len;
> + ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> + /* Any ChardevCommon member would work */
> + ChardevCommon *common = backend ? backend->u.null.data : NULL;
> +
> + if (common && common->has_logfile) {
> + int flags = O_WRONLY | O_CREAT;
> + if (common->has_logappend &&
> + common->logappend) {
> + flags |= O_APPEND;
> + } else {
> + flags |= O_TRUNC;
> + }
> + chr->logfd = qemu_open(common->logfile, flags, 0666);
> + if (chr->logfd < 0) {
> + error_setg_errno(errp, errno,
> + "Unable to open logfile %s",
> + common->logfile);
> + return;
> + }
> + }
> +
> + if (cc->open) {
> + cc->open(chr, backend, be_opened, errp);
> + }
> }
Looking good.
> @@ -1421,19 +1451,15 @@ static Chardev *qemu_chr_open_stdio(const CharDriver
> *driver,
> act.sa_handler = term_stdio_handler;
> sigaction(SIGCONT, &act, NULL);
>
> - chr = qemu_chr_open_fd(driver, 0, 1, common, errp);
> - if (!chr) {
> - return NULL;
> - }
> + qemu_chr_open_fd(chr, 0, 1);
> +
> if (opts->has_signal) {
> stdio_allow_signal = opts->signal;
> }
> qemu_chr_set_echo_stdio(chr, false);
> -
> - return chr;
> }
>
> -#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
> +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
> || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__)
> \
The change in this #if looks spurious.
> @@ -3905,18 +3925,24 @@ static void qemu_chr_parse_pipe(QemuOpts *opts,
> ChardevBackend *backend,
>
> static const CharDriver pipe_driver = {
> .kind = CHARDEV_BACKEND_KIND_PIPE,
> - .parse = qemu_chr_parse_pipe, .create = qemu_chr_open_pipe,
> + .parse = qemu_chr_parse_pipe
> +};
nit: As pointed out earlier in the series, I like trailing comma in
struct initializations.
>
> static const CharDriver udp_driver = {
> - .instance_size = sizeof(UdpChardev),
> .kind = CHARDEV_BACKEND_KIND_UDP,
> - .parse = qemu_chr_parse_udp, .create = qmp_chardev_open_udp,
> - .chr_write = udp_chr_write,
> - .chr_update_read_handler = udp_chr_update_read_handler,
> - .chr_free = udp_chr_free,
> + .parse = qemu_chr_parse_udp
> +};
Multiple places
> @@ -5020,33 +5115,44 @@ void qemu_chr_cleanup(void)
>
> static void register_types(void)
> {
> - static const CharDriver *drivers[] = {
> - &null_driver,
> - &socket_driver,
> - &udp_driver,
> - &ringbuf_driver,
> - &file_driver,
> - &stdio_driver,
> + static const struct {
> + const CharDriver *driver;
> + const TypeInfo *type;
> + } chardevs[] = {
> + { &null_driver, &char_null_type_info },
> + { &socket_driver, &char_socket_type_info },
> + { &udp_driver, &char_udp_type_info },
> + { &ringbuf_driver, &char_ringbuf_type_info },
> + { &file_driver, &char_file_type_info },
> + { &stdio_driver, &char_stdio_type_info },
> #ifdef HAVE_CHARDEV_SERIAL
> - &serial_driver,
> + { &serial_driver, &char_serial_type_info },
> #endif
> #ifdef HAVE_CHARDEV_PARPORT
> - ¶llel_driver,
> + { ¶llel_driver, &char_parallel_type_info },
> #endif
> #ifdef HAVE_CHARDEV_PTY
> - &pty_driver,
> + { &pty_driver, &char_pty_type_info },
> #endif
> #ifdef _WIN32
> - &console_driver,
> + { &console_driver, &char_console_type_info },
> #endif
> - &pipe_driver,
> - &mux_driver,
> - &memory_driver
> + { &pipe_driver, &char_pipe_type_info },
> + { &mux_driver, &char_mux_type_info },
> + { &memory_driver, &char_memory_type_info }
> };
> int i;
>
> - for (i = 0; i < ARRAY_SIZE(drivers); i++) {
> - register_char_driver(drivers[i]);
> + type_register_static(&char_type_info);
> +#ifndef _WIN32
> + type_register_static(&char_fd_type_info);
> +#else
> + type_register_static(&char_win_type_info);
> + type_register_static(&char_win_stdio_type_info);
> +#endif
> + for (i = 0; i < ARRAY_SIZE(chardevs); i++) {
> + type_register_static(chardevs[i].type);
> + register_char_driver(chardevs[i].driver);
> }
>
> /* this must be done after machine init, since we register FEs with muxes
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
So far, looks like a sane conversion. I've run out of review time
today; I'll have to pick up from this file tomorrow.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
