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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]