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

Nikolay Shirokovskiy posted 1 patch 4 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1553153234-427807-1-git-send-email-nshirokovskiy@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(-)

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

Posted by Nikolay Shirokovskiy 4 weeks ago
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@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);
     virBufferAsprintf(buf, "<domain>%d</domain>\n",
                       data->pci_dev.domain);
     virBufferAsprintf(buf, "<bus>%d</bus>\n", data->pci_dev.bus);
@@ -1645,16 +1646,16 @@ virNodeDevCapPCIDevParseXML(xmlXPathContextPtr ctxt,
     orignode = ctxt->node;
     ctxt->node = node;
 
-    if (virNodeDevCapsDefParseHexId("string(./class[1])", ctxt,
-                                    &pci_dev->klass, def,
-                                    _("no PCI class supplied for '%s'"),
-                                    _("invalid PCI class supplied for '%s'")) < 0)
-        goto out;
-
-    if (pci_dev->klass > 0xffffff) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("invalid PCI class supplied for '%s'"), def->name);
-        goto out;
+    if ((tmp = virXPathString("string(./class[1])", ctxt))) {
+        if (virStrToLong_i(tmp, NULL, 16, &pci_dev->klass) < 0 ||
+            pci_dev->klass > 0xffffff) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("invalid PCI class supplied for '%s'"), def->name);
+            goto out;
+        }
+        VIR_FREE(tmp);
+    } else {
+        pci_dev->klass = -1;
     }
 
     if (virNodeDevCapsDefParseULong("number(./domain[1])", ctxt,
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index b13bc13..e8cb315 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -153,7 +153,7 @@ struct _virNodeDevCapPCIDev {
     unsigned int function;
     unsigned int product;
     unsigned int vendor;
-    unsigned int klass;
+    int klass;
     char *product_name;
     char *vendor_name;
     virPCIDeviceAddressPtr physical_function;
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index f0e61e4..5ed3463 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -402,7 +402,7 @@ udevProcessPCI(struct udev_device *device,
     privileged = driver->privileged;
     nodeDeviceUnlock();
 
-    if (udevGetUintProperty(device, "PCI_CLASS", &pci_dev->klass, 16) < 0)
+    if (udevGetIntProperty(device, "PCI_CLASS", &pci_dev->klass, 16) < 0)
         goto cleanup;
 
     if ((p = strrchr(def->sysfs_path, '/')) == NULL ||
diff --git a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml
index 74036a6..9e8dace 100644
--- a/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml
+++ b/tests/nodedevschemadata/pci_0000_02_10_7_sriov_pf_vfs_all.xml
@@ -2,7 +2,6 @@
   <name>pci_0000_02_10_7</name>
   <parent>pci_0000_00_04_0</parent>
   <capability type='pci'>
-    <class>0xffffff</class>
     <domain>0</domain>
     <bus>2</bus>
     <slot>16</slot>
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 4 weeks ago
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@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.

The change makes sense regardless, so:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

...but I'm still curious

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Nikolay Shirokovskiy 4 weeks ago

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@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'.

Nikolay

> 
> The change makes sense regardless, so:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 
> ...but I'm still curious
> 
> Erik
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 4 weeks ago
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@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.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Nikolay Shirokovskiy 4 weeks ago

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@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.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 4 weeks ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Nikolay Shirokovskiy 4 weeks ago

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@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 need to take a look on udev/kernel code to answer the question...

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list