Re: [dmidecode] [PATCH v2] update dmidecode to parse Modern Management Controller blocks

[Jean Delvare]

Hi Neil,

On Tue, 7 Aug 2018 11:51:32 -0400, Neil Horman wrote:
> On Tue, Aug 07, 2018 at 02:51:51PM +0200, Jean Delvare wrote:
> > On Thu,  2 Aug 2018 10:55:15 -0400, Neil Horman wrote:
> > Please provide links to the documents you used to write this patch. I
> > don't see anything relevant in the SMBIOS specification itself. Every
> > document needed to understand the code should be mentioned in the
> > header comment (search for "Additional references:"). Without that
> > information, I can only review the code from a technical perspective,
> > but I can't say anything about its functional correctness.
>
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0270_1.0.0.pdf
> and
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.2.0.pdf
> 
> Its the SMBIOS reference specification.  The above is the latest version, but
> the structure layout was added in version 3.0.0 (hence the version check that 
> I
> made in the origional case statement).  Prior to that this structure is
> undefined.

I'm obviously familiar with the latter ;-) it's the former which I was
missing. Well now I see that it is referenced from the SMBIOS
specification itself, but I still believe it is better to add an explicit
reference at the top of dmidecode.c.

FWIW there is a new version of the Redfish specification:
https://www.dmtf.org/sites/default/files/DSP0270_1.0.1.pdf

> Note that with version 3.2.0 there are other management inteface
> types besides redfish.  However, I'm currently working on a redfish project, 
> and
> do not have access to any other interface types, and so have left those 
> uncoded.

That's perfectly fine, support for the rest can be added later
incrementally.

> I do note, looking at them a bit closer, a few discrepancies.  The former 
> lists
> the minimal type 42 strucute as being 0x9 bytes, while the smbios spec lists 
> it
> as 0xb bytes.  That should probably be fixed up.

0x9 bytes was the minimum length of the original definition of type 42,
in SMBIOS specification 2.7.0 (until 3.1.1). But this original
definition was hardly usable (variable length, type-specific part in
the middle of the structure, with no length field to know where it
ends, hello?), which explains why a new definition was adopted in
SMBIOS version 3.2.0. That new definition has a minimum length of 0xB
bytes.

> > Note: for the time being, the only machine I have access to which
> > implements type 42 had the type set to 0xF0 (OEM) and an apparently
> > invalid vendor ID (0xFF0102FF). Your patch changes the output of
> > dmidecode from useless information to different but equally useless
> > (and most certainly wrong) information:
>
> How on earth did that all get printed?  That looks like the code managed to 
> take
> both branches of the version check clause in the type 42 case. The Interface
> Type and Vendor ID fields are the legacy printouts from prior to my patch, 
> while
> the remainder is the output from decode_management_controller_structure().

Nah. In case it wasn't obvious, I included a diff, not a mere output,
thus the leading -/+ on each line. You first see the output before your
patch and then after your patch.

> I
> would expect the former, as I imagine your system has an SMBIOS that conforms 
> to
> a version of the spec prior to 3.0.0, but the later should not be there, and 
> the

No, my test system claims to implement SMBIOS 3.1.1. That's why it goes
through your new code (because you test for >= 3.0). My type 42
structure clearly does not comply with the new layout in 3.2.0, thus
the crappy output if it is processed by your new code. It doesn't
really seem to comply with the original definition either, but at least
it does not explode.

> if/else clause seems pretty clear to me.  Can you send me a dump of your 
> smbios
> so that I can compare?

Hopefully my explanation above clarified the situation, but I can still
send the dump to you. Will do in private.

> also, I'm attaching a copy of my smbios dump for your reference.

Thank you, I'm adding it to my collection.

-- 
Jean Delvare
SUSE L3 Support