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

[Neil Horman]

On Tue, Aug 07, 2018 at 06:32:01PM +0200, Jean Delvare wrote:
> 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
> 
Copy that, I'll add it to the next version.

> > 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.
> 
Thanks.  I'll keep an eye out for these systems in the future to test with.

> > 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.
> 
I agree, that was very short sighted.  Seems to be better now though.

> > > 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.
> 
Ah, sorry, missed the diff format.  Ok, that makes more sense.

> > 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.
> 
Ok, I see now.  The RedFish specification only ennumerated the network interface
type, but there are others which I need to ennumerate.  I can fix that.

> > 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.
> 
Yes, you've clarified it quite well, thank you.  I'll fix those items shortly
here.  Just because I'm only working on one system here, could you send me your
dmidecode dump, just so that I have something else to test with?

Thanks!
Neil


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