Hi Peter,
Thanks for the quick reply.
> This is a quick note, not a full review
Ack, appreciate the early feedback.
> it can be properly split into multiple smaller chunks
I will look into splitting it into parts related to the binary parser, XML handling and docs at least.
There are some dependencies between parts but I can turn it into a set of sequential changes.
> function naming
There are some that use glib conventions with underscores (for _class_init, _init, _finalize, _get_type functions).
In this case, I followed the existing examples from virobject, vireventthread, viridentity and I assumed
that this is an exception from the camel case preference in the coding guide. Is that the case or are you talking about other issues in function naming?
> comment usage
> indentation
I will go over the patch again and fix it in places not noticed by auto checks.
> doesn't pass the test suite
Been using the CI helper to run tests in a container while doing the changes and before submitting:
But I can see that some checks have different behaviors in different distros - I will test more combinations going forward.
Best Regards,
Dmitrii Shcherbakov
LP/MM/oftc: dmitriis
On Fri, Sep 17, 2021 at 2:10 PM Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
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@xxxxxxxxxxxxx>
> ---
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
