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);
return do_efi_set_var(argc, argv);
return _do_efi_set_var(argc, argv);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