| Message ID | 20221026104345.28714-2-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | eficonfig: add UEFI Secure Boot key maintenance interface | expand |
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
"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 --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)
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(-)