[PATCH 00/18] chardev: QOM cleanups

Eduardo Habkost posted 18 patches 3 years, 6 months ago
Failed in applying to current master (apply log)
chardev/chardev-internal.h |  2 +-
include/chardev/char-fd.h  |  2 +-
include/chardev/char-win.h |  2 +-
include/chardev/spice.h    |  2 +-
chardev/baum.c             | 14 ++++----
chardev/char-fd.c          | 14 ++++----
chardev/char-fe.c          |  4 +--
chardev/char-mux.c         | 22 ++++++------
chardev/char-parallel.c    | 28 ++++++++--------
chardev/char-pipe.c        |  2 +-
chardev/char-pty.c         | 22 ++++++------
chardev/char-ringbuf.c     | 12 +++----
chardev/char-serial.c      |  2 +-
chardev/char-socket.c      | 68 +++++++++++++++++++-------------------
chardev/char-udp.c         | 14 ++++----
chardev/char-win-stdio.c   | 14 ++++----
chardev/char-win.c         | 14 ++++----
chardev/char.c             |  2 +-
chardev/msmouse.c          | 12 +++----
chardev/spice.c            | 16 ++++-----
chardev/testdev.c          |  4 +--
chardev/wctablet.c         | 12 +++----
ui/console.c               | 10 +++---
ui/gtk.c                   |  8 ++---
ui/spice-app.c             |  2 +-
25 files changed, 151 insertions(+), 153 deletions(-)
[PATCH 00/18] chardev: QOM cleanups
Posted by Eduardo Habkost 3 years, 6 months ago
Some chardev QOM cleanup patches had to be dropped from my queue
due to build erros introduced by code movements across ifdef
boundaries at char-parallel.c.  This series redo the changes from
those patches, but the macro renames are now a little different:

In this version I have decided to rename the type checking macros
from *_CHARDEV to CHARDEV_* instead of renaming tye
TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
identifiers actually match the QOM type name strings
("chardev-*").

Eduardo Habkost (18):
  chardev: Move PARALLEL_CHARDEV macro to common code
  chardev: Move ParallelChardev typedef to common code
  chardev: Use DECLARE_INSTANCE_CHECKER macro for PARALLEL_CHARDEV
  chardev: Rename MOUSE_CHARDEV to CHARDEV_MSMOUSE
  chardev: Rename BAUM_CHARDEV to CHARDEV_BRAILLE
  chardev: Rename FD_CHARDEV to CHARDEV_FD
  chardev: Rename MUX_CHARDEV to CHARDEV_MUX
  chardev: Rename PARALLEL_CHARDEV to CHARDEV_PARALLEL
  chardev: Rename PTY_CHARDEV to CHARDEV_PTY
  chardev: Rename RINGBUF_CHARDEV to CHARDEV_RINGBUF
  chardev: Rename SOCKET_CHARDEV to CHARDEV_SOCKET
  chardev: Rename SPICE_CHARDEV to CHARDEV_SPICE
  chardev: Rename TESTDEV_CHARDEV to CHARDEV_TESTDEV
  chardev: Rename UDP_CHARDEV to CHARDEV_UDP
  chardev: Rename VC_CHARDEV to CHARDEV_VC
  chardev: Rename WCTABLET_CHARDEV to CHARDEV_WCTABLET
  chardev: Rename WIN_CHARDEV to CHARDEV_WIN
  chardev: Rename WIN_STDIO_CHARDEV to CHARDEV_WIN_STDIO

 chardev/chardev-internal.h |  2 +-
 include/chardev/char-fd.h  |  2 +-
 include/chardev/char-win.h |  2 +-
 include/chardev/spice.h    |  2 +-
 chardev/baum.c             | 14 ++++----
 chardev/char-fd.c          | 14 ++++----
 chardev/char-fe.c          |  4 +--
 chardev/char-mux.c         | 22 ++++++------
 chardev/char-parallel.c    | 28 ++++++++--------
 chardev/char-pipe.c        |  2 +-
 chardev/char-pty.c         | 22 ++++++------
 chardev/char-ringbuf.c     | 12 +++----
 chardev/char-serial.c      |  2 +-
 chardev/char-socket.c      | 68 +++++++++++++++++++-------------------
 chardev/char-udp.c         | 14 ++++----
 chardev/char-win-stdio.c   | 14 ++++----
 chardev/char-win.c         | 14 ++++----
 chardev/char.c             |  2 +-
 chardev/msmouse.c          | 12 +++----
 chardev/spice.c            | 16 ++++-----
 chardev/testdev.c          |  4 +--
 chardev/wctablet.c         | 12 +++----
 ui/console.c               | 10 +++---
 ui/gtk.c                   |  8 ++---
 ui/spice-app.c             |  2 +-
 25 files changed, 151 insertions(+), 153 deletions(-)

-- 
2.26.2



Re: [PATCH 00/18] chardev: QOM cleanups
Posted by Marc-André Lureau 3 years, 6 months ago
Hi

On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com>
wrote:

