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