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