Re: [PATCH] Add fwconfig command

[Daniel Kiper]

On Mon, Jan 23, 2017 at 04:32:26PM -0800, Matthew Garrett wrote:
> Add a command to read values from the qemu fwcfg store. This allows data
> to be passed from the qemu command line to grub.
>
> Example use:
>
> echo '(hd0,1)' >rootdev
> qemu -fw_cfg opt/rootdev,file=rootdev
>
> fwconfig opt/rootdev root

I think that this should be made clear that fwconfig command should be
executed in GRUB not in shell. At least I understand it in that way.

[...]

> +static grub_err_t
> +grub_cmd_fwconfig (grub_extcmd_context_t ctxt, int argc, char **argv)
> +{
> +  grub_uint32_t i, j, value = 0;
> +  struct grub_qemu_fwcfgfile file;
> +  char fwsig[4], signature[4] = { 'Q', 'E', 'M', 'U' };
> +
> +  if (argc != 2)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
> +
> +  /* Verify that we have meaningful hardware here */
> +  grub_outw(SIGNATURE_INDEX, SELECTOR);
> +  for (i=0; i<sizeof(fwsig); i++)
> +      fwsig[i] = grub_inb(DATA);
> +
> +  if (grub_memcmp(fwsig, signature, sizeof(signature)) != 0)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, N_("invalid fwconfig hardware 
> signature: got 0x%x%x%x%x"), fwsig[0], fwsig[1], fwsig[2], fwsig[3]);

Too long line. Could you wrap it?

> +
> +  /* Find out how many file entries we have */
> +  grub_outw(DIRECTORY_INDEX, SELECTOR);
> +  value = grub_inb(DATA) | grub_inb(DATA) << 8 | grub_inb(DATA) << 16 | 
> grub_inb(DATA) << 24;

Ditto.

> +  value = grub_be_to_cpu32(value);
> +  /* Read the file description for each file */
> +  for (i=0; i<value; i++)
> +    {
> +      for (j=0; j<sizeof(file); j++)
> +     {
> +       ((char *)&file)[j] = grub_inb(DATA);
> +     }
> +      /* Check whether it matches what we're looking for, and if so read the 
> file */

Ditto.

> +      if (grub_strncmp(file.name, argv[0], sizeof(file.name)) == 0)
> +     {
> +       grub_uint32_t filesize = grub_be_to_cpu32(file.size);
> +       grub_uint16_t location = grub_be_to_cpu16(file.select);
> +       char *data = grub_malloc(filesize+1);
> +
> +       if (!data)
> +           return grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("can't allocate 
> buffer for data"));

Ditto.

> +
> +       grub_outw(location, SELECTOR);
> +       for (j=0; j<filesize; j++)
> +         {
> +           data[j] = grub_inb(DATA);
> +         }
> +
> +       data[filesize] = '\0';
> +
> +       grub_env_set (argv[1], data);
> +
> +       grub_free(data);
> +       return 0;
> +     }
> +    }
> +
> +  return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("couldn't find entry %s"), 
> argv[0]);

Ditto.

Additionally, could you be more in line with GRUB2 coding style?
I know that it is not strictly defined but if you look at a few
files you will catch what I mean.

Though in general LGTM but this is not 2.02 material.
I am adding this to my radar for next release.

Daniel