> Some chardev QOM cleanup patches had to be dropped from my queue
> due to build erros introduced by code movements across ifdef
> boundaries at char-parallel.c.  This series redo the changes from
> those patches, but the macro renames are now a little different:
>
> In this version I have decided to rename the type checking macros
> from *_CHARDEV to CHARDEV_* instead of renaming tye
> TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
> identifiers actually match the QOM type name strings
> ("chardev-*").
>

Sounds reasonable to me, but it loses the matching with the
structure/object type name though.

- MuxChardev *d = MUX_CHARDEV(s);
+ MuxChardev *d = CHARDEV_MUX(s);

I have a preference for the first. Unless we rename all the chardev types
too...

Imho, the QOM type name is mostly an internal detail, the C type name is
dominant in the code.



> Eduardo Habkost (18):
>   chardev: Move PARALLEL_CHARDEV macro to common code
>   chardev: Move ParallelChardev typedef to common code
>   chardev: Use DECLARE_INSTANCE_CHECKER macro for PARALLEL_CHARDEV
>   chardev: Rename MOUSE_CHARDEV to CHARDEV_MSMOUSE
>   chardev: Rename BAUM_CHARDEV to CHARDEV_BRAILLE
>   chardev: Rename FD_CHARDEV to CHARDEV_FD
>   chardev: Rename MUX_CHARDEV to CHARDEV_MUX
>   chardev: Rename PARALLEL_CHARDEV to CHARDEV_PARALLEL
>   chardev: Rename PTY_CHARDEV to CHARDEV_PTY
>   chardev: Rename RINGBUF_CHARDEV to CHARDEV_RINGBUF
>   chardev: Rename SOCKET_CHARDEV to CHARDEV_SOCKET
>   chardev: Rename SPICE_CHARDEV to CHARDEV_SPICE
>   chardev: Rename TESTDEV_CHARDEV to CHARDEV_TESTDEV
>   chardev: Rename UDP_CHARDEV to CHARDEV_UDP
>   chardev: Rename VC_CHARDEV to CHARDEV_VC
>   chardev: Rename WCTABLET_CHARDEV to CHARDEV_WCTABLET
>   chardev: Rename WIN_CHARDEV to CHARDEV_WIN
>   chardev: Rename WIN_STDIO_CHARDEV to CHARDEV_WIN_STDIO
>
>  chardev/chardev-internal.h |  2 +-
>  include/chardev/char-fd.h  |  2 +-
>  include/chardev/char-win.h |  2 +-
>  include/chardev/spice.h    |  2 +-
>  chardev/baum.c             | 14 ++++----
>  chardev/char-fd.c          | 14 ++++----
>  chardev/char-fe.c          |  4 +--
>  chardev/char-mux.c         | 22 ++++++------
>  chardev/char-parallel.c    | 28 ++++++++--------
>  chardev/char-pipe.c        |  2 +-
>  chardev/char-pty.c         | 22 ++++++------
>  chardev/char-ringbuf.c     | 12 +++----
>  chardev/char-serial.c      |  2 +-
>  chardev/char-socket.c      | 68 +++++++++++++++++++-------------------
>  chardev/char-udp.c         | 14 ++++----
>  chardev/char-win-stdio.c   | 14 ++++----
>  chardev/char-win.c         | 14 ++++----
>  chardev/char.c             |  2 +-
>  chardev/msmouse.c          | 12 +++----
>  chardev/spice.c            | 16 ++++-----
>  chardev/testdev.c          |  4 +--
>  chardev/wctablet.c         | 12 +++----
>  ui/console.c               | 10 +++---
>  ui/gtk.c                   |  8 ++---
>  ui/spice-app.c             |  2 +-
>  25 files changed, 151 insertions(+), 153 deletions(-)
>
> --
> 2.26.2
>
>
>
>

-- 
Marc-André Lureau
Re: [PATCH 00/18] chardev: QOM cleanups
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com>
> wrote:
> 
> > Some chardev QOM cleanup patches had to be dropped from my queue
> > due to build erros introduced by code movements across ifdef
> > boundaries at char-parallel.c.  This series redo the changes from
> > those patches, but the macro renames are now a little different:
> >
> > In this version I have decided to rename the type checking macros
> > from *_CHARDEV to CHARDEV_* instead of renaming tye
> > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
> > identifiers actually match the QOM type name strings
> > ("chardev-*").
> >
> 
> Sounds reasonable to me, but it loses the matching with the
> structure/object type name though.
> 
> - MuxChardev *d = MUX_CHARDEV(s);
> + MuxChardev *d = CHARDEV_MUX(s);
> 
> I have a preference for the first. Unless we rename all the chardev types
> too...

I tend to think the structs should be renamed too - I've always considerd
them to be backwards.

> Imho, the QOM type name is mostly an internal detail, the C type name is
> dominant in the code.

Err it is the reverse. The QOM type name is exposed public API via QOM
commands, while the C struct names are a internal private detail.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH 00/18] chardev: QOM cleanups
Posted by Marc-André Lureau 3 years, 6 months ago
Hi

