[v4,8/9] cmd: efitool: export uefi variable helper functions

Message ID 20190115025522.12060-9-takahiro.akashi@linaro.org
State New
Headers show
Series
  • cmd: add efitool for efi environment
Related show

Commit Message

AKASHI Takahiro Jan. 15, 2019, 2:55 a.m.
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(-)

Comments

Heinrich Schuchardt Jan. 15, 2019, 5:28 a.m. | #1
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
>
AKASHI Takahiro Jan. 17, 2019, 7:30 a.m. | #2
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
> > 
>

Patch

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