[PATCH] efi_loader: Clean up efi_dp_append and efi_dp_concat
Ilias Apalodimas
ilias.apalodimas at linaro.org
Mon Nov 13 21:31:38 CET 2023
Hi Heinrich
On Mon, 13 Nov 2023 at 09:37, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 11/7/23 18:36, Ilias Apalodimas wrote:
> > Looking back at the initrd storing functionality, we introduced three
> > functions, efi_dp_append_or_concatenate(), efi_dp_append/concat(). In
> > hindsight we could have simplified that by a lot. First of all none of
> > the functions append anything. They all allocate a new device path and
> > concatenate the contents of two device paths in one. A boolean parameter
> > controls the final device path -- if that's true an end node is injected
> > between the two device paths.
> >
> > So let's rewrite this and make it a bit easier to read. Get rid of
> > efi_dp_append(), efi_dp_concat() and rename
> > efi_dp_append_or_concatenate() to efi_dp_concat(). This is far more
> > intuitive and the only adjustment that is needed is an extra boolean
> > argument on all callsites.
>
> After this patch we still have efi_dp_append_instance(). The only
> difference to efi_dp_concat(,,true) seems only to be the type of end
> node used as separator.
>
> Hence the last argument of efi_dp_contat() should be be either of:
>
> * 0
> * DEVICE_PATH_SUB_TYPE_INSTANCE_END
> * DEVICE_PATH_SUB_TYPE_END
ah fair enough, I'll clean that up as well. Add an enum as a 3rd argument then?
rd_dp) + sizeof(END);
> > } else {
> > final_dp = efi_dp_dup(dp);
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>
> >
> > - final_fp = efi_dp_concat(file_path, initrd_dp);
> > + final_fp = efi_dp_concat(file_path, initrd_dp, true);
> > if (!final_fp) {
> > printf("Cannot create final device path\n");
> > r = CMD_RET_FAILURE;
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index e24410505f40..398cd20c7ae6 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -808,8 +808,6 @@ efi_uintn_t efi_dp_instance_size(const struct efi_device_path *dp);
> > /* size of multi-instance device path excluding end node */
> > efi_uintn_t efi_dp_size(const struct efi_device_path *dp);
> > struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp);
> > -struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,
> > - const struct efi_device_path *dp2);
> > struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
> > const struct efi_device_path *node);
> > /* Create a device path node of given type, sub-type, length */
> > @@ -928,7 +926,8 @@ struct efi_load_option {
> > struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
> > const efi_guid_t *guid);
> > struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> > - const struct efi_device_path *dp2);
> > + const struct efi_device_path *dp2,
> > + bool split_end_node);
> > struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
> > efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> > efi_uintn_t *size);
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c83..646c7c7faaad 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -110,7 +110,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles,
> > if (!dp)
> > continue;
> >
> > - dp = efi_dp_append(dp, fp);
> > + dp = efi_dp_concat(dp, fp, false);
> > if (!dp)
> > continue;
> >
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 0b7579cb5af1..709ea07655c2 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1821,7 +1821,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> > if (device_path) {
> > info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
> >
> > - dp = efi_dp_append(device_path, file_path);
> > + dp = efi_dp_concat(device_path, file_path, false);
> > if (!dp) {
> > ret = EFI_OUT_OF_RESOURCES;
> > goto failure;
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index ed7214f3a347..ad79b65843e9 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -272,30 +272,27 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
> > }
> >
> > /**
> > - * efi_dp_append_or_concatenate() - Append or concatenate two device paths.
> > - * Concatenated device path will be separated
> > - * by a sub-type 0xff end node
> > + * efi_dp_concat() - Concatenate two device paths and add and terminate them
> > + * with an end node.
> > *
> > - * @dp1: First device path
> > - * @dp2: Second device path
> > - * @concat: If true the two device paths will be concatenated and separated
> > - * by an end of entrire device path sub-type 0xff end node.
> > - * If true the second device path will be appended to the first and
> > - * terminated by an end node
> > + * @dp1: First device path
> > + * @dp2: Second device path
> > + * @split_end_node: If true the two device paths will be concatenated and
> > + * separated by an end node.
>
> This description is unclear. Do you mean DEVICE_PATH_SUB_TYPE_END or
> DEVICE_PATH_SUB_TYPE_INSTANCE_END?
I think it's DEVICE_PATH_SUB_TYPE_END, I'll double check
>
> We would better make the parameter a three-state value:
>
> * 0
> * DEVICE_PATH_SUB_TYPE_INSTANCE_END
> * DEVICE_PATH_SUB_TYPE_END
Ok i'll add an enum
>
> > + * If false the second device path will be concatenated to the
> > + * first one as-is.
> > *
> > * Return:
> > * concatenated device path or NULL. Caller must free the returned value
> > */
> > -static struct
> > -efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
> > - const struct efi_device_path *dp2,
> > - bool concat)
> > +struct
> > +efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> > + const struct efi_device_path *dp2,
> > + bool split_end_node)
> > {
> > struct efi_device_path *ret;
> > size_t end_size = sizeof(END);
>
> This assignment is not needed if (!dp1 || !dp2).
>
> >
> > - if (concat)
> > - end_size = 2 * sizeof(END);
> > if (!dp1 && !dp2) {
> > /* return an end node */
> > ret = efi_dp_dup(&END);
> > @@ -307,14 +304,18 @@ efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
> > /* both dp1 and dp2 are non-null */
> > unsigned sz1 = efi_dp_size(dp1);
> > unsigned sz2 = efi_dp_size(dp2);
> > - void *p = efi_alloc(sz1 + sz2 + end_size);
> > + void *p;
> > +
> > + if (split_end_node)
> > + end_size = 2 * sizeof(END);
>
> We assign end_size twice in this case. We should instead move the first
> assignment to the if:
>
> if (!split_end_node)
> end_size = sizeof(END);
> else
> end_size = 2 * sizeof(END);
>
[...]
Thanks
/Ilias
More information about the U-Boot
mailing list