[libvirt] [PATCH v2] virSysinfo: Introduce SMBIOS type 2 support
Michal Privoznik
mprivozn at redhat.com
Thu Jun 18 08:19:52 UTC 2015
On 17.06.2015 15:44, John Ferlan wrote:
>
>
> 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 at redhat.com>
>> ---
>>
>> 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 @@
>> <entry name='product'>Virt-Manager</entry>
>> <entry name='version'>0.9.4</entry>
>> </system>
>> + <baseBoard>
>> + <entry name='manufacturer'>LENOVO</entry>
>> + <entry name='product'>20BE0061MC</entry>
>> + <entry name='version'>0B98401 Pro</entry>
>> + <entry name='serial'>W1KS427111E</entry>
>> + </baseBoard>
>> </sysinfo>
>> ...</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
Good catch! I've split v1 into two patches locally. I've extracted the
board type into a separate patch but this has somehow slipped through.
>
>> + 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...
Yeah, now that we are not trying to fit a string into an enum (= parse
board type), these two lines do not make much sense. I'll move them to
the other patch.
>
>> + 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...
Well, strictly speaking it's not needed to be exported right now,
correct. However, I think it's more future-proof if we export it. I
mean, in my scenario, if this was not exported, I'd need yet another
patch that will just export it.
Thanks for the review!
Michal
More information about the libvir-list
mailing list