Re: [PATCH] efi_loader: Clean up efi_dp_append and efi_dp_concat
Hi Heinrich
On Mon, 13 Nov 2023 at 09:37, Heinrich Schuchardt 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
Re: [PATCH] efi_loader: Clean up efi_dp_append and efi_dp_concat
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
Signed-off-by: Ilias Apalodimas
---
Kojima-san, I think this might affect your EFI HTTP boot series.
I don't mind waiting for this and merging it after your series goes in
(and adjust it). The changes should be trivial anyway
cmd/bootefi.c | 4 +-
cmd/eficonfig.c| 7 ++-
cmd/efidebug.c | 6 +-
include/efi_loader.h | 5 +-
lib/efi_loader/efi_bootmgr.c | 2 +-
lib/efi_loader/efi_boottime.c | 2 +-
lib/efi_loader/efi_device_path.c | 66 ++
lib/efi_loader/efi_device_path_utilities.c | 2 +-
8 files changed, 32 insertions(+), 62 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 20e5c94a33a4..eb839136bf7e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -513,8 +513,8 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t
source_size)
goto out;
msg_path = file_path;
} else {
- file_path = efi_dp_append(bootefi_device_path,
- bootefi_image_path);
+ file_path = efi_dp_concat(bootefi_device_path,
+ bootefi_image_path, false);
msg_path = bootefi_image_path;
log_debug("Loaded from disk\n");
}
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index e6e8a0a488e7..470adb7eddbf 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -528,7 +528,7 @@ struct efi_device_path *eficonfig_create_device_path(struct
efi_device_path *dp_
p += fp_size;
*((struct efi_device_path *)p) = END;
- dp = efi_dp_append(dp_volume, (struct efi_device_path *)buf);
+ dp = efi_dp_concat(dp_volume, (struct efi_device_path *)buf, false);
free(buf);
return dp;
@@ -1481,7 +1481,8 @@ static efi_status_t eficonfig_edit_boot_option(u16
*varname, struct eficonfig_bo
ret = EFI_OUT_OF_RESOURCES;
goto out;
}
- initrd_dp = efi_dp_append((const struct efi_device_path
*)_dp, dp);
+ initrd_dp = efi_dp_concat((const struct efi_device_path
*)_dp,
+ dp, false);
efi_free_pool(dp);
}
@@ -1492,7 +1493,7 @@ static efi_status_t eficonfig_edit_boot_option(u16
*varname, struct eficonfig_bo
}
final_dp_size = efi_dp_size(dp) + sizeof(END);
if (initrd_dp) {
- final_dp = efi_dp_concat(dp, initrd_dp);
+ final_dp = efi_dp_concat(dp, initrd_dp, true);
final_dp_size += efi_dp_size(initrd_dp) + sizeof(END);
} else {
final_dp = efi_dp_dup(dp);
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 201531ac19fc..a62298bf5987 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -698,8 +698,8 @@ struct efi_device_path *create_initrd_dp(const char *dev,
const char *part,
if (!short_fp)
short_fp = tmp_fp;
- initrd_dp = efi_dp_append((const struct efi_device_path *)_dp,
- short_fp);
+ initrd_dp = efi_dp_concat((const struct efi_device_path *)_dp,
+ short_fp, false);
out:
efi_free_pool(tmp_dp);
@@ -841,7 +841,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
goto out;
}
- 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
[PATCH] efi_loader: Clean up efi_dp_append and efi_dp_concat
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.
Signed-off-by: Ilias Apalodimas
---
Kojima-san, I think this might affect your EFI HTTP boot series.
I don't mind waiting for this and merging it after your series goes in
(and adjust it). The changes should be trivial anyway
cmd/bootefi.c | 4 +-
cmd/eficonfig.c| 7 ++-
cmd/efidebug.c | 6 +-
include/efi_loader.h | 5 +-
lib/efi_loader/efi_bootmgr.c | 2 +-
lib/efi_loader/efi_boottime.c | 2 +-
lib/efi_loader/efi_device_path.c | 66 ++
lib/efi_loader/efi_device_path_utilities.c | 2 +-
8 files changed, 32 insertions(+), 62 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 20e5c94a33a4..eb839136bf7e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -513,8 +513,8 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t
source_size)
goto out;
msg_path = file_path;
} else {
- file_path = efi_dp_append(bootefi_device_path,
- bootefi_image_path);
+ file_path = efi_dp_concat(bootefi_device_path,
+ bootefi_image_path, false);
msg_path = bootefi_image_path;
log_debug("Loaded from disk\n");
}
diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index e6e8a0a488e7..470adb7eddbf 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -528,7 +528,7 @@ struct efi_device_path *eficonfig_create_device_path(struct
efi_device_path *dp_
p += fp_size;
*((struct efi_device_path *)p) = END;
- dp = efi_dp_append(dp_volume, (struct efi_device_path *)buf);
+ dp = efi_dp_concat(dp_volume, (struct efi_device_path *)buf, false);
free(buf);
return dp;
@@ -1481,7 +1481,8 @@ static efi_status_t eficonfig_edit_boot_option(u16
*varname, struct eficonfig_bo
ret = EFI_OUT_OF_RESOURCES;
goto out;
}
- initrd_dp = efi_dp_append((const struct efi_device_path
*)_dp, dp);
+ initrd_dp = efi_dp_concat((const struct efi_device_path
*)_dp,
+ dp, false);
efi_free_pool(dp);
}
@@ -1492,7 +1493,7 @@ static efi_status_t eficonfig_edit_boot_option(u16
*varname, struct eficonfig_bo
}
final_dp_size = efi_dp_size(dp) + sizeof(END);
if (initrd_dp) {
- final_dp = efi_dp_concat(dp, initrd_dp);
+ final_dp = efi_dp_concat(dp, initrd_dp, true);
final_dp_size += efi_dp_size(initrd_dp) + sizeof(END);
} else {
final_dp = efi_dp_dup(dp);
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 201531ac19fc..a62298bf5987 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -698,8 +698,8 @@ struct efi_device_path *create_initrd_dp(const char *dev,
const char *part,
if (!short_fp)
short_fp = tmp_fp;
- initrd_dp = efi_dp_append((const struct efi_device_path *)_dp,
- short_fp);
+ initrd_dp = efi_dp_concat((const struct efi_device_path *)_dp,
+ short_fp, false);
out:
efi_free_pool(tmp_dp);
@@ -841,7 +841,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
goto out;
}
- 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
