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

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.

The change makes sense regardless, so: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; ...but I'm still curious Erik