[U-Boot] [PATCH v4 8/9] cmd: efitool: export uefi variable helper functions
AKASHI Takahiro
takahiro.akashi at linaro.org
Thu Jan 17 07:30:56 UTC 2019
On Tue, Jan 15, 2019 at 06:28:38AM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:55 AM, AKASHI Takahiro wrote:
> > Those function will be used for integration with 'env' command
> > so as to handle uefi variables.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> > cmd/efitool.c | 38 ++++++++++++++++++++++++++++++++++----
> > include/command.h | 4 ++++
> > 2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/cmd/efitool.c b/cmd/efitool.c
> > index f06718ea580d..b8fe28c53aaf 100644
> > --- a/cmd/efitool.c
> > +++ b/cmd/efitool.c
> > @@ -67,7 +67,7 @@ static void dump_var_data(char *data, unsigned long len)
> > *
> > * efi_$guid_$varname = {attributes}(type)value
> > */
> > -static int do_efi_dump_var(int argc, char * const argv[])
> > +static int _do_efi_dump_var(int argc, char * const argv[])
>
> Please, do not use two function names only distinguished by and
> underscore. A a reader I will always wonder which does what.
While I believe that "_" and "__" are a very common practice to
show an internal version of function,
> Please, use names that describe what the difference is.
I will revert _do_efi_dump_var() to do_efi_dump_var() and
change do_efi_dump_var() to do_efi_dump_var_ext().
> > {
> > char regex[256];
> > char * const regexlist[] = {regex};
> > @@ -130,6 +130,21 @@ static int do_efi_dump_var(int argc, char * const argv[])
> > return CMD_RET_SUCCESS;
> > }
> >
> > +int do_efi_dump_var(int argc, char * const argv[])
> > +{
> > + efi_status_t r;
> > +
> > + /* Initialize EFI drivers */
> > + r = efi_init_obj_list();
> > + if (r != EFI_SUCCESS) {
> > + printf("Error: Cannot set up EFI drivers, r = %lu\n",
> > + r & ~EFI_ERROR_MASK);
>
> You duplicate this code below.
Do you want another function for just calling one function?
I don't think it's worth doing so.
> So, please, put this into a separate function.
>
> That could also help to avoid having separate do_efi_set_var and
> _do_efi_set_var.
I don't get your point.
Do you want to call efi_init_obj_list() at every do_efi_xxx()?
It just doesn't make sense.
> > + return CMD_RET_FAILURE;
> > + }
> > +
> > + return _do_efi_dump_var(argc, argv);
> > +}
> > +
> > static int append_value(char **bufp, unsigned long *sizep, char *data)
> > {
> > char *tmp_buf = NULL, *new_buf = NULL, *value;
> > @@ -227,7 +242,7 @@ out:
> > return 0;
> > }
> >
> > -static int do_efi_set_var(int argc, char * const argv[])
> > +static int _do_efi_set_var(int argc, char * const argv[])
> > {
> > char *var_name, *value = NULL;
> > efi_uintn_t size = 0;
> > @@ -270,6 +285,21 @@ out:
> > return ret;
> > }
> >
> > +int do_efi_set_var(int argc, char * const argv[])
> > +{
> > + efi_status_t r;
> > +
> > + /* Initialize EFI drivers */
> > + r = efi_init_obj_list();
> > + if (r != EFI_SUCCESS) {
> > + printf("Error: Cannot set up EFI drivers, r = %lu\n",
>
> It is more then drivers that we setup. So perhaps
>
> "Cannot initialize UEFI sub-system."
I don't mind, but that string directly comes from the original code
in bootefi.c.
> > + r & ~EFI_ERROR_MASK);
> > + return CMD_RET_FAILURE;
> > + }
> > +
> > + return _do_efi_set_var(argc, argv);
> > +}
> > +
> > static int efi_get_handles_by_proto(efi_guid_t *guid, efi_handle_t **handlesp,
> > int *num)
> > {
> > @@ -1016,9 +1046,9 @@ static int do_efitool(cmd_tbl_t *cmdtp, int flag,
> > if (!strcmp(command, "boot"))
> > return do_efi_boot_opt(argc, argv);
> > else if (!strcmp(command, "dumpvar") || !strcmp(command, "dmpstore"))
> > - return do_efi_dump_var(argc, argv);
> > + return _do_efi_dump_var(argc, argv);
> > else if (!strcmp(command, "setvar"))
> > - return do_efi_set_var(argc, argv);
> > + return _do_efi_set_var(argc, argv);
> > else if (!strcmp(command, "devices"))
> > return do_efi_show_devices(argc, argv);
> > else if (!strcmp(command, "drivers"))
> > diff --git a/include/command.h b/include/command.h
> > index feddef300ccc..315e4b9aabfb 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -51,6 +51,10 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > #if defined(CONFIG_CMD_BOOTEFI)
> > extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > #endif
> > +#if defined(CONFIG_CMD_EFITOOL)
>
> The definition has not dependencies. No need for an #if here.
Currently has, but as you and Alex suggested in a separate thread,
we may want to enable it (and do_efi_dump/set_var() as well)
whether or not CMD_BOOTEFI and CMD_EFIDEBUG, respectively, are defined.
Thanks,
-Takahiro Akashi
> > +int do_efi_dump_var(int argc, char * const argv[]);
> > +int do_efi_set_var(int argc, char * const argv[]);
> > +#endi
>
> Shouldn't we put this into some include/efi*?
>
> Best regards
>
> Heinrich
>
> >
> > /* common/command.c */
> > int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >
>
More information about the U-Boot
mailing list