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

[Masahisa Kojima <masahisa.kojima@linaro.org>]

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