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

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&gt; >>> --- >>> 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.