On Tue, Oct 24, 2023 at 1:30 PM Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 10/24/23 02:21, seanedm...@linux.microsoft.com wrote:
> > From: Sean Edmond <seanedm...@microsoft.com>
> >
> > Allow dhcp server pass pxe config file full path by using option 209
> >
> > Signed-off-by: Sean Edmond <seanedm...@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))
>
