| Message ID | 20190115025522.12060-9-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cmd: add efitool for efi environment | expand |
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@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. Please, use names that describe what the difference is. > { > 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. 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. > + 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." > + 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. > +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 >
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@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 > > >
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[]) { 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); + 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", + 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) +int do_efi_dump_var(int argc, char * const argv[]); +int do_efi_set_var(int argc, char * const argv[]); +#endif /* common/command.c */ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
Those function will be used for integration with 'env' command so as to handle uefi variables. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/efitool.c | 38 ++++++++++++++++++++++++++++++++++---- include/command.h | 4 ++++ 2 files changed, 38 insertions(+), 4 deletions(-)