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.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);
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
signature.asc
Description: OpenPGP digital signature
