Re: [PATCH 00/18] chardev: QOM cleanups
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
> > 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_jbamaf9...@mail.gmail.com/
--
Eduardo
Re: [PATCH 00/18] chardev: QOM cleanups
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é
> 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
> > > 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
Hi
On Fri, Sep 11, 2020 at 12:10 PM 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
> > 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
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
> 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
Hi
On Thu, Sep 10, 2020 at 11:50 PM Eduardo Habkost
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
[PATCH 00/18] chardev: QOM cleanups
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
