Hi Heinrich,
> > + efi_status_t ret;
> > + void *buf = NULL;
> > +
> > + *size = 0;
> > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> > + if (ret == EFI_BUFFER_TOO_SMALL) {
> > + buf = malloc(*size);
>
> Please, always check the output of malloc(), e.g.
>
> if (!buf)
> ret = EFI_OUT_OF_RESOURCES;
> else
>
I just moved the function from lib/efi_loader/efi_bootmgr.c (check for
get_var()) and completely missed that. I'll fix it on the next revision.
> > + ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> > + }
> > +
> > + if (ret != EFI_SUCCESS) {
> > + free(buf);
> > + *size = 0;
> > + return NULL;
> > + }
> > +
> > + return buf;
> > +}
> > +
> > +/**
> > + * efi_dp_instance_by_idx() - Get a file path with a specific index
> > + *
> > + * @name: device path array
> > + * @size: size of the discovered device path
> > + * @idx: index of the device path
> > + *
> > + * Return: device path or NULL. Caller must free the returned value
> > + */
> > +
> > +struct
> > +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> > + efi_uintn_t *size, int idx)
> > +{
>
> idx should be of an unsigned type as we cannot have negative indices.
>
> > + struct efi_device_path *instance = NULL;
> > + efi_uintn_t instance_size = 0;
> > +
> > + if (!efi_dp_is_multi_instance(dp))
>
> Given a device path with one instance, why don't you allow me to read
> index 0? I assume this check can be removed.
>
Yea, but why would you ever use that? you can just retrieve the deserialized
device path directly if there are no other instances.
> > + return NULL;
> > +
> > + while (idx >= 0 && dp) {
> > + instance = efi_dp_get_next_instance(&dp, &instance_size);
> > + if (idx && instance) {
> > + efi_free_pool(instance);
> > + instance_size = 0;
> > + instance = NULL;
> > + }
> > + idx--;
> > + }
> > + *size = instance_size;
> > +
> > + return instance;
>
> This can be simplified with unsigned idx:
>
> for (; dp; --idx) {
> instance = efi_dp_get_next_instance(&dp, size);
> if (!idx) {
> return instance;
> efi_free_pool(instance);
> }
> return NULL;
Ok I'll have a look
>
> > +}
> > +
> > +/**
> > + * create_boot_var_indexed() - Return Boot#### name were #### is replaced
> > by
> > + * the value of BootCurrent
> > + *
> > + * @var_name: variable name
> > + * @var_name_size: size of var_name
> > + *
> > + * Return: Status code
> > + */
> > +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t
> > var_name_size)
> > +{
> > + efi_uintn_t boot_order_size;
>
> You are reading BootCurrent, not BootOrder.
>
> > + efi_status_t ret;
> > + u16 boot_order;
>
> Same here.
>
Ok
> > + if (!file_path) {
> > + printf("Instance not found\n");
>
> This message is not enough for a user to take action. I suggest
>
> ("Missing file path with index %d in %ls", idx, var_name)
>
> We want to use logging. I assume this is not an error. Can we use
> log_debug() here?
>
That's a leftover from my opwn debugging that I neglected to remove prior to
the RFC. I'll just add a log_debug()
> > + return NULL;
> > + }
> > +
> > + return file_path;
> > +}
> >
>
> Some other functions would fit into the same C module. Candidates are:
>
> * efi_create_indexed_name()
> * efi_deserialize_load_option()
> * efi_serialize_load_option()
>
> But that can be done in a separate patch.
Yea, that's the idea, we can also use the efi_get_var() in multiple places,
but I'll send different patches for that, once we merge this.
I assume we agree on the architecture. If so I'll fix the selftests and post a
proper patch
Regards
/Ilias
>
> Best regards
>
> Heinrich
