[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

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