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

[Neil Horman]

On Tue, Aug 07, 2018 at 02:51:51PM +0200, Jean Delvare wrote:
> Hi Neil,

><snip most comments> 
> 
> Note for next time: please start a new thread for each new iteration of
> a patch. Otherwise the comments about different versions of the patch
> can interleave and this create confusion.
> 
Will do

><snip>
> > +                                 (addrtype == 0x1 ?
> > +                                   inet_ntop(AF_INET, (char 
> > *)&rdata[REC_HOST_ADDR], (char *)buf, 64) :
> > +                                   inet_ntop(AF_INET6, (char 
> > *)&rdata[REC_HOST_ADDR], (char *)buf, 64)));
> > +
> > +                           /* Prints the Host IP Mask */
> > +                           printf("%s\t\t%s Mask: %s\n", prefix,
> > +                                 (addrtype == ADDR_IPV4 ? "Ipv4" : "Ipv6"),
> > +                                 (addrtype == ADDR_IPV4 ?
> > +                                   inet_ntop(AF_INET, (char 
> > *)&rdata[REC_HOST_MASK], (char *)buf, 64) :
> > +                                   inet_ntop(AF_INET6, (char 
> > *)&rdata[REC_HOST_MASK], (char *)buf, 64)));
> > +                   }
> 
> Any idea how portable is inet_ntop()? Note that in theory it can fail,
> but you don't check for that. I admit I can't see how that would happen
> here, but if it did, printf would cause a segfault.
> 
inet_ntop is part of POSIX-2001, and so should be fairly portable and
universally available.

><snip>

> 
> One issue with your patch is that the Interface Type Specific Data part
> of OEM records is no longer decoded for recent SMBIOS implementations.
> This could lead to regressions for some users (although to be honest
> type 42 records are not exactly popular, so that's essentially
> theoretical...)
> 
Redfish is supposedly going to be making type 42 a much more used structure, as
its currently the only way for an OS to discover its local BMC interface.  Thats
why I'm working on it here.  We will have to see if that comes to fruition.

Not sure what you mean by the above statement "One issue with your patch is that
the Interface Type Specific Data part of OEM records is no longer decoded...".
Prior to my patch the interface specific data area was never decoded, at least
no that I can see.  The only change I've made is that for SMBIOS versions less
than 0x0302, it does exactly what it did before, and for more recent versions
(with that length field properly specified in the middle of the structure), it
does the more complete decode.  Is there some other decoding that I'm missing?

> By the way, I am considering the possibility to stop decoding type 42
> records completely for SMBIOS implementations < 3.2.0 because the
> definition was just too crappy. I have yet to see a valid type 42
> record other than yours...
> 
FWIW, I'd be fine with that, as the dump I provided you isn't correct either,
its version field is 0x0301, it specifies the interface length field (I think),
but it adds an extra byte at the end of the length field before providing the
protocol count.  I would agree, you may not want to touch anything prior to
0x0302.  

I've addressed all the comments that you had in this initial note, and am
testing them on a new system now (that is supposedly v3.0.2 compliant.  When I'm
safisfied that it works, I'll post a v4 patch (likely monday)

Neil

> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support
>