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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
