Hi
On Wed, Jan 4, 2017 at 9:26 PM Eric Blake <ebl...@redhat.com> 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 <marcandre.lur...@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);
>
>
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
