Re: [PATCH 09/14] pxe: Refactor to reduce the size of label_boot()

2023-12-09 Thread Tom Rini
On Sun, Dec 03, 2023 at 05:31:33PM -0700, Simon Glass wrote:

> This function is far too long and complicated. Split out the part
> which actually calls the boot commands into a separate function.
> 
> Change a strncpy() to strlcpy() to keep checkpatch happy.
> 
> No functional change is intended.
> 
> Signed-off-by: Simon Glass 

Please split strncpy -> strlcpy in to its own patch so it can be
reviewed easily, thanks.

-- 
Tom


signature.asc
Description: PGP signature


[PATCH 09/14] pxe: Refactor to reduce the size of label_boot()

2023-12-03 Thread Simon Glass
This function is far too long and complicated. Split out the part
which actually calls the boot commands into a separate function.

Change a strncpy() to strlcpy() to keep checkpatch happy.

No functional change is intended.

Signed-off-by: Simon Glass 
---

 boot/pxe_utils.c | 301 +--
 1 file changed, 163 insertions(+), 138 deletions(-)

diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index a92bb896c63e..6fbccadd99e6 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -470,159 +470,47 @@ skip_overlay:
 #endif
 
 /**
- * label_boot() - Boot according to the contents of a pxe_label
+ * label_run_boot() - Run the correct boot procedure
  *
- * If we can't boot for any reason, we return.  A successful boot never
- * returns.
+ * fdt usage is optional:
+ * It handles the following scenarios.
  *
- * The kernel will be stored in the location given by the 'kernel_addr_r'
- * environment variable.
+ * Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is
+ * defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to
+ * bootm, and adjust argc appropriately.
  *
- * If the label specifies an initrd file, it will be stored in the location
- * given by the 'ramdisk_addr_r' environment variable.
+ * If retrieve fails and no exact fdt blob is specified in pxe file with
+ * "fdt" label, try Scenario 2.
  *
- * If the label specifies an 'append' line, its contents will overwrite that
- * of the 'bootargs' environment variable.
+ * Scenario 2: If there is an fdt_addr specified, pass it along to
+ * bootm, and adjust argc appropriately.
+ *
+ * Scenario 3: If there is an fdtcontroladdr specified, pass it along to
+ * bootm, and adjust argc appropriately, unless the image type is fitImage.
+ *
+ * Scenario 4: fdt blob is not available.
  *
  * @ctx: PXE context
  * @label: Label to process
+ * @kernel_addr: string containing the kernel address / config
+ * @initrd_str: string containing the initrd address / size
+ * @initrd_addr_str: initrd address, or NULL if none
+ * @initrd_filesize: initrd size in bytes; only valid if initrd_addr_str is not
+ * NULL
  * Returns does not return on success, otherwise returns 0 if a localboot
  * label was processed, or 1 on error
  */
-static int label_boot(struct pxe_context *ctx, struct pxe_label *label)
+static int label_run_boot(struct pxe_context *ctx, struct pxe_label *label,
+ char *kernel_addr, char *initrd_str,
+ char *initrd_addr_str, char *initrd_filesize)
 {
char *bootm_argv[] = { "bootm", NULL, NULL, NULL, NULL };
char *zboot_argv[] = { "zboot", NULL, "0", NULL, NULL };
-   char *kernel_addr = NULL;
-   char *initrd_addr_str = NULL;
-   char initrd_filesize[10];
-   char initrd_str[28];
-   char mac_str[29] = "";
-   char ip_str[68] = "";
-   char *fit_addr = NULL;
+   ulong kernel_addr_r;
int bootm_argc = 2;
int zboot_argc = 3;
-   int len = 0;
-   ulong kernel_addr_r;
void *buf;
 
-   label_print(label);
-
-   label->attempted = 1;
-
-   if (label->localboot) {
-   if (label->localboot_val >= 0)
-   label_localboot(label);
-   return 0;
-   }
-
-   if (!label->kernel) {
-   printf("No kernel given, skipping %s\n",
-  label->name);
-   return 1;
-   }
-
-   if (get_relfile_envaddr(ctx, label->kernel, "kernel_addr_r",
-   NULL) < 0) {
-   printf("Skipping %s for failure retrieving kernel\n",
-  label->name);
-   return 1;
-   }
-
-   kernel_addr = env_get("kernel_addr_r");
-   /* for FIT, append the configuration identifier */
-   if (label->config) {
-   int len = strlen(kernel_addr) + strlen(label->config) + 1;
-
-   fit_addr = malloc(len);
-   if (!fit_addr) {
-   printf("malloc fail (FIT address)\n");
-   return 1;
-   }
-   snprintf(fit_addr, len, "%s%s", kernel_addr, label->config);
-   kernel_addr = fit_addr;
-   }
-
-   /* For FIT, the label can be identical to kernel one */
-   if (label->initrd && !strcmp(label->kernel_label, label->initrd)) {
-   initrd_addr_str =  kernel_addr;
-   } else if (label->initrd) {
-   ulong size;
-   if (get_relfile_envaddr(ctx, label->initrd, "ramdisk_addr_r",
-   ) < 0) {
-   printf("Skipping %s for failure retrieving initrd\n",
-  label->name);
-   goto cleanup;
-   }
-   strcpy(initrd_filesize, simple_xtoa(size));
-   initrd_addr_str = env_get("ramdisk_addr_r");
-   size = snprintf(initrd_str, sizeof(initrd_str),