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

[Marc-André Lureau]

Hi

On Wed, Jan 4, 2017 at 9:26 PM Eric Blake <address@hidden> wrote:

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

yes, fixed


>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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);
>
>
The follow-up of the series converts it to the more appropriate
 BaumChardev *baum = BAUM_CHARDEV(obj);

So considering that this is temporary commit, do I have to change that?

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.
>
>
right


> >      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.
>
>
thanks, the goal is to get rid of them, but not in this commit :) See
"chardev: qom-ify".
-- 
Marc-André Lureau