Re: [libvirt] [PATCH v2] virSysinfo: Introduce SMBIOS type 2 support

On 06/12/2015 06:02 AM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1220527 > > This type of information defines attributes of a system > baseboard. With one caveat: in qemu they call it family, while > in the specification it's referred to as type. I'm sticking with > the latter. Perhaps should update this since 'family' and 'type' aren't processed by libvirt. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > > diff to v1: > -I've dropped the 'family' attribute. It's not implemented in qemu yet. We can > add it later. > > docs/formatdomain.html.in | 37 ++++- > docs/schemas/domaincommon.rng | 23 +++ > src/conf/domain_conf.c | 63 ++++++++ > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 54 +++++++ > src/util/virsysinfo.c | 170 ++++++++++++++++++++- > src/util/virsysinfo.h | 16 ++ > .../qemuxml2argv-smbios-multiple-type2.xml | 58 +++++++ > tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 + > tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 8 + > tests/qemuxml2xmltest.c | 1 + > 11 files changed, 427 insertions(+), 6 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-multiple-type2.xml > There's a couple issues pointed out below, fixable without need for a v3 I believe... ACK with the adjustments John > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 0478cb2..977660e 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -375,6 +375,12 @@ > &lt;entry name='product'&gt;Virt-Manager&lt;/entry&gt; > &lt;entry name='version'&gt;0.9.4&lt;/entry&gt; > &lt;/system&gt; > + &lt;baseBoard&gt; > + &lt;entry name='manufacturer'&gt;LENOVO&lt;/entry&gt; > + &lt;entry name='product'&gt;20BE0061MC&lt;/entry&gt; > + &lt;entry name='version'&gt;0B98401 Pro&lt;/entry&gt; > + &lt;entry name='serial'&gt;W1KS427111E&lt;/entry&gt; > + &lt;/baseBoard&gt; > &lt;/sysinfo&gt; > ...</pre> > > @@ -435,11 +441,32 @@ > <dt><code>family</code></dt> > <dd>Identify the family a particular computer belongs to.</dd> > </dl> > - NB: Incorrectly supplied entries in either the <code>bios</code> > - or <code>system</code> blocks will be ignored without error. > - Other than <code>uuid</code> validation and <code>date</code> > - format checking, all values are passed as strings to the > - hypervisor driver. > + </dd> > + <dt><code>baseBoard</code></dt> > + <dd> > + This is block 2 of SMBIOS. This element can be repeated multiple > + times, to describe all the base boards. However, not all s/times,/times s/boards. However,/boards; however, > + hypervisors support the repetition necessarily. The element can s/support the repetition necessarily/necessarily support the repitition or s/support the repetition necessarily./support multiple base boards./ > + have the following children: > + <dl> > + <dt><code>manufacturer</code></dt> > + <dd>Manufacturer of BIOS</dd> > + <dt><code>product</code></dt> > + <dd>Product Name</dd> > + <dt><code>version</code></dt> > + <dd>Version of the product</dd> > + <dt><code>serial</code></dt> > + <dd>Serial number</dd> > + <dt><code>asset</code></dt> > + <dd>Asset tag</dd> > + <dt><code>location</code></dt> > + <dd>Location in chassis</dd> > + </dl> > + NB: Incorrectly supplied entries for the > + <code>bios</code>, <code>system</code> or <code>baseBoard</code> > + blocks will be ignored without error. Other than <code>uuid</code> > + validation and <code>date</code> format checking, all values are > + passed as strings to the hypervisor driver. > </dd> > </dl> > </dd> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index e38b927..32d28cd 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -4248,6 +4248,18 @@ > </oneOrMore> > </element> > </optional> > + <zeroOrMore> > + <element name="baseBoard"> > + <oneOrMore> > + <element name="entry"> > + <attribute name="name"> > + <ref name="sysinfo-baseBoard-name"/> > + </attribute> > + <ref name="sysinfo-value"/> > + </element> > + </oneOrMore> > + </element> > + </zeroOrMore> > </interleave> > </element> > </define> > @@ -4273,6 +4285,17 @@ > </choice> > </define> > > + <define name="sysinfo-baseBoard-name"> > + <choice> > + <value>manufacturer</value> > + <value>product</value> > + <value>version</value> > + <value>serial</value> > + <value>asset</value> > + <value>location</value> > + </choice> > + </define> > + > <define name="sysinfo-value"> > <data type="string"> > <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index bf7eeb2..c2174d9 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -11227,6 +11227,65 @@ virSysinfoSystemParseXML(xmlNodePtr node, > return ret; > } > > +static int > +virSysinfoBaseBoardParseXML(xmlXPathContextPtr ctxt, > + virSysinfoBaseBoardDefPtr *baseBoard, > + size_t *nbaseBoard) > +{ > + int ret = -1; > + virSysinfoBaseBoardDefPtr boards = NULL; > + size_t i, nboards = 0; > + char *board_type = NULL; Unused

> + xmlNodePtr *nodes = NULL, oldnode = ctxt->node; > + int n; > + > + if ((n = virXPathNodeSet("./baseBoard", ctxt, &nodes)) < 0) > + return ret; > + > + if (n && VIR_ALLOC_N(boards, n) < 0) > + goto cleanup; > + > + for (i = 0; i < n; i++) { > + virSysinfoBaseBoardDefPtr def = boards + nboards; > + > + ctxt->node = nodes[i]; > + > + def->manufacturer = > + virXPathString("string(entry[@name='manufacturer'])", ctxt); > + def->product = > + virXPathString("string(entry[@name='product'])", ctxt); > + def->version = > + virXPathString("string(entry[@name='version'])", ctxt); > + def->serial = > + virXPathString("string(entry[@name='serial'])", ctxt); > + def->asset = > + virXPathString("string(entry[@name='asset'])", ctxt); > + def->location = > + virXPathString("string(entry[@name='location'])", ctxt); > + > + if (!def->manufacturer && !def->product && !def->version && > + !def->serial && !def->asset && !def->location) { > + /* nada */ > + } else { > + nboards++; > + } > + } > + > + *baseBoard = boards; > + *nbaseBoard = nboards; > + boards = NULL; > + nboards = 0; > + ret = 0; > + cleanup: > + while (nboards--) > + virSysinfoBaseBoardDefClear(&boards[nboards]); Coverity notes nboards can never be anything other than zero when we get here. IOW we can never jump out of that for loop where nboards gets incremented unlike virSysinfoParseBaseBoard where the VIR_EXPAND_N failure...

> + VIR_FREE(boards); > + VIR_FREE(board_type); Unused > + VIR_FREE(nodes); > + ctxt->node = oldnode; > + return ret; > +} > + > static virSysinfoDefPtr > virSysinfoParseXML(xmlNodePtr node, > xmlXPathContextPtr ctxt, > @@ -11281,6 +11340,10 @@ virSysinfoParseXML(xmlNodePtr node, > ctxt->node = oldnode; > } > > + /* Extract system base board metadata */ > + if (virSysinfoBaseBoardParseXML(ctxt, &def->baseBoard, &def->nbaseBoard) < 0) > + goto error; > + > cleanup: > VIR_FREE(type); > return def; > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index dc8a52d..2348f14 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2182,6 +2182,7 @@ virVasprintfInternal; > > > # util/virsysinfo.h > +virSysinfoBaseBoardDefClear; Probably won't be necessary now that domain_conf.c doesn't need it...