diff mbox series

[v6,1/5] eficonfig: refactor eficonfig_select_file_handler()

Message ID 20221026104345.28714-2-masahisa.kojima@linaro.org
State Superseded
Headers show
Series eficonfig: add UEFI Secure Boot key maintenance interface | expand

Commit Message

Masahisa Kojima Oct. 26, 2022, 10:43 a.m. UTC
eficonfig_select_file_handler() is commonly used to select the
file. eficonfig_display_select_file_option() intends to add the
additional menu mainly to clear the selected file information.
eficonfig_display_select_file_option() is not necessary for the
file selection process, so it should be outside of
eficonfig_select_file_handler().

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No change since v2

newly created in v2

 cmd/eficonfig.c                                | 13 +++++--------
 test/py/tests/test_eficonfig/test_eficonfig.py |  1 +
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Ilias Apalodimas Nov. 4, 2022, 3:12 p.m. UTC | #1
Hi Kojima-san

I think there's some information missing from the commit message. 

On Wed, Oct 26, 2022 at 07:43:41PM +0900, Masahisa Kojima wrote:
> eficonfig_select_file_handler() is commonly used to select the
> file.
> eficonfig_display_select_file_option() intends to add the
> additional menu mainly to clear the selected file information.
'eficonfig_display_select_file_option() adds an additional menu to clear
the selected file' sounds better?

> eficonfig_display_select_file_option() is not necessary for the
> file selection process, so it should be outside of
> eficonfig_select_file_handler().

In a followup patch I think we should rename eficonfig_select_file().  It's
a bit confusing to have both eficonfig_select_file_handler() and
eficonfig_select_file() and iirc the latter creates the menu for the file
selection. 

> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No change since v2
> 
> newly created in v2
> 
>  cmd/eficonfig.c                                | 13 +++++--------
>  test/py/tests/test_eficonfig/test_eficonfig.py |  1 +
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 2595dd9563..f6a99bd01a 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -968,7 +968,7 @@ efi_status_t eficonfig_process_clear_file_selection(void *data)
>  }
>  
>  static struct eficonfig_item select_file_menu_items[] = {
> -	{"Select File", eficonfig_process_select_file},

eficonfig_process_select_file() is not used anywhere anymore now.  We need
to get rid of the function declaration

> +	{"Select File", eficonfig_select_file_handler},

This is a different change right? It effectively allows you to choose
between files. 
Can we explain this change as well on the commit message?

>  	{"Clear", eficonfig_process_clear_file_selection},
>  	{"Quit", eficonfig_process_quit},
>  };
> @@ -980,12 +980,13 @@ static struct eficonfig_item select_file_menu_items[] = {
>   * @file_info:	pointer to the file information structure
>   * Return:	status code
>   */
> -efi_status_t eficonfig_display_select_file_option(struct eficonfig_select_file_info *file_info)
> +efi_status_t eficonfig_display_select_file_option(void *data)
>  {
>  	efi_status_t ret;
>  	struct efimenu *efi_menu;
>  
> -	select_file_menu_items[1].data = file_info;
> +	select_file_menu_items[0].data = data;
> +	select_file_menu_items[1].data = data;
>  	efi_menu = eficonfig_create_fixed_menu(select_file_menu_items,
>  					       ARRAY_SIZE(select_file_menu_items));
>  	if (!efi_menu)
> @@ -1016,10 +1017,6 @@ efi_status_t eficonfig_select_file_handler(void *data)
>  	struct eficonfig_select_file_info *tmp = NULL;
>  	struct eficonfig_select_file_info *file_info = data;
>  
> -	ret = eficonfig_display_select_file_option(file_info);
> -	if (ret != EFI_SUCCESS)
> -		return ret;
> -
>  	tmp = calloc(1, sizeof(struct eficonfig_select_file_info));
>  	if (!tmp)
>  		return EFI_OUT_OF_RESOURCES;
> @@ -1284,7 +1281,7 @@ static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char
>  	utf8_utf16_strcpy(&p, devname);
>  	u16_strlcat(file_name, file_info->current_path, len);
>  	ret = create_boot_option_entry(efi_menu, title, file_name,
> -				       eficonfig_select_file_handler, file_info);
> +				       eficonfig_display_select_file_option, file_info);
>  out:
>  	free(devname);
>  	free(file_name);
> diff --git a/test/py/tests/test_eficonfig/test_eficonfig.py b/test/py/tests/test_eficonfig/test_eficonfig.py
> index 99606d9c4b..102bfd7541 100644
> --- a/test/py/tests/test_eficonfig/test_eficonfig.py
> +++ b/test/py/tests/test_eficonfig/test_eficonfig.py
> @@ -349,6 +349,7 @@ def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
>          press_up_down_enter_and_wait(0, 1, True, 'Quit')
>          press_up_down_enter_and_wait(0, 0, True, 'No block device found!')
>          press_escape_key(False)
> +        press_escape_key(False)
>          check_current_is_maintenance_menu()
>          # Return to U-Boot console
>          press_escape_key(True)
> -- 
> 2.17.1
> 

Thanks
/Ilias
Masahisa Kojima Nov. 7, 2022, 2:31 a.m. UTC | #2
"Hi Ilias,

On Sat, 5 Nov 2022 at 00:12, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san
>
> I think there's some information missing from the commit message.
>
> On Wed, Oct 26, 2022 at 07:43:41PM +0900, Masahisa Kojima wrote:
> > eficonfig_select_file_handler() is commonly used to select the
> > file.
> > eficonfig_display_select_file_option() intends to add the
> > additional menu mainly to clear the selected file information.
> 'eficonfig_display_select_file_option() adds an additional menu to clear
> the selected file' sounds better?

OK, I will update the commit message.

>
> > eficonfig_display_select_file_option() is not necessary for the
> > file selection process, so it should be outside of
> > eficonfig_select_file_handler().
>
> In a followup patch I think we should rename eficonfig_select_file().  It's
> a bit confusing to have both eficonfig_select_file_handler() and
> eficonfig_select_file() and iirc the latter creates the menu for the file
> selection.

Yes, I agree.
I will rename the functions as follows.
 eficonfig_select_file_handler -> eficonfig_process_select_file()
 eficonfig_select_file -> eficonfig_show_file_selection()

>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No change since v2
> >
> > newly created in v2
> >
> >  cmd/eficonfig.c                                | 13 +++++--------
> >  test/py/tests/test_eficonfig/test_eficonfig.py |  1 +
> >  2 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 2595dd9563..f6a99bd01a 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -968,7 +968,7 @@ efi_status_t eficonfig_process_clear_file_selection(void *data)
> >  }
> >
> >  static struct eficonfig_item select_file_menu_items[] = {
> > -     {"Select File", eficonfig_process_select_file},
>
> eficonfig_process_select_file() is not used anywhere anymore now.  We need
> to get rid of the function declaration

Yes, I will remove it. Note that this function name
"eficonfig_process_select_file"
will be used for renaming "eficonfig_select_file_handler" in the followup patch.

>
> > +     {"Select File", eficonfig_select_file_handler},
>
> This is a different change right? It effectively allows you to choose
> between files.
> Can we explain this change as well on the commit message?

Yes, I will update the commit message.

Thank you for your feedback.

Regards,
Masahisa Kojima

>
> >       {"Clear", eficonfig_process_clear_file_selection},
> >       {"Quit", eficonfig_process_quit},
> >  };
> > @@ -980,12 +980,13 @@ static struct eficonfig_item select_file_menu_items[] = {
> >   * @file_info:       pointer to the file information structure
> >   * Return:   status code
> >   */
> > -efi_status_t eficonfig_display_select_file_option(struct eficonfig_select_file_info *file_info)
> > +efi_status_t eficonfig_display_select_file_option(void *data)
> >  {
> >       efi_status_t ret;
> >       struct efimenu *efi_menu;
> >
> > -     select_file_menu_items[1].data = file_info;
> > +     select_file_menu_items[0].data = data;
> > +     select_file_menu_items[1].data = data;
> >       efi_menu = eficonfig_create_fixed_menu(select_file_menu_items,
> >                                              ARRAY_SIZE(select_file_menu_items));
> >       if (!efi_menu)
> > @@ -1016,10 +1017,6 @@ efi_status_t eficonfig_select_file_handler(void *data)
> >       struct eficonfig_select_file_info *tmp = NULL;
> >       struct eficonfig_select_file_info *file_info = data;
> >
> > -     ret = eficonfig_display_select_file_option(file_info);
> > -     if (ret != EFI_SUCCESS)
> > -             return ret;
> > -
> >       tmp = calloc(1, sizeof(struct eficonfig_select_file_info));
> >       if (!tmp)
> >               return EFI_OUT_OF_RESOURCES;
> > @@ -1284,7 +1281,7 @@ static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char
> >       utf8_utf16_strcpy(&p, devname);
> >       u16_strlcat(file_name, file_info->current_path, len);
> >       ret = create_boot_option_entry(efi_menu, title, file_name,
> > -                                    eficonfig_select_file_handler, file_info);
> > +                                    eficonfig_display_select_file_option, file_info);
> >  out:
> >       free(devname);
> >       free(file_name);
> > diff --git a/test/py/tests/test_eficonfig/test_eficonfig.py b/test/py/tests/test_eficonfig/test_eficonfig.py
> > index 99606d9c4b..102bfd7541 100644
> > --- a/test/py/tests/test_eficonfig/test_eficonfig.py
> > +++ b/test/py/tests/test_eficonfig/test_eficonfig.py
> > @@ -349,6 +349,7 @@ def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
> >          press_up_down_enter_and_wait(0, 1, True, 'Quit')
> >          press_up_down_enter_and_wait(0, 0, True, 'No block device found!')
> >          press_escape_key(False)
> > +        press_escape_key(False)
> >          check_current_is_maintenance_menu()
> >          # Return to U-Boot console
> >          press_escape_key(True)
> > --
> > 2.17.1
> >
>
> Thanks
> /Ilias
diff mbox series

Patch

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 2595dd9563..f6a99bd01a 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -968,7 +968,7 @@  efi_status_t eficonfig_process_clear_file_selection(void *data)
 }
 
 static struct eficonfig_item select_file_menu_items[] = {
-	{"Select File", eficonfig_process_select_file},
+	{"Select File", eficonfig_select_file_handler},
 	{"Clear", eficonfig_process_clear_file_selection},
 	{"Quit", eficonfig_process_quit},
 };
