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: https://gist.github.com/dshcherb/6d8dca51d4503ec32e753f0b5434342d 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 <pkre...@redhat.com> 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.shcherba...@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 > >