Re: [PATCH v2] Add a module for retrieving SMBIOS information

[David Michael]

On Wed, Feb 11, 2015 at 6:34 AM, Andrei Borzenkov <address@hidden> wrote:
> В Sun, 08 Feb 2015 13:54:46 -0500
> David Michael <address@hidden> пишет:
>> address@hidden
>> +Specifying @option{-t} will only print entries with a matching @var{type}.  
>> The
>> +type can be any value from 0 to 255.  Specify a value outside this range to
>> +match entries with any type.  The default is -1.
>
> I think out of range value should result in error.

Okay, but I think the user should still be able to specify a value
that can remove a previously specified type.  Maybe a separate long
option "--no-type" or a magic value like "-t any"?

>> +  /* Write the entry's mandatory four header bytes. */
>> +  length = ptr[1];
>> +  grub_printf ("Entry: Type=0x%02x Length=0x%02x Handle=0x%04x\n",
>> +               ptr[0], length, *(1 + (grub_uint16_t *)ptr));
>
> Could you reverse the order in 1 + (grub_uint16_t *)ptr and in other
> places too?

I'll replace all these pointer casts+dereferences with functions that
avoid alignment issues.

>> +  /* Dump of the formatted area (including the header) in hex. */
>> +  grub_printf (" Hex Dump: ");
>> +  while (length-- > 0)
>> +    grub_printf ("%02x", *ptr++);
>> +  grub_printf ("\n");
>> +
>> +  /* Print each string found in the appended string list. */
>> +  while (ptr[0] != 0 || ptr[1] != 0)
>
> I wonder do we have any ad hoc way to limit it? It looks like Linux
> kernel does more sanity checks and does not blindly trust data.

The entry point structure has a two-byte field for total SMBIOS table
length, so that can put an upper limit on the string sets.

>

Thanks for the detailed review.  I hope to be able to be able to send
a new revision later in the week.

Side note: A new major version of the SMBIOS specification was posted
last week which includes 64-bit addresses and such.  I'm not planning
to try to add this at the moment, unless anyone has a need for it.

Thanks.

David