Re: [PATCH v2 1/3] net: Get pxe config file from dhcp option 209

On Tue, Oct 24, 2023 at 1:30 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/24/23 02:21, seanedmond@linux.microsoft.com wrote:

...

From: Sean Edmond seanedmond@microsoft.com

Allow dhcp server pass pxe config file full path by using option 209

Signed-off-by: Sean Edmond seanedmond@microsoft.com

cmd/Kconfig | 4 ++++ cmd/pxe.c | 10 ++++++++++ net/bootp.c | 21 +++++++++++++++++++++ 3 files changed, 35 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig index 5bc0a92d57..adbb1a6187 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH default 0x15 if ARM default 0x0 if X86

+config BOOTP_PXE_DHCP_OPTION

• bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
  

• depends on BOOTP_PXE
  

Why should this be disabled by default?

Do we really want a separate config variable?

I had similar thoughts, if it's a defined DHCP option I suspect most users will want to use that for PXE, if it's not defined we'll have the same process as we have now for the use to fall back to.

...

• config BOOTP_VCI_STRING string depends on CMD_BOOTP

diff --git a/cmd/pxe.c b/cmd/pxe.c index 704589702f..9404f44518 100644 --- a/cmd/pxe.c +++ b/cmd/pxe.c @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_a int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);

  free(pxelinux_configfile);

• /* set to NULL to avoid double-free if DHCP is tried again */
  

• pxelinux_configfile = NULL;
  
  return ret;
  

  }

@@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6) env_get("bootfile"), use_ipv6)) return -ENOMEM;

• if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
  

•     pxelinux_configfile && !use_ipv6) {
  

•         if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
  

•                 goto done;
  

•         goto error_exit;
  

• }
  

• if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
      pxelinux_configfile && use_ipv6) {
          if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
  

diff --git a/net/bootp.c b/net/bootp.c index 7b0f45e18a..6800290963 100644 --- a/net/bootp.c +++ b/net/bootp.c @@ -26,6 +26,7 @@ #ifdef CONFIG_BOOTP_RANDOM_DELAY #include "net_rand.h" #endif +#include <malloc.h>

#define BOOTP_VENDOR_MAGIC 0x63825363 /* RFC1048 Magic Cookie */

@@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int message_type, struct in_addr server_ip, *e++ = 42; *cnt += 1; #endif

• if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
  

•         *e++ = 209;     /* PXELINUX Config File */
  

•         *cnt += 1;
  

• }
  /* no options, so back up to avoid sending an empty request list */
  if (*cnt == 0)
          e -= 2;
  

@@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt, uchar *end) net_boot_file_name[size] = 0; } break;

•         case 209:       /* PXELINUX Config File */
  

This 209 appears in multiple places. Please, define a constant, e.g. DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please, document the constant referring to RFC 5071.

...

•                 if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
  

•                         /* In case it has already been allocated when get DHCP Offer packet,
  

•                          * free first to avoid memory leak.
  

•                          */
  

•                         if (pxelinux_configfile)
  

•                                 free(pxelinux_configfile);
  

•                         pxelinux_configfile = (char *)malloc((oplen + 1) * sizeof(char));
  

•                         if (pxelinux_configfile)
  

•                                 strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
  

•                         else
  

•                                 printf("Error: Failed to allocate pxelinux_configfile\n");
  

We do the same in dhcp6_parse_options(). Please, factor out a common function to avoid code duplication.

Best regards

Heinrich

...

•                 }
  

•                 break;
          default:
  

  #if defined(CONFIG_BOOTP_VENDOREX) if (dhcp_vendorex_proc(popt))