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@xxxxxxxxxx> >> --- >> >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list