Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On Fri, Oct 1, 2021 at 8:28 PM Daniel P. Berrangé 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 g 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.
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On Fri, Oct 1, 2021 at 7:49 PM Daniel P. Berrangé wrote:
> > A GTree is used as a data structure in order to maintain key ordering
> > which will be important in XML to XML tests later.
>
> Now, I've learnt a bit more about VPD and considering my comments on
> the XML format in the last patch, I think this use of GTree is
> unduly opaque and overkill
>
> I think we should be representing the data we're extracting as
> plan struct fields, in the same way that we handle SMBIOS in the
> virsysinfo.h file
>
> I feel like all of this can be reduced down to just a couple
> of public structs and a single method:
>
> typedef struct virPCIVPDResourceCustom virPCIVPDResourceCustom;
> struct virPCIVPDResourceCustom {
> char idx;
> char *value;
> };
>
> typedef struct virPCIVPDResourceRO virPCIVPDResourceRO;
> struct virPCIVPDResourceRO {
> char *part_numer;
> char *change_level;
> char *fabric_geography;
> char *location;
> char *manufcatur_id;
> char *pci_geography;
> char *serial_number;
> size_t nvendor_specific;
> virPCIVPDResourceCustom *vendor_specific;
> };
>
>
> typedef struct virPCIVPDResourceRW virPCIVPDResourceRW;
> struct virPCIVPDResourceRW {
> char *asset_tag;
> size_t nvendor_specific;
> virPCIVPDResourceCustom *vendor_specific;
> size_t nsystem_specific;
> virPCIVPDResourceCustom *system_specific;
> };
>
> typedef struct virPCIVPDResource virPCIVPDResource;
> struct virPCIVPDResource {
> char *name;
> virPCIVPDResourceRO *ro;
> virPCIVPDResourceRW *rw;
> }
>
> virPCIVPDResource *virPCIVPDParse(int vpdFileFd);
> void virPCIVPDResourceFree(virPCIVPDResource *res);
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResource, virPCIVPDResourceFree);
>
Ack, I can rework it as you describe.
There will be a bit more work to do in XML serialization and
deserialization code to map struct fields to keyword names and vice
versa but I'll see what I can come up with.
My original goal with using Glib data structures was to use standard
iteration, insertion and lookup operations and also simplify automatic
cleanup implementation. On the other hand, there are some specifics of
working with them as my workaround for a bug with derived types shows.
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
> > +if (VIR_CLOSE(vpdFileFd) < 0) {
> > +virReportSystemError(errno, _("Unable to close the VPD file, fd:
> > %d"), vpdFileFd);
> > +return NULL;
> > +}
>
> This is closing an FD that is owned & passed in by the caller. I'd
> consider that an undesirable pattern. Whomever opens an FD should
> generally take responsiiblity for closing it too, as that gives
> clear semantics on state of the FD, when this method returns an
> error state.
Makes sense, I'll rework it to have the FD closed in caller functions.
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
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. > > > +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 g 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. > 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: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
Thanks a lot for the review!
Responses inline - ACKs to address in v6.
On Fri, Oct 1, 2021 at 12:58 PM Daniel P. Berrangé wrote:
> I don't know what the thread concurrency rules are of the callers of
> this code, but regardless we generally aim to make any one-time
> initializers thread safe by default.
>
> Recommend using VIR_ONCE_GLOBAL_INIT to do one-time init.
>
Ack, will address in v6.
> IIUC, this hash table is created once and never changed. I'm
> not seeing a clear need for g_strdup here. Can't we just
> directly use the constant string as the key ?
Good point, no need to store copies of constant strings here.
> > +if (!g_regex_match_simple("^[a-zA-Z0-9\\-_\\s,.:=]*$", value, 0, 0)) {
>
> Since you're only trying to validate a set of permitted characters,
> it is sufficient to use strspn() for this task.
>
> eg what we do in vsh.c is
>
> /* Characters permitted in $EDITOR environment variable and temp filename.
> */
> #define ACCEPTED_CHARS \
> "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-/_.:@"
>
> if (strspn(editor, ACCEPTED_CHARS) != strlen(editor)) {
>error...
Ack, seems like a cheaper way to do this than with regex.
> > +G_BEGIN_DECLS;
>
> This is not required in libvirt internal code, since we never use C++
> internally.
Ack, will remove in v6.
> > +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.
Ack, will address in v6.
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
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-XX";
+/* 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).
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
>
> The VPD format is specified in "I.3. VPD Definitions" in PCI specs
> (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
> notes, the PCI Local Bus and PCIe VPD formats are binary compatible
> and PCIe 4.0 merely started incorporating what was already present in
> PCI specs.
>
> Linux kernel exposes a binary blob in the VPD format via sysfs since
> v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
> a parser to interpret.
>
> A GTree is used as a data structure in order to maintain key ordering
> which will be important in XML to XML tests later.
Now, I've learnt a bit more about VPD and considering my comments on
the XML format in the last patch, I think this use of GTree is
unduly opaque and overkill
I think we should be representing the data we're extracting as
plan struct fields, in the same way that we handle SMBIOS in the
virsysinfo.h file
> diff --git a/src/util/virpcivpd.h b/src/util/virpcivpd.h
> new file mode 100644
> index 00..7327806df3
> --- /dev/null
> +++ b/src/util/virpcivpd.h
> +
> +#include "internal.h"
> +
> +/*
> + * PCI Local bus (2.2+, Appendix I) and PCIe 4.0+ (7.9.19 VPD Capability)
> define
> + * the VPD capability structure (8 bytes in total) and VPD registers that
> can be used to access
> + * VPD data including:
> + * bit 31 of the first 32-bit DWORD: data transfer completion flag (between
> the VPD data register
> + * and the VPD data storage hardware);
> + * bits 30:16 of the first 32-bit DWORD: VPD address of the first VPD data
> byte to be accessed;
> + * bits 31:0 of the second 32-bit DWORD: VPD data bytes with LSB being
> pointed to by the VPD address.
> + *
> + * Given that only 15 bits (30:16) are allocated for VPD address its mask is
> 0x7fff.
> +*/
> +#define PCI_VPD_ADDR_MASK 0x7FFF
> +
> +/*
> + * VPD data consists of small and large resource data types. Information
> within a resource type
> + * consists of a 2-byte keyword, 1-byte length and data bytes (up to 255).
> +*/
> +#define PCI_VPD_MAX_FIELD_SIZE 255
> +#define PCI_VPD_LARGE_RESOURCE_FLAG 0x80
> +#define PCI_VPD_STRING_RESOURCE_FLAG 0x02
> +#define PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG 0x10
> +#define PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG 0x11
> +#define PCI_VPD_RESOURCE_END_TAG 0x0F
> +#define PCI_VPD_RESOURCE_END_VAL PCI_VPD_RESOURCE_END_TAG << 3
> +#define VIR_TYPE_PCI_VPD_RESOURCE_TYPE vir_pci_vpd_resource_type_get_type()
> +
> +G_BEGIN_DECLS;
> +
> +typedef enum {
> +VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT = 1,
> +VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY,
> +VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD,
> +VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR,
> +VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST
> +} virPCIVPDResourceFieldValueFormat;
> +
> +typedef enum {
> +VIR_PCI_VPD_RESOURCE_TYPE_STRING = 1,
> +VIR_PCI_VPD_RESOURCE_TYPE_VPD_R,
> +VIR_PCI_VPD_RESOURCE_TYPE_VPD_W,
> +VIR_PCI_VPD_RESOURCE_TYPE_LAST
> +} virPCIVPDResourceType;
> +
> +GType vir_pci_vpd_resource_type_get_type(void);
> +
> +virPCIVPDResourceFieldValueFormat virPCIVPDResourceGetFieldValueFormat(const
> gchar *value);
> +
> +#define VIR_TYPE_PCI_VPD_RESOURCE vir_pci_vpd_resource_get_type()
> +G_DECLARE_DERIVABLE_TYPE(virPCIVPDResource, vir_pci_vpd_resource, VIR,
> PCI_VPD_RESOURCE, GObject);
> +struct _virPCIVPDResourceClass {
> +GObjectClass parent_class;
> +};
> +
> +/* Glib 2.59 adds proper support for g_autolist for derivable types
> + * see 86c073dba9d82ef3f1bc3d3116b058b9b5c3b1eb. At the time of writing
> + * 2.56 is the minimum version used by Libvirt. Without the below code
> + * compilation errors will occur.
> + */
> +#if !GLIB_CHECK_VERSION(2, 59, 0)
> +typedef GList *virPCIVPDResource_listautoptr;
> +static inline void
> +glib_listautoptr_cleanup_virPCIVPDResource(GList **_l)
> +{
> +g_list_free_full(*_l, (GDestroyNotify)g_object_unref);
> +}
> +#endif
> +
> +GEnumValue *virPCIVPDResourceGetResourceType(virPCIVPDResource *res);
> +
> +#define VIR_TYPE_PCI_VPD_STRING_RESOURCE
> vir_pci_vpd_string_resource_get_type()
> +G_DECLARE_FINAL_TYPE(virPCIVPDStringResource, vir_pci_vpd_string_resource,
> VIR,
> + PCI_VPD_STRING_RESOURCE, virPCIVPDResource);
> +
> +struct _virPCIVPDStringResourceClass {
> +virPCIVPDResourceClass parent_class;
> +};
> +
> +virPCIVPDStringResource *virPCIVPDStringResourceNew(gchar *value);
> +
> +const gchar *virPCIVPDStringResourceGetValue(const virPCIVPDStringResource
> *res);
> +
> +#define VIR_TYPE_PCI_VPD_KEYWORD_RESOURCE
> vir_pci_vpd_keyword_resource_get_type()
> +G_DECLARE_FINAL_TYPE(virPCIVPDKeywordResource, vir_pci_vpd_keyword_resource,
> VIR,
> + PCI_VPD_KEYWORD_RESOURCE, virPCIVPDResource);
> +virPCIVPDKeywordResource *virPCIVPDKeywordResourceNew(GTree
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
>
> The VPD format is specified in "I.3. VPD Definitions" in PCI specs
> (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
> notes, the PCI Local Bus and PCIe VPD formats are binary compatible
> and PCIe 4.0 merely started incorporating what was already present in
> PCI specs.
>
> Linux kernel exposes a binary blob in the VPD format via sysfs since
> v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
> a parser to interpret.
>
> A GTree is used as a data structure in order to maintain key ordering
> which will be important in XML to XML tests later.
>
> Signed-off-by: Dmitrii Shcherbakov
> ---
> build-aux/syntax-check.mk | 4 +-
> po/POTFILES.in| 1 +
> src/libvirt_private.syms | 15 +
> src/util/meson.build | 1 +
> src/util/virpcivpd.c | 755 ++
> src/util/virpcivpd.h | 117 ++
> src/util/virpcivpdpriv.h | 45 +++
> tests/meson.build | 1 +
> tests/testutils.c | 40 ++
> tests/testutils.h | 4 +
> tests/virpcivpdtest.c | 705 +++
> 11 files changed, 1686 insertions(+), 2 deletions(-)
> create mode 100644 src/util/virpcivpd.c
> create mode 100644 src/util/virpcivpd.h
> create mode 100644 src/util/virpcivpdpriv.h
> create mode 100644 tests/virpcivpdtest.c
> +/**
> + * virPCIVPDParse:
> + * @vpdFileFd: a file descriptor associated with a file containing PCI
> device VPD.
> + *
> + * Parse a PCI device's Vital Product Data (VPD) contained in a file
> descriptor.
> + *
> + * Returns: a pointer to a GList of VPDResource types which needs to be
> freed by the caller or
> + * NULL if getting it failed for some reason.
> + */
> +GList *
> +virPCIVPDParse(int vpdFileFd)
> +{
> +/* A checksum which is calculated as a sum of all bytes from VPD byte 0
> up to
> + * the checksum byte in the RV field's value. The RV field is only
> present in the
> + * VPD-R resource and the checksum byte there is the first byte of the
> field's value.
> + * The checksum byte in RV field is actually a two's complement of the
> sum of all bytes
> + * of VPD that come before it so adding the two together must produce 0
> if data
> + * was not corrupted and VPD storage is intact.
> + */
> +uint8_t csum = 0;
> +uint8_t headerBuf[2];
> +
> +g_autolist(virPCIVPDResource) resList = NULL;
> +uint16_t resPos = 0, resDataLen;
> +uint8_t tag = 0;
> +bool endResReached = false, hasReadOnlyRes = false;
> +
> +g_autoptr(virPCIVPDResource) res = NULL;
> +
> +while (resPos <= PCI_VPD_ADDR_MASK) {
> +/* Read the resource data type tag. */
> +if (virPCIVPDReadVPDBytes(vpdFileFd, , 1, resPos, ) != 1) {
> +break;
> +}
> +/* 0x80 == 0b1000 - the large resource data type flag. */
> +if (tag & PCI_VPD_LARGE_RESOURCE_FLAG) {
> +if (resPos > PCI_VPD_ADDR_MASK + 1 - 3) {
> +/* Bail if the large resource starts at the position where
> the end tag should be. */
> +break;
> +}
> +/* Read the two length bytes of the large resource record. */
> +if (virPCIVPDReadVPDBytes(vpdFileFd, headerBuf, 2, resPos + 1,
> ) != 2) {
> +break;
> +}
> +resDataLen = headerBuf[0] + (headerBuf[1] << 8);
> +/* Change the position to the byte following the tag and length
> bytes. */
> +resPos += 3;
> +} else {
> +/* Handle a small resource record.
> + * 0yyy & 0111, where - resource data type bits,
> yyy - length bits. */
> +resDataLen = tag & 7;
> +/* 0yyy >> 3 == */
> +tag >>= 3;
> +/* Change the position to the byte past the byte containing tag
> and length bits. */
> +resPos += 1;
> +}
> +if (tag == PCI_VPD_RESOURCE_END_TAG) {
> +/* Stop VPD traversal since the end tag was encountered. */
> +endResReached = true;
> +break;
> +}
> +if (resDataLen > PCI_VPD_ADDR_MASK + 1 - resPos) {
> +/* Bail if the resource is too long to fit into the VPD address
> space. */
> +break;
> +}
> +
> +switch (tag) {
> +/* Large resource type which is also a string: 0x80 | 0x02 =
> 0x82 */
> +case PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG:
> +res =
> +(virPCIVPDResource *)
> virPCIVPDParseVPDLargeResourceString(vpdFileFd, resPos,
> +
> resDataLen, );
> +break;
> +/*
Re: [libvirt PATCH v5 1/7] Add a PCI/PCIe device VPD Parser
On Mon, Sep 27, 2021 at 10:30:47PM +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and storing
> results in memory.
>
> The VPD format is specified in "I.3. VPD Definitions" in PCI specs
> (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0
> notes, the PCI Local Bus and PCIe VPD formats are binary compatible
> and PCIe 4.0 merely started incorporating what was already present in
> PCI specs.
>
> Linux kernel exposes a binary blob in the VPD format via sysfs since
> v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires
> a parser to interpret.
>
> A GTree is used as a data structure in order to maintain key ordering
> which will be important in XML to XML tests later.
>
> Signed-off-by: Dmitrii Shcherbakov
> ---
> build-aux/syntax-check.mk | 4 +-
> po/POTFILES.in| 1 +
> src/libvirt_private.syms | 15 +
> src/util/meson.build | 1 +
> src/util/virpcivpd.c | 755 ++
> src/util/virpcivpd.h | 117 ++
> src/util/virpcivpdpriv.h | 45 +++
> tests/meson.build | 1 +
> tests/testutils.c | 40 ++
> tests/testutils.h | 4 +
> tests/virpcivpdtest.c | 705 +++
> 11 files changed, 1686 insertions(+), 2 deletions(-)
> create mode 100644 src/util/virpcivpd.c
> create mode 100644 src/util/virpcivpd.h
> create mode 100644 src/util/virpcivpdpriv.h
> create mode 100644 tests/virpcivpdtest.c
>
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index cb54c8ba36..a428831380 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -775,9 +775,9 @@ sc_prohibit_windows_special_chars_in_filename:
> { echo '$(ME): Windows special chars in filename not allowed' 1>&2;
> echo exit 1; } || :
>
> sc_prohibit_mixed_case_abbreviations:
> - @prohibit='Pci|Usb|Scsi' \
> + @prohibit='Pci|Usb|Scsi|Vpd' \
> in_vc_files='\.[ch]$$' \
> - halt='Use PCI, USB, SCSI, not Pci, Usb, Scsi' \
> + halt='Use PCI, USB, SCSI, VPD, not Pci, Usb, Scsi, Vpd' \
> $(_sc_search_regexp)
>
> # Require #include in all files that call setlocale()
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index c200d7452a..4be4139529 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -302,6 +302,7 @@
> @SRCDIR@src/util/virnvme.c
> @SRCDIR@src/util/virobject.c
> @SRCDIR@src/util/virpci.c
> +@SRCDIR@src/util/virpcivpd.c
> @SRCDIR@src/util/virperf.c
> @SRCDIR@src/util/virpidfile.c
> @SRCDIR@src/util/virpolkit.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6de9d9aef1..bb9b380599 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3571,6 +3571,21 @@ virVHBAManageVport;
> virVHBAPathExists;
>
>
> +# util/virpcivpd.h
> +
> +virPCIVPDKeywordResourceForEach;
> +virPCIVPDKeywordResourceNew;
> +virPCIVPDParse;
> +virPCIVPDParseVPDLargeResourceFields;
> +virPCIVPDParseVPDLargeResourceString;
> +virPCIVPDReadVPDBytes;
> +virPCIVPDResourceGetFieldValueFormat;
> +virPCIVPDResourceGetResourceType;
> +virPCIVPDResourceIsExpectedKeyword;
> +virPCIVPDResourceIsValidTextValue;
> +virPCIVPDStringResourceGetValue;
> +virPCIVPDStringResourceNew;
> +
> # util/virvsock.h
> virVsockAcquireGuestCid;
> virVsockSetGuestCid;
> diff --git a/src/util/meson.build b/src/util/meson.build
> index 05934f6841..24350a3e67 100644
> --- a/src/util/meson.build
> +++ b/src/util/meson.build
> @@ -105,6 +105,7 @@ util_sources = [
>'virutil.c',
>'viruuid.c',
>'virvhba.c',
> + 'virpcivpd.c',
>'virvsock.c',
>'virxml.c',
> ]
> diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c
> new file mode 100644
> index 00..849ea0570c
> --- /dev/null
> +++ b/src/util/virpcivpd.c
> @@ -0,0 +1,755 @@
> +/*
> + * virpcivpd.c: helper APIs for working with the PCI/PCIe VPD capability
> + *
> + * Copyright (C) 2021 Canonical Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + *
