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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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.
> 
> Review part 2:
> 
> >  spice-qemu-char.c     |  144 +++---
> >  ui/console.c          |   73 +--
> >  ui/gtk.c              |   51 +-
> >  vl.c                  |    2 +
> 
> > +++ b/spice-qemu-char.c
> > @@ -18,6 +18,12 @@ typedef struct SpiceChardev {
> >      QLIST_ENTRY(SpiceChardev) next;
> >  } SpiceChardev;
> >  
> > +#define TYPE_CHARDEV_SPICE "chardev-spice"
> > +#define TYPE_CHARDEV_SPICEVMC "chardev-spicevmc"
> > +#define TYPE_CHARDEV_SPICEPORT "chardev-spiceport"
> 
> Why do we have a lot of TYPE_CHARDEV_* in sysemu/char.h, but not these?
> Should they all live in the same place?

I think in general, we keep typename private, unless the type is being used by 
some other file.

In this refactoring series, it's a bit tricky, because there are types such as 
mux, memory and pty  that are shared with several units (when splitting).

Moving them back and forth in char.h introduces yet again code churning in my 
tree ;) It would be easier to clean that later in the series. So for this 
patch, all types and common helper macros for qemu-char.c are in char.h. I'll 
update commit message with this detail.

> 
> > +++ b/ui/console.c
> > @@ -1040,9 +1040,12 @@ typedef struct VCChardev {
> >      QemuConsole *console;
> >  } VCChardev;
> >  
> > -static int console_puts(Chardev *chr, const uint8_t *buf, int len)
> > +#define TYPE_CHARDEV_VC "chardev-vc"
> > +#define VC_CHARDEV(obj) OBJECT_CHECK(VCChardev, (obj), TYPE_CHARDEV_VC)
> > +
> > +static int vc_chr_write(Chardev *chr, const uint8_t *buf, int len)
> ...
> 
> > @@ -1951,9 +1954,9 @@ int qemu_console_get_height(QemuConsole *con, int
> > fallback)
> >      return con ? surface_height(con->surface) : fallback;
> >  }
> >  
> > -static void text_console_set_echo(Chardev *chr, bool echo)
> > +static void vc_chr_set_echo(Chardev *chr, bool echo)
> 
> Should these function renames be split to a separate patch?

ok

> 
> > +++ b/ui/gtk.c
> > @@ -184,6 +184,9 @@ typedef struct VCChardev {
> >      bool echo;
> >  } VCChardev;
> >  
> > +#define TYPE_CHARDEV_VC "chardev-vc"
> > +#define VC_CHARDEV(obj) OBJECT_CHECK(VCChardev, (obj), TYPE_CHARDEV_VC)
> 
> Why do we have TYPE_CHARDEV_VC and VC_CHARDEV() defined in two different
> .c files?
> 
> Overall the conversion looks good; as said elsewhere in the thread, I
> think you can post a v2 of 1-15 and get that merged first, while still
> hammering out the details of the rest of the series.

ok