Re: [PATCH v3 01/14] hw/char/serial-pci: Replace DO_UPCAST(PCISerialState) by PCI_SERIAL()

[BALATON Zoltan]

On Thu, 16 Feb 2023, Philippe Mathieu-Daudé wrote:
> On 16/2/23 12:20, Thomas Huth wrote:
> > On 13/02/2023 19.43, Philippe Mathieu-Daudé wrote:
> > > Use the PCI_SERIAL() QOM type-checking macro to avoid DO_UPCAST().
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/char/serial-pci.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
> > > index 801b769aba..9689645cac 100644
> > > --- a/hw/char/serial-pci.c
> > > +++ b/hw/char/serial-pci.c
> > > @@ -36,7 +36,10 @@
> > >   #include "qom/object.h"
> > >   struct PCISerialState {
> > > +    /*< private >*/
> > >       PCIDevice dev;
> > > +    /*< public >*/
> > > +
> >
> > I'm not sure about this part of the patch. It does not seem to be related 
> > to the other changes at all, and are you sure about which parts are really 
> > "public" and which parts are "private"? If so, I'd like to see a 
> > description about this in the commit message, preferably in a separate 
> > patch. Also, why an empty line after the "public" comment?
>
> This is how QOM style separates the object 'private' part -- the
> inherited parent, used by the QOM-cast macros -- and the fields
> specific to this object.
> The private field *must* be the first one in the structure for the
> cast macros to work.
>
> Maybe this isn't a convention and we could make one, to unify the
> API style. I'm open to better suggestion :)
>
> I suppose I got custom to see it to distinct the QOM hierarchy and
> now it helps me to detect what is QOM and what isn't.
> Anyway I'll remove from this patch.

I also dislike these comments and empty lines in these struct definitions. 
I think it should be enough to document this QOM convention in the docs 
saying that each QOM object state has to have it's parent's state as first 
member and you're not supposed to access it directly (except maybe from 
very closely related sub class) but do a QOM cast instead. If this is 
clearly stated in the docs then there's no need to add comments about this 
in every object. You could tell QOM objects from other structs by the 
first member also being a QOM object and usually called parent or similar 
but sometimes just dev. If you really want to get fancy maybe you could 
hide it in a macro, something like:

OBJECT_STATE(PCISerialState, PCIDevice)
...
END_OBJECT_STATE

but I'm not sure I like that because it has to hide the braces in the 
macro so it's not clear it's just a struct. So just describing it in the 
docs if it's not already is probably enough.

Regards,
BALATON Zoltan