From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Thu, 17 Jan 2019 16:30:56 +0900 Subject: [U-Boot] [PATCH v4 8/9] cmd: efitool: export uefi variable helper functions In-Reply-To: References: <20190115025522.12060-1-takahiro.akashi@linaro.org> <20190115025522.12060-9-takahiro.akashi@linaro.org> Message-ID: <20190117073054.GW20286@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > --- > > 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 > > >