Re: [Qemu-devel] [PATCH 15/54] chardev: qom-ify

[Eric Blake <eblake@redhat.com>]

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

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.lureau@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
> -        &parallel_driver,
> +        { &parallel_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


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