[libvirt] [PATCH] xml: nodedev: make pci capability class element optional
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
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
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
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
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
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
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
participants (2)
-
Erik Skultety -
Nikolay Shirokovskiy