[libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser

[Dmitrii Shcherbakov]

On Fri, Oct 1, 2021 at 8:28 PM Daniel P. Berrangé <berrange at redhat.com> wrote:
>
> On Fri, Oct 01, 2021 at 01:13:00PM -0400, Laine Stump wrote:
> > On 10/1/21 5:57 AM, Daniel P. Berrangé wrote:
> > > On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> >
> > [...]
> >
> > > +GType
> > > > +vir_pci_vpd_resource_type_get_type(void)
> >
> > I know you had asked about using under_scored_naming in a reply to Peter
> > pointing out "non-standard" names in V3 of your patches, but I don't think
> > anyone followed up on that.
>
> That very specific case is something that is required when we use
> GObject for the type. see util/viridentity.h for the same scenario.
> There are a couple of other similar requirements force on us by
> GObject in this regard, but aside from that we should follow
> normal libvirt naming practice.

Yes, those names are enforced in the glib macros:

https://github.com/GNOME/glib/blob/2.53.7/gobject/gtype.h#L1396
GType module_obj_name##_get_type (void);
                                \

https://github.com/GNOME/glib/blob/2.53.7/gobject/gtype.h#L1949-L1951
static void     type_name##_init              (TypeName        *self); \
static void     type_name##_class_init        (TypeName##Class *klass); \

So there isn't much choice.

But Peter was right about property getters/setters and the finalize
function - for those glib merely needs function pointers and Libvirt
naming conventions can be used there.

>
> > > > +static gboolean
> >
> > In a similar "coding standards" vein - other uses of "gboolean" (rather than
> > the much more common "bool", or *very rarely* "boolean") in our code seem to
> > be the product of auto-generated(?) xdr headers for wireshark, functions
> > that are registered as callbacks to glib (and so must follow the types in
> > the callback function pointer prototype), and a very small number of other
> > cases. Do we want new code using gboolean rather than bool in these "other"
> > cases that don't require gboolean for proper glib type compatibility?
>
> gboolean is a completely differnt sized type from bool.
>
> As a general rule we want to use 'bool' + true/false, except
> where we must have strict type compatibility with glib. The
> main scenario that matters is callbacks integrating with
> GLib's event loop or similar.
>
> All the other gNNNN basic types are essentially identical
> to stdint.h types, so there's no compelling reason to use
> them. We can use the plain C types, or the stdint.h sized
> types as appropriate and they'll be fully compatible with
> any GLib APIs.
>
> > (a side-nit completely unrelated to your patches - I noticed that in at
> > least a couple places in libvirt, a gboolean is assigned "true" or "false",
> > although the glib documentation says that it should only have the value
> > "TRUE" or "FALSE". Good thing those happen to use the same values!)
>
> We're lucky in that the constants do the right thing if
> you assign/compare. eg a
>
>    bool foo= TRUE;
>    if (foo == TRUE)
>       ..
>
> will be ok.
>
> I'm not so sure about
>
>    bool foo = TRUE
>    gboolean bar == TRUE;
>    if (foo == bar)
>       ...
>
> because they're different types, and so when you size-extend
> I think they end up with different values, despite both being
> TRUE.
>

Thanks a lot for raising and discussing this - I was confused as to
which types to preferably use as well.