[libvirt] [PATCH] xml: nodedev: make pci capability class element optional

[Erik Skultety]

On Fri, Mar 22, 2019 at 08:49:22AM +0000, Nikolay Shirokovskiy wrote:
>
>
> On 21.03.2019 15:28, Erik Skultety wrote:
> > On Thu, Mar 21, 2019 at 11:49:55AM +0000, Nikolay Shirokovskiy wrote:
> >>
> >>
> >> On 21.03.2019 14:32, Erik Skultety wrote:
> >>> On Thu, Mar 21, 2019 at 10:27:14AM +0300, Nikolay Shirokovskiy wrote:
> >>>> Commit 3bd4ed46 introduced this element as required which
> >>>> breaks backcompat for test driver. Let's make the element optional.
> >>>>
> >>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> >>>> ---
> >>>>  docs/formatnode.html.in                            |  2 +-
> >>>>  docs/schemas/nodedev.rng                           | 12 ++++++-----
> >>>>  src/conf/node_device_conf.c                        | 23 +++++++++++-----------
> >>>>  src/conf/node_device_conf.h                        |  2 +-
> >>>>  src/node_device/node_device_udev.c                 |  2 +-
> >>>>  .../pci_0000_02_10_7_sriov_pf_vfs_all.xml          |  1 -
> >>>>  6 files changed, 22 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> >>>> index 5095b97..c2a8f8f 100644
> >>>> --- a/docs/formatnode.html.in
> >>>> +++ b/docs/formatnode.html.in
> >>>> @@ -71,7 +71,7 @@
> >>>>              include:
> >>>>              <dl>
> >>>>                <dt><code>class</code></dt>
> >>>> -              <dd>Combined class, subclass and
> >>>> +              <dd>Optional element for combined class, subclass and
> >>>>                  programming interface codes as 6-digit hexadecimal number.
> >>>>                  <span class="since">Since 5.2.0</span></dd>
> >>>>                <dt><code>domain</code></dt>
> >>>> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> >>>> index 0f45d79..fe6ffa0 100644
> >>>> --- a/docs/schemas/nodedev.rng
> >>>> +++ b/docs/schemas/nodedev.rng
> >>>> @@ -133,11 +133,13 @@
> >>>>        <value>pci</value>
> >>>>      </attribute>
> >>>>
> >>>> -    <element name='class'>
> >>>> -      <data type="string">
> >>>> -        <param name="pattern">0x[0-9a-fA-F]{6}</param>
> >>>> -      </data>
> >>>> -    </element>
> >>>> +    <optional>
> >>>> +      <element name='class'>
> >>>> +        <data type="string">
> >>>> +          <param name="pattern">0x[0-9a-fA-F]{6}</param>
> >>>> +        </data>
> >>>> +      </element>
> >>>> +    </optional>
> >>>>      <element name='domain'>
> >>>>        <ref name='unsignedLong'/>
> >>>>      </element>
> >>>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> >>>> index 19c601a..b96d10c 100644
> >>>> --- a/src/conf/node_device_conf.c
> >>>> +++ b/src/conf/node_device_conf.c
> >>>> @@ -208,7 +208,8 @@ virNodeDeviceCapPCIDefFormat(virBufferPtr buf,
> >>>>  {
> >>>>      size_t i;
> >>>>
> >>>> -    virBufferAsprintf(buf, "<class>0x%.6x</class>\n", data->pci_dev.klass);
> >>>> +    if (data->pci_dev.klass >= 0)
> >>>> +        virBufferAsprintf(buf, "<class>0x%.6x</class>\n", data->pci_dev.klass);
> >>>
> >>> Can 0 be a valid class? I mean semantically, can't 0 mean something like "no
> >>> class"? I'm just wondering, whether we could report 0x000000, provided it
> >>> doesn't denote a valid class already, instead of omitting the <class> element
> >>> on the output, but then again, from mgmt app POV it doesn't make any difference
> >>> to check whether the <class> element is missing or it reports 0 for devices
> >>> that have no class specified.
> >>>
> >>> Do you have some further reading on the classes? I didn't manage to find
> >>> anything that would help me decode the hex value.
> >>
> >> I found https://wiki.osdev.org/PCI. So looks like 0x000000 should be
> >> treated as 'Unclassified/Non-VGA-Compatible devices'.
> >
> > Hmm, do you know whether udev would report 0x000000 for devices which don't
> > report any class code (if there are any such devices out there) or it woudln't
> > report anything to us? In which case what your have in this patch is the only
> > right thing to do.
> >
>
> I have zero knowledge of kernel and took only a superfluos research on topic but
> thats what I learned:
>
> - kernel report what it reads from pci bus on PCI_CLASS_REVISION in sysfs uevent in PCI_CLASS var. I guess
>   errors here are only internal ones.
> - systemd's udev can return NULL from udev_device_get_property_value in case of OOMs
> - we use udevGetUintProperty wrapper on udev_device_get_property_value which leaves pci_dev->klass unchanged (so 0) on NULL from libudev
>
> So I think it is reasonable to have -1 as unknown value and fix initialization pci_dev->klass to -1 before calling wrapper udevGetUintProperty.

Yes, I agree, thanks for digging.

Erik