| Message ID | 20221026104345.28714-4-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | eficonfig: add UEFI Secure Boot key maintenance interface | expand |
Hi Kojima-san On Wed, Oct 26, 2022 at 07:43:43PM +0900, Masahisa Kojima wrote: > This commit refactors change boot order implementation > to use 'eficonfig_entry' structure. Please add an explanation on why we are doing this, instead of what the patch is doing. I am assuming it cleans up some code and allows us to reuse eficonfig_entry since ->data is now pointing to an eficonfig_boot_order_data struct? > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > No update since v5 > > Changes in v5: > - remove direct access mode > > newly created in v4 > > cmd/eficonfig.c | 129 +++++++++++++++++++++++++----------------------- > 1 file changed, 67 insertions(+), 62 deletions(-) > > list_del(&tmp->list); [...] > @@ -1891,11 +1884,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > case KEY_MINUS: > if (efi_menu->active < efi_menu->count - 3) { > list_for_each_safe(pos, n, &efi_menu->list) { > - entry = list_entry(pos, struct eficonfig_boot_order, list); > + entry = list_entry(pos, struct eficonfig_entry, list); > if (entry->num == efi_menu->active) > break; > } > - tmp = list_entry(pos->next, struct eficonfig_boot_order, list); > + tmp = list_entry(pos->next, struct eficonfig_entry, list); > entry->num++; > tmp->num--; > list_del(&entry->list); > @@ -1921,9 +1914,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > case KEY_SPACE: > if (efi_menu->active < efi_menu->count - 2) { > list_for_each_safe(pos, n, &efi_menu->list) { > - entry = list_entry(pos, struct eficonfig_boot_order, list); > + entry = list_entry(pos, struct eficonfig_entry, list); > if (entry->num == efi_menu->active) { > - entry->active = entry->active ? false : true; > + struct eficonfig_boot_order_data *data = entry->data; > + > + data->active = data->active ? false : true; data->active = !!data->active seems a bit better here imho > return EFI_NOT_READY; > } > } > @@ -1949,12 +1944,13 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_menu, > u32 boot_index, bool active) > { > + char *title, *p; > efi_status_t ret; > efi_uintn_t size; > void *load_option; > struct efi_load_option lo; > u16 varname[] = u"Boot####"; > - struct eficonfig_boot_order *entry; > + struct eficonfig_boot_order_data *data; > > efi_create_indexed_name(varname, sizeof(varname), "Boot", boot_index); > load_option = efi_get_var(varname, &efi_global_variable_guid, &size); > @@ -1962,31 +1958,38 @@ static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_me > return EFI_SUCCESS; > > ret = efi_deserialize_load_option(&lo, load_option, &size); > - if (ret != EFI_SUCCESS) { > - free(load_option); > - return ret; > + if (ret != EFI_SUCCESS) > + goto out; > + > + data = calloc(1, sizeof(struct eficonfig_boot_order_data)); sizeof(*data) > + if (!data) { > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > } > > - entry = calloc(1, sizeof(struct eficonfig_boot_order)); sizeof(*entry) > - if (!entry) { > - free(load_option); > - return EFI_OUT_OF_RESOURCES; > + title = calloc(1, utf16_utf8_strlen(lo.label) + 1); > + if (!title) { > + free(data); > + ret = EFI_OUT_OF_RESOURCES; > + goto out; > } > + p = title; > + utf16_utf8_strcpy(&p, lo.label); > > - entry->description = u16_strdup(lo.label); > - if (!entry->description) { > - free(load_option); > - free(entry); > - return EFI_OUT_OF_RESOURCES; > + data->boot_index = boot_index; > + data->active = active; > + > + ret = eficonfig_append_menu_entry(efi_menu, title, NULL, data); > + if (ret != EFI_SUCCESS) { > + free(data); > + free(title); Thanks /Ilias
Hi Ilias, On Sat, 5 Nov 2022 at 07:08, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Kojima-san > > On Wed, Oct 26, 2022 at 07:43:43PM +0900, Masahisa Kojima wrote: > > This commit refactors change boot order implementation > > to use 'eficonfig_entry' structure. > > Please add an explanation on why we are doing this, instead of what the > patch is doing. I am assuming it cleans up some code and allows us to reuse > eficonfig_entry since ->data is now pointing to an > eficonfig_boot_order_data struct? Yes, I will update the commit message. > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > No update since v5 > > > > Changes in v5: > > - remove direct access mode > > > > newly created in v4 > > > > cmd/eficonfig.c | 129 +++++++++++++++++++++++++----------------------- > > 1 file changed, 67 insertions(+), 62 deletions(-) > > > > list_del(&tmp->list); > > [...] > > > @@ -1891,11 +1884,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > > case KEY_MINUS: > > if (efi_menu->active < efi_menu->count - 3) { > > list_for_each_safe(pos, n, &efi_menu->list) { > > - entry = list_entry(pos, struct eficonfig_boot_order, list); > > + entry = list_entry(pos, struct eficonfig_entry, list); > > if (entry->num == efi_menu->active) > > break; > > } > > - tmp = list_entry(pos->next, struct eficonfig_boot_order, list); > > + tmp = list_entry(pos->next, struct eficonfig_entry, list); > > entry->num++; > > tmp->num--; > > list_del(&entry->list); > > @@ -1921,9 +1914,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > > case KEY_SPACE: > > if (efi_menu->active < efi_menu->count - 2) { > > list_for_each_safe(pos, n, &efi_menu->list) { > > - entry = list_entry(pos, struct eficonfig_boot_order, list); > > + entry = list_entry(pos, struct eficonfig_entry, list); > > if (entry->num == efi_menu->active) { > > - entry->active = entry->active ? false : true; > > + struct eficonfig_boot_order_data *data = entry->data; > > + > > + data->active = data->active ? false : true; > > data->active = !!data->active seems a bit better here imho Yes, I will replace with "data->active = !data->active;" here. > > > return EFI_NOT_READY; > > } > > } > > @@ -1949,12 +1944,13 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) > > static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_menu, > > u32 boot_index, bool active) > > { > > + char *title, *p; > > efi_status_t ret; > > efi_uintn_t size; > > void *load_option; > > struct efi_load_option lo; > > u16 varname[] = u"Boot####"; > > - struct eficonfig_boot_order *entry; > > + struct eficonfig_boot_order_data *data; > > > > efi_create_indexed_name(varname, sizeof(varname), "Boot", boot_index); > > load_option = efi_get_var(varname, &efi_global_variable_guid, &size); > > @@ -1962,31 +1958,38 @@ static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_me > > return EFI_SUCCESS; > > > > ret = efi_deserialize_load_option(&lo, load_option, &size); > > - if (ret != EFI_SUCCESS) { > > - free(load_option); > > - return ret; > > + if (ret != EFI_SUCCESS) > > + goto out; > > + > > + data = calloc(1, sizeof(struct eficonfig_boot_order_data)); > > sizeof(*data) OK. > > > + if (!data) { > > + ret = EFI_OUT_OF_RESOURCES; > > + goto out; > > } > > > > - entry = calloc(1, sizeof(struct eficonfig_boot_order)); > > sizeof(*entry) OK. Thanks, Masahisa Kojima > > > - if (!entry) { > > - free(load_option); > > - return EFI_OUT_OF_RESOURCES; > > + title = calloc(1, utf16_utf8_strlen(lo.label) + 1); > > + if (!title) { > > + free(data); > > + ret = EFI_OUT_OF_RESOURCES; > > + goto out; > > } > > + p = title; > > + utf16_utf8_strcpy(&p, lo.label); > > > > - entry->description = u16_strdup(lo.label); > > - if (!entry->description) { > > - free(load_option); > > - free(entry); > > - return EFI_OUT_OF_RESOURCES; > > + data->boot_index = boot_index; > > + data->active = active; > > + > > + ret = eficonfig_append_menu_entry(efi_menu, title, NULL, data); > > + if (ret != EFI_SUCCESS) { > > + free(data); > > + free(title); > > Thanks > /Ilias
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0cb0770ac3..c765b795d0 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -93,20 +93,14 @@ struct eficonfig_boot_selection_data { }; /** - * struct eficonfig_boot_order - structure to be used to update BootOrder variable + * struct eficonfig_boot_order_data - structure to be used to update BootOrder variable * - * @num: index in the menu entry - * @description: pointer to the description string * @boot_index: boot option index * @active: flag to include the boot option into BootOrder variable - * @list: list structure */ -struct eficonfig_boot_order { - u32 num; - u16 *description; +struct eficonfig_boot_order_data { u32 boot_index; bool active; - struct list_head list; }; /** @@ -1814,7 +1808,7 @@ static void eficonfig_display_change_boot_order(struct efimenu *efi_menu) { bool reverse; struct list_head *pos, *n; - struct eficonfig_boot_order *entry; + struct eficonfig_entry *entry; printf(ANSI_CLEAR_CONSOLE ANSI_CURSOR_POSITION "\n ** Change Boot Order **\n" @@ -1830,7 +1824,7 @@ static void eficonfig_display_change_boot_order(struct efimenu *efi_menu) /* draw boot option list */ list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); + entry = list_entry(pos, struct eficonfig_entry, list); reverse = (entry->num == efi_menu->active); printf(ANSI_CURSOR_POSITION, entry->num + 4, 7); @@ -1839,13 +1833,13 @@ static void eficonfig_display_change_boot_order(struct efimenu *efi_menu) puts(ANSI_COLOR_REVERSE); if (entry->num < efi_menu->count - 2) { - if (entry->active) + if (((struct eficonfig_boot_order_data *)entry->data)->active) printf("[*] "); else printf("[ ] "); } - printf("%ls", entry->description); + printf("%s", entry->title); if (reverse) puts(ANSI_COLOR_RESET); @@ -1862,9 +1856,8 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) { int esc = 0; struct list_head *pos, *n; - struct eficonfig_boot_order *tmp; enum bootmenu_key key = KEY_NONE; - struct eficonfig_boot_order *entry; + struct eficonfig_entry *entry, *tmp; while (1) { bootmenu_loop(NULL, &key, &esc); @@ -1873,11 +1866,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) case KEY_PLUS: if (efi_menu->active > 0) { list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); + entry = list_entry(pos, struct eficonfig_entry, list); if (entry->num == efi_menu->active) break; } - tmp = list_entry(pos->prev, struct eficonfig_boot_order, list); + tmp = list_entry(pos->prev, struct eficonfig_entry, list); entry->num--; tmp->num++; list_del(&tmp->list); @@ -1891,11 +1884,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) case KEY_MINUS: if (efi_menu->active < efi_menu->count - 3) { list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); + entry = list_entry(pos, struct eficonfig_entry, list); if (entry->num == efi_menu->active) break; } - tmp = list_entry(pos->next, struct eficonfig_boot_order, list); + tmp = list_entry(pos->next, struct eficonfig_entry, list); entry->num++; tmp->num--; list_del(&entry->list); @@ -1921,9 +1914,11 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) case KEY_SPACE: if (efi_menu->active < efi_menu->count - 2) { list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); + entry = list_entry(pos, struct eficonfig_entry, list); if (entry->num == efi_menu->active) { - entry->active = entry->active ? false : true; + struct eficonfig_boot_order_data *data = entry->data; + + data->active = data->active ? false : true; return EFI_NOT_READY; } } @@ -1949,12 +1944,13 @@ static efi_status_t eficonfig_choice_change_boot_order(struct efimenu *efi_menu) static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_menu, u32 boot_index, bool active) { + char *title, *p; efi_status_t ret; efi_uintn_t size; void *load_option; struct efi_load_option lo; u16 varname[] = u"Boot####"; - struct eficonfig_boot_order *entry; + struct eficonfig_boot_order_data *data; efi_create_indexed_name(varname, sizeof(varname), "Boot", boot_index); load_option = efi_get_var(varname, &efi_global_variable_guid, &size); @@ -1962,31 +1958,38 @@ static efi_status_t eficonfig_add_change_boot_order_entry(struct efimenu *efi_me return EFI_SUCCESS; ret = efi_deserialize_load_option(&lo, load_option, &size); - if (ret != EFI_SUCCESS) { - free(load_option); - return ret; + if (ret != EFI_SUCCESS) + goto out; + + data = calloc(1, sizeof(struct eficonfig_boot_order_data)); + if (!data) { + ret = EFI_OUT_OF_RESOURCES; + goto out; } - entry = calloc(1, sizeof(struct eficonfig_boot_order)); - if (!entry) { - free(load_option); - return EFI_OUT_OF_RESOURCES; + title = calloc(1, utf16_utf8_strlen(lo.label) + 1); + if (!title) { + free(data); + ret = EFI_OUT_OF_RESOURCES; + goto out; } + p = title; + utf16_utf8_strcpy(&p, lo.label); - entry->description = u16_strdup(lo.label); - if (!entry->description) { - free(load_option); - free(entry); - return EFI_OUT_OF_RESOURCES; + data->boot_index = boot_index; + data->active = active; + + ret = eficonfig_append_menu_entry(efi_menu, title, NULL, data); + if (ret != EFI_SUCCESS) { + free(data); + free(title); + goto out; } - entry->num = efi_menu->count++; - entry->boot_index = boot_index; - entry->active = active; - list_add_tail(&entry->list, &efi_menu->list); +out: free(load_option); - return EFI_SUCCESS; + return ret; } /** @@ -2001,8 +2004,8 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi u16 *bootorder, efi_uintn_t num) { u32 i; + char *title; efi_status_t ret; - struct eficonfig_boot_order *entry; /* list the load option in the order of BootOrder variable */ for (i = 0; i < num; i++) { @@ -2029,27 +2032,25 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi } /* add "Save" and "Quit" entries */ - entry = calloc(1, sizeof(struct eficonfig_boot_order)); - if (!entry) + title = strdup("Save"); + if (!title) { + ret = EFI_OUT_OF_RESOURCES; goto out; + } - entry->num = efi_menu->count++; - entry->description = u16_strdup(u"Save"); - list_add_tail(&entry->list, &efi_menu->list); - - entry = calloc(1, sizeof(struct eficonfig_boot_order)); - if (!entry) + ret = eficonfig_append_menu_entry(efi_menu, title, NULL, NULL); + if (ret != EFI_SUCCESS) goto out; - entry->num = efi_menu->count++; - entry->description = u16_strdup(u"Quit"); - list_add_tail(&entry->list, &efi_menu->list); + ret = eficonfig_append_quit_entry(efi_menu); + if (ret != EFI_SUCCESS) + goto out; efi_menu->active = 0; return EFI_SUCCESS; out: - return EFI_OUT_OF_RESOURCES; + return ret; } /** @@ -2065,7 +2066,7 @@ static efi_status_t eficonfig_process_change_boot_order(void *data) efi_status_t ret; efi_uintn_t num, size; struct list_head *pos, *n; - struct eficonfig_boot_order *entry; + struct eficonfig_entry *entry; struct efimenu *efi_menu; efi_menu = calloc(1, sizeof(struct efimenu)); @@ -2096,9 +2097,16 @@ static efi_status_t eficonfig_process_change_boot_order(void *data) /* create new BootOrder */ count = 0; list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); - if (entry->active) - new_bootorder[count++] = entry->boot_index; + struct eficonfig_boot_order_data *data; + + entry = list_entry(pos, struct eficonfig_entry, list); + /* exit the loop when iteration reaches "Save" */ + if (!strncmp(entry->title, "Save", strlen("Save"))) + break; + + data = entry->data; + if (data->active) + new_bootorder[count++] = data->boot_index; } size = count * sizeof(u16); @@ -2117,15 +2125,12 @@ static efi_status_t eficonfig_process_change_boot_order(void *data) } } out: + free(bootorder); list_for_each_safe(pos, n, &efi_menu->list) { - entry = list_entry(pos, struct eficonfig_boot_order, list); - list_del(&entry->list); - free(entry->description); - free(entry); + entry = list_entry(pos, struct eficonfig_entry, list); + free(entry->data); } - - free(bootorder); - free(efi_menu); + eficonfig_destroy(efi_menu); /* to stay the parent menu */ ret = (ret == EFI_ABORTED) ? EFI_NOT_READY : ret;
This commit refactors change boot order implementation to use 'eficonfig_entry' structure. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- No update since v5 Changes in v5: - remove direct access mode newly created in v4 cmd/eficonfig.c | 129 +++++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 62 deletions(-)