Re: [PATCH v6 3/5] eficonfig: refactor change boot order implementation

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