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

[Laine Stump]

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.

I do see some of the examples you pointed out in your reply, so 
definitely there is precedent. Personally when I see a function with 
underscores in its name, my subconscious immediately thinks (before 
looking at the words in the name) that they must be from an external 
library somewhere. My preference would be to have all functions defined 
within libvirt follow libvirt's naming convention (yeah, I know there's 
lots of stuff that doesn't!), but I may be an outlier here though 
(especially since at least some of your examples, e.g. 
vir_object_init(), was authored by danpb, who also hasn't complained 
about your choices for names, so...)

So this is more a question for the rest of longtime libvirt developers 
rather than you - do we care about this? If so, exactly what is the policy?

>> +{
>> +    static GType resourceType;
>> +
>> +    static const GEnumValue resourceTypes[] = {
>> +        {VIR_PCI_VPD_RESOURCE_TYPE_STRING, "String resource", "string"},
>> +        {VIR_PCI_VPD_RESOURCE_TYPE_VPD_R, "Read-only resource", "vpd-r"},
>> +        {VIR_PCI_VPD_RESOURCE_TYPE_VPD_W, "Read-write resource", "vpd-w"},
>> +        {VIR_PCI_VPD_RESOURCE_TYPE_LAST, "last", "last"},
>> +        {0, NULL, NULL},
>> +    };
>> +
>> +    if (!resourceType) {
>> +        resourceType = g_enum_register_static("virPCIVPDResourceType", resourceTypes);
>> +    }
>> +    return resourceType;
>> +}
>> +
>> +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?

I don't have a strong opinion except that I think consistency is good 
(otherwise people will spend time trying to decide which one to use in 
every case), and now is a better time than change the types than later, 
if that's what people want.

(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!)

>> +virPCIVPDResourceIsVendorKeyword(const gchar *keyword)


I similarly wonder about use of gchar rather than char when it's not 
required for type compatibility with glib APIs. So I guess basically I'm 
wondering just how far down the glib rabbit hole we aim to go :-)


>> +
>> +G_BEGIN_DECLS;
 >
 > This is not required in libvirt internal code, since we never use C++
 > internally.
 >

Note to Daniel: I'm glad you gave up on waiting for me to review these 
patches, because I'm not conversant enough with glib to have caught this 
(and also would have missed the whole thing about the unnecessary 
strduping of hash keys).

> 
> 
>> +#ifdef __linux__
>> +/**
>> + * virCreateAnonymousFile:
>> + * @data: a pointer to data to be written into a new file.
>> + * @len: the length of data to be written (in bytes).
>> + *
>> + * Create a fake fd, write initial data to it.
>> + *
>> + */
>> +int
>> +virCreateAnonymousFile(const uint8_t *data, size_t len)
>> +{
>> +    int fd = -1;
>> +    char path[] = abs_builddir "testutils-memfd-XXXXXX";
>> +    /* A temp file is used since not all supported distributions support memfd. */
>> +    if ((fd = g_mkstemp_full(path, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
>> +        return fd;
>> +    }
>> +    g_unlink(path);
>> +
>> +    if (ftruncate(fd, len) != 0) {
>> +        VIR_TEST_DEBUG("%s: %s", "failed to ftruncate an anonymous file",
>> +                g_strerror(errno));
>> +        goto cleanup;
>> +    }
> 
> Since it is a new temporary file it is guaranteed zero length, so
> this should be redundant AFAICT.

I would've missed this too, not due to unfamiliarity, but just that I'd 
be too busy looking for non-standard names and leaked pointers to pay 
attention. (Okay, I'll stop embarrassing myself now).