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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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