Re: [Qemu-devel] [PATCH 08/54] char: allocate CharDriverState as a single object

[Eric Blake <eblake@redhat.com>]

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

On 12/12/2016 04:42 PM, Marc-André Lureau wrote:
> Use a single allocation for CharDriverState, this avoids extra
> allocations & pointers, and is a step towards more object-oriented
> CharDriver.
> 
> Gtk console is a bit peculiar, gd_vc_chr_set_echo

Truncated paragraph?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  backends/baum.c       |  23 ++---
>  backends/msmouse.c    |  16 +--
>  backends/testdev.c    |  22 ++--
>  gdbstub.c             |   1 +
>  hw/bt/hci-csr.c       |  10 +-
>  qemu-char.c           | 280 ++++++++++++++++++++++++++------------------------
>  spice-qemu-char.c     |  39 +++----
>  ui/console.c          |  21 ++--
>  ui/gtk.c              |  30 ++++--
>  include/sysemu/char.h |   2 +-
>  10 files changed, 230 insertions(+), 214 deletions(-)
> 
> diff --git a/backends/baum.c b/backends/baum.c
> index ef6178993a..6244929ac6 100644
> --- a/backends/baum.c
> +++ b/backends/baum.c
> @@ -87,7 +87,7 @@
>  #define BUF_SIZE 256
>  
>  typedef struct {
> -    CharDriverState *chr;
> +    CharDriverState parent;
>  
>      brlapi_handle_t *brlapi;
>      int brlapi_fd;
> @@ -270,7 +270,7 @@ static int baum_deferred_init(BaumDriverState *baum)
>  /* The serial port can receive more of our data */
>  static void baum_accept_input(struct CharDriverState *chr)
>  {
> -    BaumDriverState *baum = chr->opaque;
> +    BaumDriverState *baum = (BaumDriverState *)chr;

It might be a little nicer to use:

BaumDriverState *baum = container_of(chr, BaumDriverState, parent);

to avoid a cast and work even if the members are rearranged within the
structure. You can even write a one-line helper function to hide the
cast behind a more legible semantic (for example, see to_qov() in
qobject-output-visitor.c).

(Same comment applies to most other base-to-derived casts in your patch)

> +++ b/hw/bt/hci-csr.c
> @@ -28,11 +28,11 @@
>  #include "hw/bt.h"
>  
>  struct csrhci_s {
> +    CharDriverState chr;
>      int enable;
>      qemu_irq *pins;
>      int pin_state;
>      int modem_state;
> -    CharDriverState chr;
>  #define FIFO_LEN	4096
>      int out_start;
>      int out_len;
> @@ -314,7 +314,7 @@ static void csrhci_ready_for_next_inpkt(struct csrhci_s *s)
>  static int csrhci_write(struct CharDriverState *chr,
>                  const uint8_t *buf, int len)
>  {
> -    struct csrhci_s *s = (struct csrhci_s *) chr->opaque;
> +    struct csrhci_s *s = (struct csrhci_s *)chr;

Wonder if a typedef would make this more legible, but maybe as a
separate cleanup.

> +++ b/ui/gtk.c
> @@ -178,6 +178,12 @@ struct GtkDisplayState {
>      bool ignore_keys;
>  };
>  
> +typedef struct VCDriverState {
> +    CharDriverState parent;
> +    VirtualConsole *console;
> +    bool echo;
> +} VCDriverState;
> +
>  static void gd_grab_pointer(VirtualConsole *vc, const char *reason);
>  static void gd_ungrab_pointer(GtkDisplayState *s);
>  static void gd_grab_keyboard(VirtualConsole *vc, const char *reason);
> @@ -1675,7 +1681,8 @@ static void gd_vc_adjustment_changed(GtkAdjustment *adjustment, void *opaque)
>  
>  static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>  {
> -    VirtualConsole *vc = chr->opaque;
> +    VCDriverState *vcd = (VCDriverState *)chr;
> +    VirtualConsole *vc = vcd->console;
>  
>      vte_terminal_feed(VTE_TERMINAL(vc->vte.terminal), (const char *)buf, len);
>      return len;
> @@ -1683,9 +1690,14 @@ static int gd_vc_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>  
>  static void gd_vc_chr_set_echo(CharDriverState *chr, bool echo)
>  {
> -    VirtualConsole *vc = chr->opaque;
> +    VCDriverState *vcd = (VCDriverState *)chr;
> +    VirtualConsole *vc = vcd->console;
>  
> -    vc->vte.echo = echo;
> +    if (vc) {
> +        vc->vte.echo = echo;
> +    } else {
> +        vcd->echo = echo;
> +    }
>  }
>  
>  static int nb_vcs;
> @@ -1694,6 +1706,7 @@ static CharDriverState *vcs[MAX_VCS];
>  static CharDriverState *gd_vc_handler(ChardevVC *vc, Error **errp)
>  {
>      static const CharDriver gd_vc_driver = {
> +        .instance_size = sizeof(VCDriverState),
>          .kind = CHARDEV_BACKEND_KIND_VC,
>          .chr_write = gd_vc_chr_write,
>          .chr_set_echo = gd_vc_chr_set_echo,
> @@ -1712,9 +1725,6 @@ static CharDriverState *gd_vc_handler(ChardevVC *vc, Error **errp)
>          return NULL;
>      }
>  
> -    /* Temporary, until gd_vc_vte_init runs.  */
> -    chr->opaque = g_new0(VirtualConsole, 1);
> -

Okay, I see the weirdness going on with the 'echo' stuff - you have to
simulate the temporary placeholder for whether or not to set echo, such
that gd_vc_chr_set_echo() can now be called with a NULL opaque where it
used to always have an object to dereference.

>      vcs[nb_vcs++] = chr;
>  
>      return chr;
> @@ -1755,14 +1765,12 @@ static GSList *gd_vc_vte_init(GtkDisplayState *s, VirtualConsole *vc,
>      GtkWidget *box;
>      GtkWidget *scrollbar;
>      GtkAdjustment *vadjustment;
> -    VirtualConsole *tmp_vc = chr->opaque;
> +    VCDriverState *vcd = (VCDriverState *)chr;
>  
>      vc->s = s;
> -    vc->vte.echo = tmp_vc->vte.echo;
> -
> +    vc->vte.echo = vcd->echo;
>      vc->vte.chr = chr;
> -    chr->opaque = vc;
> -    g_free(tmp_vc);
> +    vcd->console = vc;
>  

So it is indeed weird, but looks correct in the end.

>      snprintf(buffer, sizeof(buffer), "vc%d", idx);
>      vc->label = g_strdup_printf("%s", vc->vte.chr->label
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 07dfa59afe..5d8ec982a9 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -93,7 +93,6 @@ struct CharDriverState {
>      const CharDriver *driver;
>      QemuMutex chr_write_lock;
>      CharBackend *be;
> -    void *opaque;
>      char *label;
>      char *filename;
>      int logfd;
> @@ -482,6 +481,7 @@ struct CharDriver {
>                                 ChardevBackend *backend,
>                                 ChardevReturn *ret, bool *be_opened,
>                                 Error **errp);
> +    size_t instance_size;

My biggest gripe is the number of casts that could be container_of()
instead; but otherwise it looks like a sane conversion.

-- 
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 --]