[v2,13/14] cmd: efishell: export uefi variable helper functions

Message ID 20181105090653.7409-14-takahiro.akashi@linaro.org
State New
Headers show
Series
  • efi: make efi and bootmgr more usable
Related show

Commit Message

AKASHI Takahiro Nov. 5, 2018, 9:06 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/efishell.c    | 4 ++--
 include/command.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Alexander Graf Dec. 2, 2018, 11:54 p.m. | #1
On 05.11.18 10:06, 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/efishell.c    | 4 ++--
>  include/command.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/efishell.c b/cmd/efishell.c
> index bd2b99e74079..8122b842dd76 100644
> --- a/cmd/efishell.c
> +++ b/cmd/efishell.c
> @@ -68,7 +68,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[])
> +int do_efi_dump_var(int argc, char * const argv[])
>  {
>  	char regex[256];
>  	char * const regexlist[] = {regex};
> @@ -228,7 +228,7 @@ out:
>  	return 0;
>  }
>  
> -static int do_efi_set_var(int argc, char * const argv[])
> +int do_efi_set_var(int argc, char * const argv[])
>  {
>  	char *var_name, *value = NULL;
>  	unsigned long size = 0;
> diff --git a/include/command.h b/include/command.h
> index 5bb675122cce..2ce8b53f74c8 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -50,6 +50,8 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>  #endif
>  #if defined(CONFIG_CMD_BOOTEFI)
>  int do_bootefi_bootmgr_exec(int boot_id);
> +int do_efi_dump_var(int argc, char * const argv[]);
> +int do_efi_set_var(int argc, char * const argv[]);

I guess you're seeing a pattern to my comments by now. This needs to go
into efi_loader.h. The respective functions need to go into lib/efi_loader.


Alex
AKASHI Takahiro Dec. 3, 2018, 8:08 a.m. | #2
On Mon, Dec 03, 2018 at 12:54:44AM +0100, Alexander Graf wrote:
> 
> 
> On 05.11.18 10:06, 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/efishell.c    | 4 ++--
> >  include/command.h | 2 ++
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cmd/efishell.c b/cmd/efishell.c
> > index bd2b99e74079..8122b842dd76 100644
> > --- a/cmd/efishell.c
> > +++ b/cmd/efishell.c
> > @@ -68,7 +68,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[])
> > +int do_efi_dump_var(int argc, char * const argv[])
> >  {
> >  	char regex[256];
> >  	char * const regexlist[] = {regex};
> > @@ -228,7 +228,7 @@ out:
> >  	return 0;
> >  }
> >  
> > -static int do_efi_set_var(int argc, char * const argv[])
> > +int do_efi_set_var(int argc, char * const argv[])
> >  {
> >  	char *var_name, *value = NULL;
> >  	unsigned long size = 0;
> > diff --git a/include/command.h b/include/command.h
> > index 5bb675122cce..2ce8b53f74c8 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -50,6 +50,8 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >  #endif
> >  #if defined(CONFIG_CMD_BOOTEFI)
> >  int do_bootefi_bootmgr_exec(int boot_id);
> > +int do_efi_dump_var(int argc, char * const argv[]);
> > +int do_efi_set_var(int argc, char * const argv[]);
> 
> I guess you're seeing a pattern to my comments by now.

Definitely, but

> This needs to go
> into efi_loader.h. The respective functions need to go into lib/efi_loader.

Those two functions are dedicated for command interfaces, and in my opinion,
it is hard for them to be seen as part of efi_loader functionality.

Thanks,
-Takahiro Akashi

> 
> 
> Alex
Alexander Graf Dec. 23, 2018, 3:13 a.m. | #3
On 03.12.18 09:08, AKASHI Takahiro wrote:
> On Mon, Dec 03, 2018 at 12:54:44AM +0100, Alexander Graf wrote:
>>
>>
>> On 05.11.18 10:06, 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/efishell.c    | 4 ++--
>>>  include/command.h | 2 ++
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/cmd/efishell.c b/cmd/efishell.c
>>> index bd2b99e74079..8122b842dd76 100644
>>> --- a/cmd/efishell.c
>>> +++ b/cmd/efishell.c
>>> @@ -68,7 +68,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[])
>>> +int do_efi_dump_var(int argc, char * const argv[])
>>>  {
>>>  	char regex[256];
>>>  	char * const regexlist[] = {regex};
>>> @@ -228,7 +228,7 @@ out:
>>>  	return 0;
>>>  }
>>>  
>>> -static int do_efi_set_var(int argc, char * const argv[])
>>> +int do_efi_set_var(int argc, char * const argv[])
>>>  {
>>>  	char *var_name, *value = NULL;
>>>  	unsigned long size = 0;
>>> diff --git a/include/command.h b/include/command.h
>>> index 5bb675122cce..2ce8b53f74c8 100644
>>> --- a/include/command.h
>>> +++ b/include/command.h
>>> @@ -50,6 +50,8 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>>>  #endif
>>>  #if defined(CONFIG_CMD_BOOTEFI)
>>>  int do_bootefi_bootmgr_exec(int boot_id);
>>> +int do_efi_dump_var(int argc, char * const argv[]);
>>> +int do_efi_set_var(int argc, char * const argv[]);
>>
>> I guess you're seeing a pattern to my comments by now.
> 
> Definitely, but
> 
>> This needs to go
>> into efi_loader.h. The respective functions need to go into lib/efi_loader.
> 
> Those two functions are dedicated for command interfaces, and in my opinion,
> it is hard for them to be seen as part of efi_loader functionality.

Same comment here then :). Feel free to introduce a new header file for
bootefi bits, but I'm not a big fan of a giant command.h that just
exposes all possible command handlers in the world.


