<div dir="ltr"><div>Hi Peter,</div><div dir="ltr"><br></div><div>Thanks for the quick reply.</div><div dir="ltr"><br></div><div dir="ltr">> This is a quick note, not a full review</div><div dir="ltr"><br></div><div dir="ltr">Ack, appreciate the early feedback.</div><div dir="ltr"><br></div><div dir="ltr">> it can be properly split into multiple smaller chunks</div><div dir="ltr"><br></div><div dir="ltr">I will look into splitting it into parts related to the binary parser, XML handling and docs at least.<br></div><div dir="ltr"><br></div><div>There are some dependencies between parts but I can turn it into a set of sequential changes.</div><div dir="ltr"><br></div><div dir="ltr">> function naming</div><div dir="ltr"><br></div><div>There are some that use glib conventions with underscores (for _class_init, _init, _finalize, _get_type functions).</div><div><br></div><div>In this case, I followed the existing examples from virobject, vireventthread, viridentity and I assumed</div><div>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?</div><div><br></div><div>> comment usage</div><div>> indentation</div><div><br></div><div>I will go over the patch again and fix it in places not noticed by auto checks.</div><div><br></div><div>> doesn't pass the test suite<br></div><div><br></div><div>Been using the CI helper to run tests in a container while doing the changes and before submitting:</div><div><a href="https://gist.github.com/dshcherb/6d8dca51d4503ec32e753f0b5434342d">https://gist.github.com/dshcherb/6d8dca51d4503ec32e753f0b5434342d</a><br></div><div><br></div><div>But I can see that some checks have different behaviors in different distros - I will test more combinations going forward.</div><div dir="ltr"><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Best Regards,<div>Dmitrii Shcherbakov</div><div><span style="color:rgb(136,136,136);font-size:12.8px">LP/MM/oftc: dmitriis</span><br></div></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 17, 2021 at 2:10 PM Peter Krempa <<a href="mailto:pkrempa@redhat.com">pkrempa@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Sep 17, 2021 at 13:54:49 +0300, Dmitrii Shcherbakov wrote:<br> > Add support for deserializing the binary PCI/PCIe VPD format and<br> > exposing VPD resources as XML elements in a new nested capability<br> > of PCI/PCIe devices called 'vpd'.<br> > <br> > The VPD format is specified in "I.3. VPD Definitions" in PCI specs<br> > (2.2+) and "6.28.1 VPD Format" PCIe 4.0. As section 6.28 in PCIe 4.0<br> > notes, the PCI Local Bus and PCIe VPD formats are binary compatible<br> > and PCIe 4.0 merely started incorporating what was already present in<br> > PCI specs.<br> > <br> > Linux kernel exposes a binary blob in the VPD format via sysfs since<br> > v2.6.26 (commit 94e6108803469a37ee1e3c92dafdd1d59298602f) which requires<br> > a parser to interpret.<br> > <br> > Signed-off-by: Dmitrii Shcherbakov <<a href="mailto:dmitrii.shcherbakov@canonical.com" target="_blank">dmitrii.shcherbakov@canonical.com</a>><br> > ---<br> <br> This is a quick note, not a full review:<br> <br> > build-aux/<a href="http://syntax-check.mk" rel="noreferrer" target="_blank">syntax-check.mk</a> | 4 +-<br> > docs/<a href="http://drvnodedev.html.in" rel="noreferrer" target="_blank">drvnodedev.html.in</a> | 46 ++<br> > docs/<a href="http://formatnode.html.in" rel="noreferrer" target="_blank">formatnode.html.in</a> | 24 +-<br> > docs/schemas/nodedev.rng | 40 +<br> > include/libvirt/libvirt-nodedev.h | 1 +<br> > po/POTFILES.in | 1 +<br> > src/conf/node_device_conf.c | 258 ++++++<br> > src/conf/node_device_conf.h | 6 +-<br> > src/conf/virnodedeviceobj.c | 7 +-<br> > src/libvirt_private.syms | 17 +<br> > src/node_device/node_device_driver.c | 2 +<br> > src/node_device/node_device_udev.c | 2 +<br> > src/util/meson.build | 1 +<br> > src/util/virpci.c | 60 ++<br> > src/util/virpci.h | 3 +<br> > src/util/virpcivpd.c | 771 +++++++++++++++++<br> > src/util/virpcivpd.h | 106 +++<br> > src/util/virpcivpdpriv.h | 42 +<br> > tests/meson.build | 1 +<br> > .../pci_0000_42_00_0_vpd.xml | 33 +<br> > .../pci_0000_42_00_0_vpd.xml | 1 +<br> > tests/nodedevxml2xmltest.c | 1 +<br> > tests/testutils.c | 51 ++<br> > tests/testutils.h | 6 +<br> > tests/virpcitest.c | 3 +<br> > tests/virpcivpdtest.c | 777 ++++++++++++++++++<br> > tools/virsh-nodedev.c | 3 +<br> > 27 files changed, 2262 insertions(+), 5 deletions(-)<br> > create mode 100644 src/util/virpcivpd.c<br> > create mode 100644 src/util/virpcivpd.h<br> > create mode 100644 src/util/virpcivpdpriv.h<br> > create mode 100644 tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml<br> > create mode 120000 tests/nodedevxml2xmlout/pci_0000_42_00_0_vpd.xml<br> > create mode 100644 tests/virpcivpdtest.c<br> <br> Firstly your patch is massive and it looks to me as it can be properly<br> split into multiple smaller chunks as our contributor guidelines ask<br> for. [1]<br> <br> Additionally few of the functions I had a look at don't conform to our<br> coding standard at least in term of comment usage, function naming and<br> indentation.<br> <br> Lastly it doesn't pass the test suite (ninja test). Note that once<br> you'll split the patch, the testsuite must pass after every commit.<br> <br> [1] <a href="https://www.libvirt.org/hacking.html" rel="noreferrer" target="_blank">https://www.libvirt.org/hacking.html</a><br> <a href="https://www.libvirt.org/best-practices.html" rel="noreferrer" target="_blank">https://www.libvirt.org/best-practices.html</a><br> <a href="https://www.libvirt.org/coding-style.html" rel="noreferrer" target="_blank">https://www.libvirt.org/coding-style.html</a><br> <br> </blockquote></div></div>