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

[Andrei Borzenkov <arvidjaar@gmail.com>]

В Sun, 22 Mar 2015 22:01:49 -0400
David Michael <fedora.dm0@gmail.com> пишет:
> +struct __attribute__ ((packed)) grub_smbios_eps
> +  {
> +    grub_uint8_t  anchor[4]; /* "_SM_" */

any plans to implement SMBIOS 3.0 (64 bit address) support?

> +    grub_uint8_t  checksum;
> +    grub_uint8_t  length;
> +    grub_uint8_t  version_major;
> +    grub_uint8_t  version_minor;
> +    grub_uint16_t maximum_structure_size;
> +    grub_uint8_t  revision;
> +    grub_uint8_t  formatted[5];
> +    struct grub_smbios_ieps intermediate;
> +  };
> +
> +#define eps_table_begin(eps) ((grub_addr_t)((eps)->intermediate.table_address))
> +#define eps_table_end(eps) \
> +  ((grub_addr_t)((eps)->intermediate.table_address + \
> +                 (eps)->intermediate.table_length))
> +

To make adding 64 bit SMBIOS easier, may be extract entry point and
size (and other relevant fields) instead of referring to them directly.
Then we'd just need to add additional search for 3.0 entry point and
all other code won't need to be changed - tables themselves remain the
same as far as I can tell.
...

> +
> +static grub_err_t
> +grub_cmd_smbios (grub_extcmd_context_t ctxt,
> +                 int argc __attribute__ ((unused)),
> +                 char **argv __attribute__ ((unused)))
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +
> +  grub_int16_t type = -1;
> +  grub_int32_t handle = -1;
> +  grub_uint16_t match = 0;
> +  grub_uint8_t offset = 0;
> +
> +  grub_int32_t option;
> +  const grub_uint8_t *structure;
> +  grub_uint8_t accessors;
> +  grub_uint8_t i;
> +  char buffer[24]; /* 64-bit number -> maximum 20 decimal digits */
> +  const char *value = buffer;
> +

Could we avoid this aliasing? It is extremely confusing to see buffer
used everywhere and then suddenly value in the last line. What is the
reason?
> +
> +  /* Store or print the requested value. */
> +  if (state[8].set)
> +    {
> +      grub_env_set (state[8].arg, value);
> +      grub_env_export (state[8].arg);

Why export variable here? It is up to user what to do with it later.

> +static const struct grub_arg_option options[] =
> +  {
> +    {"type",       't', 0, N_("Match entries with the given type."),
> +                           N_("type"), ARG_TYPE_INT},
> +    {"handle",     'h', 0, N_("Match entries with the given handle."),
> +                           N_("handle"), ARG_TYPE_INT},
> +    {"match",      'm', 0, N_("Select a structure when several match."),
> +                           N_("match"), ARG_TYPE_INT},
> +    {"get-byte",   'b', 0, N_("Get the byte's value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-word",   'w', 0, N_("Get two bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-dword",  'd', 0, N_("Get four bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-qword",  'q', 0, N_("Get eight bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-string", 's', 0, N_("Get the string specified at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"set",       '\0', 0, N_("Store the value in the given variable name."),
> +                           N_("variable"), ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };

One non-trivial structure field that is rather awkward to get
otherwise is UUID.