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(a)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.