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

[Jean Delvare]

On Tue, 7 Aug 2018 07:33:54 -0400, Neil Horman wrote:
> On Tue, Aug 07, 2018 at 09:29:44AM +0200, Jean Delvare wrote:
> > That's tempting, however it does not seem like these macros are dealing
> > with unaligned access. If we are running on a little-endian machine
> > which does not support unaligned memory access (hello IA-64!), we end
> > up with the following definitions:
> > 
> > #  define le16toh(x) (x)
> > #  define le32toh(x) (x)
> > #  define le64toh(x) (x)
> > 
> > which will surely crash.  
> I don't think so, at least not on linux, where the kernel installs a handler 
> to
> fix up unaligned accesses.

Is it a "recent" addition? I am certain that it used to crash, back in
2003 - I did not implement ALIGNMENT_WORKAROUND just for fun. The git
log also has evidence that gcc used to complain about unaligned
structure member copies. If you can point me to the kernel piece of
code which deals with that, I'm interested. That being said, I am
afraid that it must be slower to do things wrong and let the kernel
cope with the mess, than do the right thing directly?

I guess I could just connect to an IA-64 system at work and try, but if
it doesn't work and I crash the machine, my colleagues might
complain ;-)

> Either way, it should be pretty easily fixable I
> think by defining the macros as:
> 
> #define WORD(x) le16toh(get_unaligned(x))

get_unaligned() doesn't seem to be a standard user-space function.

> ...
> That may not be exactly right (might need to serialize the macro use), but its
> close.

Doesn't sound that much more simple than what I did ;-)

> Either way, I guess it doesn't really matter, this was more about not
> maintaining duplicate code for endian macros, but since its already written 
> and
> likely won't change again for another decade, theres less value in this.

Well I'm always interested in ways to clean up my code, so it remains
an interesting discussion.

> (...)
> Sure, as soon as I get it back up and running.  Also note, I'm starting to 
> think
> it might have a problem in the offset of its protocol record area (I'll need

Oh you think? ;-D

> another system to compare it too, which I'll have later this week I think).
> Either way, it might be useful to have an erroneous dump for testing purposes

Having several samples to test a new features is always good.

And yes, I have a number of known bad DMI tables in my test suite, to
exercise the error paths. But I'm not sure that's what you have... See
my review of your patch.

-- 
Jean Delvare
SUSE L3 Support