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

Fri Sep 17 12:52:16 UTC 2021

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 <pkrempa at 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.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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210917/c0c9db72/attachment-0001.htm>