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@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