On Fri, Sep 11, 2020 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com>
> > wrote:
> >
> > > Some chardev QOM cleanup patches had to be dropped from my queue
> > > due to build erros introduced by code movements across ifdef
> > > boundaries at char-parallel.c.  This series redo the changes from
> > > those patches, but the macro renames are now a little different:
> > >
> > > In this version I have decided to rename the type checking macros
> > > from *_CHARDEV to CHARDEV_* instead of renaming tye
> > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
> > > identifiers actually match the QOM type name strings
> > > ("chardev-*").
> > >
> >
> > Sounds reasonable to me, but it loses the matching with the
> > structure/object type name though.
> >
> > - MuxChardev *d = MUX_CHARDEV(s);
> > + MuxChardev *d = CHARDEV_MUX(s);
> >
> > I have a preference for the first. Unless we rename all the chardev types
> > too...
>
> I tend to think the structs should be renamed too - I've always considerd
> them to be backwards.
>
> > Imho, the QOM type name is mostly an internal detail, the C type name is
> > dominant in the code.
>
> Err it is the reverse. The QOM type name is exposed public API via QOM
> commands, while the C struct names are a internal private detail.
>
>
Yes, but without the chardev- prefix (unless you try object-add which I
don't think will work with chardev)


-- 
Marc-André Lureau
Re: [PATCH 00/18] chardev: QOM cleanups
Posted by Daniel P. Berrangé 3 years, 6 months ago
On Fri, Sep 11, 2020 at 12:19:08PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 11, 2020 at 12:10 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com>
> > > wrote:
> > >
> > > > Some chardev QOM cleanup patches had to be dropped from my queue
> > > > due to build erros introduced by code movements across ifdef
> > > > boundaries at char-parallel.c.  This series redo the changes from
> > > > those patches, but the macro renames are now a little different:
> > > >
> > > > In this version I have decided to rename the type checking macros
> > > > from *_CHARDEV to CHARDEV_* instead of renaming tye
> > > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
> > > > identifiers actually match the QOM type name strings
> > > > ("chardev-*").
> > > >
> > >
> > > Sounds reasonable to me, but it loses the matching with the
> > > structure/object type name though.
> > >
> > > - MuxChardev *d = MUX_CHARDEV(s);
> > > + MuxChardev *d = CHARDEV_MUX(s);
> > >
> > > I have a preference for the first. Unless we rename all the chardev types
> > > too...
> >
> > I tend to think the structs should be renamed too - I've always considerd
> > them to be backwards.
> >
> > > Imho, the QOM type name is mostly an internal detail, the C type name is
> > > dominant in the code.
> >
> > Err it is the reverse. The QOM type name is exposed public API via QOM
> > commands, while the C struct names are a internal private detail.
> >
> 
> Yes, but without the chardev- prefix (unless you try object-add which I
> don't think will work with chardev)

Sure, that's just the way it had to be wired into the -chardev arg
syntax, but from the POV of exposed QOM type information the canonical
type name has the full chardev- prefix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH 00/18] chardev: QOM cleanups
Posted by Eduardo Habkost 3 years, 6 months ago
On Fri, Sep 11, 2020 at 09:10:18AM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 11, 2020 at 12:07:27PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost <ehabkost@redhat.com>
> > wrote:
> > 
> > > Some chardev QOM cleanup patches had to be dropped from my queue
> > > due to build erros introduced by code movements across ifdef
> > > boundaries at char-parallel.c.  This series redo the changes from
> > > those patches, but the macro renames are now a little different:
> > >
> > > In this version I have decided to rename the type checking macros
> > > from *_CHARDEV to CHARDEV_* instead of renaming tye
> > > TYPE_CHARDEV_* constants to TYPE_*_CHARDEV, to make the
> > > identifiers actually match the QOM type name strings
> > > ("chardev-*").
> > >
> > 
> > Sounds reasonable to me, but it loses the matching with the
> > structure/object type name though.
> > 
> > - MuxChardev *d = MUX_CHARDEV(s);
> > + MuxChardev *d = CHARDEV_MUX(s);
> > 
> > I have a preference for the first. Unless we rename all the chardev types
> > too...
> 
> I tend to think the structs should be renamed too - I've always considerd
> them to be backwards.

FWIW, "MuxChardev" sounds better to me.  Not a big deal, though.

(Also, I am not planning to touch any struct names for the sake
of the new QOM declaration/definition macros.  Renaming the type
checking functions is enough churn.)

> 
> > Imho, the QOM type name is mostly an internal detail, the C type name is
> > dominant in the code.
> 
> Err it is the reverse. The QOM type name is exposed public API via QOM
> commands, while the C struct names are a internal private detail.

I agree with Marc-André here.  The C code is not just a detail.
Code needs be easy to read, change and refactor.  I'd really
prefer to not have the external public API forcing a specific
internal naming style.

We have at least one case where it's probably going to be
impossible to keep an exact match between the QOM type and type
checker functions: "accel"/ACCEL.
https://lore.kernel.org/qemu-devel/CAFEAcA9WEjne5TfwggVWPuBprkRs-a2-iNc43Xa_jBamaf9t8A@mail.gmail.com/

-- 
Eduardo