12:13 p.m.
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).