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 ilias.apalodimas@linaro.org

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 *)&id_dp, dp);
  

• initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_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 *)&id_dp,

• 		  short_fp);
  

• initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_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 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?

We would better make the parameter a three-state value:

* 0 * DEVICE_PATH_SUB_TYPE_INSTANCE_END * DEVICE_PATH_SUB_TYPE_END

• •     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);

Best regards

Heinrich

• p = efi_alloc(sz1 + sz2 + end_size);
  

  if (!p) return NULL; ret = p; memcpy(p, dp1, sz1); p += sz1;

• if (concat) {
  

• if (split_end_node) {
  memcpy(p, &END, sizeof(END));
  p += sizeof(END);
  

  }

@@ -328,37 +329,6 @@ efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1, return ret; }

-/**

• • efi_dp_append() - Append a device to an existing device path.
• • @dp1: First device path
• • @dp2: Second device path
• • Return:
• • concatenated device path or NULL. Caller must free the returned value
• */

-struct efi_device_path *efi_dp_append(const struct efi_device_path *dp1,

• 		      const struct efi_device_path *dp2)
  

-{

• return efi_dp_append_or_concatenate(dp1, dp2, false);

-}

-/**

• • efi_dp_concat() - Concatenate 2 device paths. The final device path will

• •               contain two device paths separated by and end node (0xff).
    

• • @dp1: First device path
• • @dp2: Second device path
• • Return:
• • concatenated device path or NULL. Caller must free the returned value
• */

-struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,

• 		      const struct efi_device_path *dp2)
  

-{

• return efi_dp_append_or_concatenate(dp1, dp2, true);

-}

• struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp, const struct efi_device_path *node) {

diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index a07d9bab3a3c..fa211aee495e 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -77,7 +77,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);

• return EFI_EXIT(efi_dp_append(src1, src2));

• return EFI_EXIT(efi_dp_concat(src1, src2, false)); }

  /*

-- 2.40.1