Re: [dmidecode] [PATCH] dmidecode: Extensions to Memory Device (Type 17)

[Jerry Hoemann]

On Sat, Jun 16, 2018 at 07:07:57PM +0200, Jean Delvare wrote:
> On Fri, 15 Jun 2018 15:08:39 -0600, Jerry Hoemann wrote:
> > On Fri, Jun 15, 2018 at 10:01:02AM +0200, Jean Delvare wrote:
> > > On Mon, 11 Jun 2018 11:40:07 -0600, Jerry Hoemann wrote:  
> > > > +static void dmi_memory_size(u64 code)
> > > > +{
> > > > +       if (code.h == 0xFFFFFFFF && code.l == 0xFFFFFFFF)
> > > > +               printf(" Unknown");
> > > > +       else
> > > > +               printf(" 0x%08X%08X", code.h, code.l);
> > > > +}  
> > > 
> > > Please re-use dmi_print_memory_size() instead.  
> > 
> > Done.
> > 
> > Looking at spec again, fields 34h, 3Ch, and 44h, say if the value is 0,
> > there is no XXXX portion.  This verbiage isn't present for field 4Ch.
> 
> True, but later in the specification, is mentioned: "When Memory Type
> is not Logical, Logical Size shall be 0." So that field can be 0 as well.
> 
> > I'm thinking that for 34h, 3Ch, 44h, we also special case when field is "0."
> > This would have then two functions,  dmi_memory_size() for these three 
> > fields
> > and dmi_memory_logical_size() for 4Ch.
> 
> Special case how? Printing (for example):
> 
>       Volatile Size: None
> 
> or not printing that line at all? The latter has my preference.


I'm printing "None" but leaving it in if the field is specified by FW.
If FW doesn't specify the field, it won't appear.


> 
> As a side note, I really don't like this Logical Memory thing. The
> specification fails to explain the purpose of that abstraction layer,
> which then makes it hard to grasp the actual meaning of the related
> bits and fields :-(
> 
> > > (...)
> > > Also, the specification says that length must be at least 34h for
> > > SMBIOS version 3.2.0 and later. This suggests that the fields after
> > > that are optional, and may not be present in all implementations. So it
> > > would be safer to check for h->length < 0x34 first, and add another
> > > length check later in the code flow for the remaining fields.  
> > 
> > I missed this.  I was thinking it was all or nothing on the new fields.
> 
> That's almost always how things are implemented in practice, and also
> in most cases the specification explicitly requests that all new fields
> are present. But the fact that the minimum length mentioned in the
> description of the Length field is less than the new fields is an
> indication that it may not be the case this time. So I would check both.
> 
> > > I think your length check is wrong too... Last field *starts* at 0x4C
> > > and is 8 byte long, so the length check for a full record should
> > > be < 0x54.  
> > 
> > You're correct.  Fixed.
> > 
> > Next version will have appropriate length checks before printing
> > *each* of the new fields.
> 
> Not fundamentally incorrect but overkill in my humble opinion. The
> current code has boundary checks which essentially match the fields
> grouped by the specification version in which they were added. I have a
> script crawling my collection of DMI table dumps and verifying that
> none of them has a record length that the code does not expect, so I
> believe we are on the safe side. I think it caught only 4 unexpected
> record lengths in 15 years.
> 

I'm checking each field once we're *beyond* the check for the group upto 34h.
Like you said, the spec would allow for FW to not have all the fields
beyond 34h.  Better safe than sorry.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------