Alex
AKASHI Takahiro Dec. 25, 2018, 12:14 p.m. | #4
On Sun, Dec 23, 2018 at 04:13:57AM +0100, Alexander Graf wrote:
> 
> 
> On 03.12.18 09:08, AKASHI Takahiro wrote:
> > On Mon, Dec 03, 2018 at 12:54:44AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 05.11.18 10:06, 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/efishell.c    | 4 ++--
> >>>  include/command.h | 2 ++
> >>>  2 files changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/cmd/efishell.c b/cmd/efishell.c
> >>> index bd2b99e74079..8122b842dd76 100644
> >>> --- a/cmd/efishell.c
> >>> +++ b/cmd/efishell.c
> >>> @@ -68,7 +68,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[])
> >>> +int do_efi_dump_var(int argc, char * const argv[])
> >>>  {
> >>>  	char regex[256];
> >>>  	char * const regexlist[] = {regex};
> >>> @@ -228,7 +228,7 @@ out:
> >>>  	return 0;
> >>>  }
> >>>  
> >>> -static int do_efi_set_var(int argc, char * const argv[])
> >>> +int do_efi_set_var(int argc, char * const argv[])
> >>>  {
> >>>  	char *var_name, *value = NULL;
> >>>  	unsigned long size = 0;
> >>> diff --git a/include/command.h b/include/command.h
> >>> index 5bb675122cce..2ce8b53f74c8 100644
> >>> --- a/include/command.h
> >>> +++ b/include/command.h
> >>> @@ -50,6 +50,8 @@ extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>>  #endif
> >>>  #if defined(CONFIG_CMD_BOOTEFI)
> >>>  int do_bootefi_bootmgr_exec(int boot_id);
> >>> +int do_efi_dump_var(int argc, char * const argv[]);
> >>> +int do_efi_set_var(int argc, char * const argv[]);
> >>
> >> I guess you're seeing a pattern to my comments by now.
> > 
> > Definitely, but
> > 
> >> This needs to go
> >> into efi_loader.h. The respective functions need to go into lib/efi_loader.
> > 
> > Those two functions are dedicated for command interfaces, and in my opinion,
> > it is hard for them to be seen as part of efi_loader functionality.
> 
> Same comment here then :). Feel free to introduce a new header file for
> bootefi bits, but I'm not a big fan of a giant command.h that just
> exposes all possible command handlers in the world.

I've assumed that all the do_xxx() stuff should go into command.h.
I will leave them in command.h for now.

-Takahiro Akashi

> 
> Alex

Patch

diff --git a/cmd/efishell.c b/cmd/efishell.c
index bd2b99e74079..8122b842dd76 100644
--- a/cmd/efishell.c
+++ b/cmd/efishell.c
@@ -68,7 +68,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[])
+int do_efi_dump_var(int argc, char * const argv[])
 {
 	char regex[256];
 	char * const regexlist[] = {regex};
@@ -228,7 +228,7 @@  out:
 	return 0;
 }
 
-static int do_efi_set_var(int argc, char * const argv[])
+int do_efi_set_var(int argc, char * const argv[])
 {
 	char *var_name, *value = NULL;
 	unsigned long size = 0;
diff --git a/include/command.h b/include/command.h
index 5bb675122cce..2ce8b53f74c8 100644
--- a/include/command.h
+++ b/include/command.h
@@ -50,6 +50,8 @@  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #endif
 #if defined(CONFIG_CMD_BOOTEFI)
 int do_bootefi_bootmgr_exec(int boot_id);
+int do_efi_dump_var(int argc, char * const argv[]);
+int do_efi_set_var(int argc, char * const argv[]);
 #endif
 
 /* common/command.c */