[libvirt PATCH v3 1/1] Add a PCI/PCIe device VPD Capability

[Peter Krempa]

On Fri, Sep 17, 2021 at 13:54:49 +0300, Dmitrii Shcherbakov wrote:
> Add support for deserializing the binary PCI/PCIe VPD format and
> exposing VPD resources as XML elements in a new nested capability
> of PCI/PCIe devices called 'vpd'.
> 
> 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.
> 
> Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov at canonical.com>
> ---

This is a quick note, not a full review:

>  build-aux/syntax-check.mk                     |   4 +-
>  docs/drvnodedev.html.in                       |  46 ++
>  docs/formatnode.html.in                       |  24 +-
>  docs/schemas/nodedev.rng                      |  40 +
>  include/libvirt/libvirt-nodedev.h             |   1 +
>  po/POTFILES.in                                |   1 +
>  src/conf/node_device_conf.c                   | 258 ++++++
>  src/conf/node_device_conf.h                   |   6 +-
>  src/conf/virnodedeviceobj.c                   |   7 +-
>  src/libvirt_private.syms                      |  17 +
>  src/node_device/node_device_driver.c          |   2 +
>  src/node_device/node_device_udev.c            |   2 +
>  src/util/meson.build                          |   1 +
>  src/util/virpci.c                             |  60 ++
>  src/util/virpci.h                             |   3 +
>  src/util/virpcivpd.c                          | 771 +++++++++++++++++
>  src/util/virpcivpd.h                          | 106 +++
>  src/util/virpcivpdpriv.h                      |  42 +
>  tests/meson.build                             |   1 +
>  .../pci_0000_42_00_0_vpd.xml                  |  33 +
>  .../pci_0000_42_00_0_vpd.xml                  |   1 +
>  tests/nodedevxml2xmltest.c                    |   1 +
>  tests/testutils.c                             |  51 ++
>  tests/testutils.h                             |   6 +
>  tests/virpcitest.c                            |   3 +
>  tests/virpcivpdtest.c                         | 777 ++++++++++++++++++
>  tools/virsh-nodedev.c                         |   3 +
>  27 files changed, 2262 insertions(+), 5 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/nodedevschemadata/pci_0000_42_00_0_vpd.xml
>  create mode 120000 tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml
>  create mode 100644 tests/virpcivpdtest.c

Firstly your patch is massive and it looks to me as it can be properly
split into multiple smaller chunks as our contributor guidelines ask
for. [1]

Additionally few of the functions I had a look at don't conform to our
coding standard at least in term of comment usage, function naming and
indentation.

Lastly it doesn't pass the test suite (ninja test). Note that once
you'll split the patch, the testsuite must pass after every commit.

[1] https://www.libvirt.org/hacking.html
    https://www.libvirt.org/best-practices.html
    https://www.libvirt.org/coding-style.html