Re: [RFC 01/13] cmd: bootefi: unfold do_bootefi_image()

Hi Ilias,

On Fri, Oct 27, 2023 at 03:23:07PM +0300, Ilias Apalodimas wrote:

Akashi-san

On Fri, 27 Oct 2023 at 04:00, Tom Rini trini@konsulko.com wrote:

...

On Fri, Oct 27, 2023 at 09:25:44AM +0900, AKASHI Takahiro wrote:

...

On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:

...

On 10/26/23 07:30, AKASHI Takahiro wrote:

...

Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding refactor work.

I don't disagree with the patchset, but what the patch does is pretty obvious.

Yeah, that is what I tried to do in this patch series, i.e. each step be as trivial as possible to ensure that the semantics is unchanged while the code is being morphed.

It would help a lot to briefly describe how the unfolding process helps the refactoring.

Not sure, but will add a few more words.

Thanks, -Takahiro Akashi

Thanks /Ilias

...

...

...

...

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/bootefi.c | 101 ++++++++++++++++++-------------------------------- 1 file changed, 37 insertions(+), 64 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 20e5c94a33a4..1b28bf5a318d 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -425,58 +425,6 @@ static int do_efibootmgr(void) return CMD_RET_SUCCESS; }

-/**

• • do_bootefi_image() - execute EFI binary
• • Set up memory image for the binary to be loaded, prepare device path, and
• • then call do_bootefi_exec() to execute it.
• • @image_opt: string with image start address
• • @size_opt: string with image size or NULL
• • Return: status code
• */

-static int do_bootefi_image(const char *image_opt, const char *size_opt) -{

• void *image_buf;
• unsigned long addr, size;
• efi_status_t ret;

-#ifdef CONFIG_CMD_BOOTEFI_HELLO

• if (!strcmp(image_opt, "hello")) {

•     image_buf = __efi_helloworld_begin;
  

•     size = __efi_helloworld_end - __efi_helloworld_begin;
  

•     efi_clear_bootdev();
  

• } else

-#endif

• {

•     addr = strtoul(image_opt, NULL, 16);
  

•     /* Check that a numeric value was passed */
  

•     if (!addr)
  

•             return CMD_RET_USAGE;
  

•     image_buf = map_sysmem(addr, 0);
  

•     if (size_opt) {
  

•             size = strtoul(size_opt, NULL, 16);
  

•             if (!size)
  

•                     return CMD_RET_USAGE;
  

•             efi_clear_bootdev();
  

•     } else {
  

•             if (image_buf != image_addr) {
  

•                     log_err("No UEFI binary known at %s\n",
  

•                             image_opt);
  

•                     return CMD_RET_FAILURE;
  

•             }
  

•             size = image_size;
  

•     }
  

• }
• ret = efi_run_image(image_buf, size);
• if (ret != EFI_SUCCESS)

•     return CMD_RET_FAILURE;
  

• return CMD_RET_SUCCESS;

-}

• /**
  • efi_run_image() - run loaded UEFI image

@@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { efi_status_t ret;

• char *img_addr, *img_size, *str_copy, *pos;
• void *fdt;

• char *p;

• void *fdt, *image_buf;

• unsigned long addr, size;

      if (argc < 2)
              return CMD_RET_USAGE;
  

@@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc, if (!strcmp(argv[1], "selftest")) return do_efi_selftest(); #endif

• str_copy = strdup(argv[1]);
• if (!str_copy) {

•     log_err("Out of memory\n");
  

•     return CMD_RET_FAILURE;
  

+#ifdef CONFIG_CMD_BOOTEFI_HELLO

I would prefer a normal if and let the linker sort it out.

Here I focused on simply unfolding("moving") the function, keeping the code as same as possible. Any how the code is in a tentative form at this point and will be reshaped in later patches, using IS_ENABLED(). Please look at the final code after applying the whole series.

I also agree with this approach.

-- Tom