@@ -980,12 +980,13 @@  static struct eficonfig_item select_file_menu_items[] = {
  * @file_info:	pointer to the file information structure
  * Return:	status code
  */
-efi_status_t eficonfig_display_select_file_option(struct eficonfig_select_file_info *file_info)
+efi_status_t eficonfig_display_select_file_option(void *data)
 {
 	efi_status_t ret;
 	struct efimenu *efi_menu;
 
-	select_file_menu_items[1].data = file_info;
+	select_file_menu_items[0].data = data;
+	select_file_menu_items[1].data = data;
 	efi_menu = eficonfig_create_fixed_menu(select_file_menu_items,
 					       ARRAY_SIZE(select_file_menu_items));
 	if (!efi_menu)
@@ -1016,10 +1017,6 @@  efi_status_t eficonfig_select_file_handler(void *data)
 	struct eficonfig_select_file_info *tmp = NULL;
 	struct eficonfig_select_file_info *file_info = data;
 
-	ret = eficonfig_display_select_file_option(file_info);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
 	tmp = calloc(1, sizeof(struct eficonfig_select_file_info));
 	if (!tmp)
 		return EFI_OUT_OF_RESOURCES;
@@ -1284,7 +1281,7 @@  static efi_status_t prepare_file_selection_entry(struct efimenu *efi_menu, char
 	utf8_utf16_strcpy(&p, devname);
 	u16_strlcat(file_name, file_info->current_path, len);
 	ret = create_boot_option_entry(efi_menu, title, file_name,
-				       eficonfig_select_file_handler, file_info);
+				       eficonfig_display_select_file_option, file_info);
 out:
 	free(devname);
 	free(file_name);
diff --git a/test/py/tests/test_eficonfig/test_eficonfig.py b/test/py/tests/test_eficonfig/test_eficonfig.py
index 99606d9c4b..102bfd7541 100644
--- a/test/py/tests/test_eficonfig/test_eficonfig.py
+++ b/test/py/tests/test_eficonfig/test_eficonfig.py
@@ -349,6 +349,7 @@  def test_efi_eficonfig(u_boot_console, efi_eficonfig_data):
         press_up_down_enter_and_wait(0, 1, True, 'Quit')
         press_up_down_enter_and_wait(0, 0, True, 'No block device found!')
         press_escape_key(False)
+        press_escape_key(False)
         check_current_is_maintenance_menu()
         # Return to U-Boot console
         press_escape_key(True)