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

[Jean Delvare]

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.

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.

-- 
Jean Delvare
SUSE L3 Support