Re: [PATCH v5 2/2] Add a module for retrieving SMBIOS information

[Andrei Borzenkov <arvidjaar@gmail.com>]

Two nitpicks otherwise looks good. Thanks!

20.02.2016 22:12, David Michael пишет:
...

> diff --git a/grub-core/commands/smbios.c b/grub-core/commands/smbios.c
> new file mode 100644
> index 0000000..80a01a3
> --- /dev/null
> +++ b/grub-core/commands/smbios.c
> @@ -0,0 +1,364 @@
> +/* smbios.c - retrieve smbios information. */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2016  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/dl.h>
> +#include <grub/env.h>
> +#include <grub/extcmd.h>
> +#include <grub/i18n.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/smbios.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +
> +/* Locate the SMBIOS entry point structure depending on the hardware. */
> +struct grub_smbios_eps *
> +grub_smbios_get_eps (void)
> +{
> +  static struct grub_smbios_eps *eps = NULL;
> +  if (eps != NULL)
> +    return eps;
> +  eps = grub_machine_smbios_get_eps ();
> +  return eps;
> +}
> +
> +/* Locate the SMBIOS3 entry point structure depending on the hardware. */
> +struct grub_smbios_eps3 *
> +grub_smbios_get_eps3 (void)
> +{
> +  static struct grub_smbios_eps3 *eps = NULL;
> +  if (eps != NULL)
> +    return eps;
> +  eps = grub_machine_smbios_get_eps3 ();
> +  return eps;
> +}
> +
> +/* Abstract useful values found in either the SMBIOS3 or SMBIOS EPS. */
> +static struct {
> +  grub_addr_t start;
> +  grub_addr_t end;
> +  grub_uint16_t structures;
> +} table_desc = {0, 0, 0};

Static is supposed to be initialized to zero by default.

...
> +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;
> +
> +  const grub_uint8_t *structure;
> +  const char *value;
> +  grub_int32_t option;
> +  grub_int8_t field_type = -1;
> +  grub_uint8_t i;
> +
> +  if (table_desc.start == 0)
> +    return grub_error (GRUB_ERR_IO,
> +                       N_("the SMBIOS entry point structure was not found"));
> +
> +  /* Read the given filtering options. */
> +  if (state[0].set)
> +    {
> +      option = grub_strtol (state[0].arg, NULL, 0);
> +      if (option < 0 || option > 255)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                           N_("the type must be between 0 and 255"));
> +      type = (grub_int16_t)option;
> +    }
> +  if (state[1].set)
> +    {
> +      option = grub_strtol (state[1].arg, NULL, 0);
> +      if (option < 0 || option > 65535)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                           N_("the handle must be between 0 and 65535"));
> +      handle = (grub_int32_t)option;
> +    }
> +  if (state[2].set)
> +    {
> +      option = grub_strtol (state[2].arg, NULL, 0);
> +      if (option <= 0)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                           N_("the match must be a positive integer"));
> +      match = (grub_uint16_t)option;

Should we check that option <= 65535?