[v2,06/11] libnet: Add functions for downloading and parsing pxelinux.cfg files

Booting a kernel via pxelinux.cfg files is common on x86 and also with
ppc64 bootloaders like petitboot, so it would be nice to support this
in SLOF, too. This patch adds functions for downloading and parsing
such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
https://www.syslinux.org/wiki/index.php?title=PXELINUX

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 lib/libnet/Makefile   |   2 +-
 lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/libnet/pxelinux.h |  33 ++++++++
 3 files changed, 243 insertions(+), 1 deletion(-)
 create mode 100644 lib/libnet/pxelinux.c
 create mode 100644 lib/libnet/pxelinux.h

On 19/5/18 1:45 am, Thomas Huth wrote:
> Booting a kernel via pxelinux.cfg files is common on x86 and also with
> ppc64 bootloaders like petitboot, so it would be nice to support this
> in SLOF, too. This patch adds functions for downloading and parsing
> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
> https://www.syslinux.org/wiki/index.php?title=PXELINUX
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/libnet/Makefile   |   2 +-
>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libnet/pxelinux.h |  33 ++++++++
>  3 files changed, 243 insertions(+), 1 deletion(-)
>  create mode 100644 lib/libnet/pxelinux.c
>  create mode 100644 lib/libnet/pxelinux.h
> 
> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
> index dfefea9..a2a6570 100644
> --- a/lib/libnet/Makefile
> +++ b/lib/libnet/Makefile
> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>  
>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>  
>  OBJS = $(SRCS:%.c=%.o)
>  
> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
> new file mode 100644
> index 0000000..0c0b42e
> --- /dev/null
> +++ b/lib/libnet/pxelinux.c
> @@ -0,0 +1,209 @@
> +/*****************************************************************************
> + * pxelinux.cfg-style config file support.
> + *
> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
> + * about the pxelinux config file layout.
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program and the accompanying materials are made available under the
> + * terms of the BSD License which accompanies this distribution, and is
> + * available at http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + *     Thomas Huth, Red Hat Inc. - initial implementation
> + *****************************************************************************/
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include "tftp.h"
> +#include "pxelinux.h"
> +
> +/**
> + * Call tftp() and report errors (excet "file-not-found" errors)
> + */
> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
> +{
> +	tftp_err_t tftp_err;
> +	int rc;
> +
> +	rc = tftp(fnip, buffer, len, &tftp_err);
> +
> +	if (rc > 0) {
> +		printf("\r  TFTP: Received %s (%d bytes)\n",
> +		       fnip->filename, rc);
> +	} else if (rc == -3) {
> +		/* Ignore file-not-found (since we are probing the files)
> +		 * and simply erase the "Receiving data:  0 KBytes" string */
> +		printf("\r                           \r");
> +	} else {
> +		const char *errstr = NULL;
> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
> +		if (errstr)
> +			printf("\r  TFTP error: %s\n", errstr);
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * Try to load a pxelinux.cfg file by probing the possible file names.
> + */
> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                             char *cfgbuf, int cfgbufsize)
> +{
> +	int rc, idx;
> +	char basedir[sizeof(fn_ip->filename) - 40];

Why 40? +40 would make sense to accommodate UUID but -40 I do not understand.

> +	char *slash;
> +
> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */


This should go to pxelinux_load_parse_cfg().

> +
> +	/* Did we get a usable base directory via DHCP? */
> +	slash = strrchr(fn_ip->filename, '/');
> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
> +		slash[1] = 0;
> +		strcpy(basedir, fn_ip->filename);
> +	} else {
> +		strcpy(basedir, "pxelinux.cfg/");


If a long directory name was returned via DHCP, you'll end up here silently
using 'pxelinux.cfg/'.


> +	}
> +
> +	printf("Trying pxelinux.cfg files...\n");
> +
> +	/* Try to load config file with name based on the VM UUID */
> +	if (uuid) {
> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
> +			uuid[12], uuid[13], uuid[14], uuid[15]);
> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +		if (rc > 0) {
> +			return rc;
> +		}
> +	}
> +
> +	/* Look for config file with MAC address in its name */
> +	sprintf(fn_ip->filename, "%s%02x-%02x-%02x-%02x-%02x-%02x", basedir,
> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +	if (rc > 0) {
> +		return rc;
> +	}
> +
> +	/* Look for config file with IP address in its name */
> +	if (fn_ip->ip_version == 4) {
> +		for (idx = 0; idx <= 7; idx++) {
> +			sprintf(fn_ip->filename, "%s%02X%02X%02X%02X", basedir,
> +				(fn_ip->own_ip >> 24) & 0xff,
> +				(fn_ip->own_ip >> 16) & 0xff,
> +				(fn_ip->own_ip >> 8) & 0xff,
> +				fn_ip->own_ip & 0xff);


You do not need to sprintf every iteration, you can do it once before for()
and then cut bytes from the end in this loop. Or it may change somewhere in
lib/libnet/tftp.c? I could not spot where.


> +			fn_ip->filename[strlen(fn_ip->filename) - idx] = 0;
> +			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +			if (rc > 0) {
> +				return rc;
> +			}
> +		}
> +	}
> +
> +	/* Try "default" config file */
> +	sprintf(fn_ip->filename, "%sdefault", basedir);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +
> +	return rc;
> +}
> +
> +/**
> + * Parse a pxelinux-style configuration file.
> + * @param cfg          Pointer to the buffer with contents of the config file
> + * @param cfgsize      Size of the cfg buffer
> + * @param entries      Pointer to array where the results should be put into
> + * @param max_entries  Number of available slots in the entries array
> + * @param def_ent      Used to return the index of the default entry
> + * @return             Number of valid entries
> + */
> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,


Are you planning on returning errors from this? If not, make "num_entries"
unsigned and then you can drop " > 0" in "num_entries > 0" below.


> +                       int max_entries, int *def_ent)
> +{
> +	int num_entries = 0;
> +	char *ptr = cfg, *nextptr, *eol, *arg;
> +	char *defaultlabel = NULL;
> +
> +	*def_ent = 0;
> +
> +	while (ptr < cfg + cfgsize && num_entries < max_entries) {
> +		eol = strchr(ptr, '\n');
> +		if (!eol) {
> +			eol = cfg + cfgsize;
> +		}
> +		nextptr = eol + 1;
> +		do {
> +			*eol-- = '\0';	/* Remove spaces, tabs and returns */
> +		} while (eol >= ptr &&
> +		         (*eol == '\r' || *eol == ' ' || *eol == '\t'));
> +		while (*ptr == ' ' || *ptr == '\t') {
> +			ptr++;
> +		}
> +		if (*ptr == 0 || *ptr == '#') {
> +			goto nextline;	/* Ignore comments and empty lines */
> +		}
> +		arg = strchr(ptr, ' ');	/* Search space between cmnd and arg */
> +		if (!arg) {
> +			arg = strchr(ptr, '\t');
> +		}
> +		if (!arg) {
> +			printf("Failed to parse this line:\n %s\n", ptr);
> +			goto nextline;
> +		}
> +		*arg++ = 0;
> +		while (*arg == ' ' || *arg == '\t') {
> +			arg++;
> +		}
> +		if (!strcasecmp("default", ptr)) {
> +			defaultlabel = arg;
> +		} else if (!strcasecmp("label", ptr)) {
> +			entries[num_entries].label = arg;
> +			if (defaultlabel && !strcmp(arg, defaultlabel)) {
> +				*def_ent = num_entries;
> +			}
> +			num_entries++;
> +		} else if (!strcasecmp("kernel", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].kernel = arg;
> +		} else if (!strcasecmp("initrd", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].initrd = arg;
> +		} else if (!strcasecmp("append", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].append = arg;
> +		} else {
> +			printf("Command '%s' is not supported.\n", ptr);
> +		}
> +nextline:
> +		ptr = nextptr;
> +	}
> +
> +	return num_entries;
> +}
> +
> +/**
> + * Try to load and parse a pxelinux-style configuration file.
> + * @param fn_ip        must contain server and client IP information
> + * @param mac          MAC address which should be used for probing
> + * @param uuid         UUID which should be used for probing (can be NULL)
> + * @param cfgbuf       Pointer to the buffer where config file should be loaded
> + * @param cfgsize      Size of the cfgbuf buffer
> + * @param entries      Pointer to array where the results should be put into
> + * @param max_entries  Number of available slots in the entries array
> + * @param def_ent      Used to return the index of the default entry
> + * @return             Number of valid entries
> + */
> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
> +                            int max_entries, int *def_ent)
> +{
> +	int rc;

"static char cfgbuf[CFG_BUF_SIZE];" should be here (without "static" I
suspect).

> +
> +	rc = pxelinux_load_cfg(fn_ip, mac, uuid, cfgbuf, cfgsize);
> +	if (rc < 0)
> +		return rc;


And this:

	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */


to make it clear what code actually cares about this nul termination.


> +
> +	return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent);
> +}
> diff --git a/lib/libnet/pxelinux.h b/lib/libnet/pxelinux.h
> new file mode 100644
> index 0000000..9fd1b2f
> --- /dev/null
> +++ b/lib/libnet/pxelinux.h
> @@ -0,0 +1,33 @@
> +/*****************************************************************************
> + * Definitions for pxelinux-style config file support
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program and the accompanying materials
> + * are made available under the terms of the BSD License
> + * which accompanies this distribution, and is available at
> + * http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + *     Thomas Huth, Red Hat Inc. - initial implementation
> + *****************************************************************************/
> +
> +#ifndef LIBNET_PXELINUX_H
> +#define LIBNET_PXELINUX_H
> +
> +/* This structure holds the data from one pxelinux.cfg file entry */
> +struct lkia {

The name "lkia" is funny, does some other pxelinux implementation use it?
:) "pxe_cfg_entry" would make more sense imho.


> +    const char *label;
> +    const char *kernel;
> +    const char *initrd;
> +    const char *append;
> +};
> +
> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,
> +                       int max_entries, int *def_ent);
> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
> +                            int max_entries, int *def_ent);
> +
> +
> +#endif
>

On 24.05.2018 11:02, Alexey Kardashevskiy wrote:
> On 19/5/18 1:45 am, Thomas Huth wrote:
>> Booting a kernel via pxelinux.cfg files is common on x86 and also with
>> ppc64 bootloaders like petitboot, so it would be nice to support this
>> in SLOF, too. This patch adds functions for downloading and parsing
>> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
>> https://www.syslinux.org/wiki/index.php?title=PXELINUX
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/libnet/Makefile   |   2 +-
>>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/libnet/pxelinux.h |  33 ++++++++
>>  3 files changed, 243 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/libnet/pxelinux.c
>>  create mode 100644 lib/libnet/pxelinux.h
>>
>> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
>> index dfefea9..a2a6570 100644
>> --- a/lib/libnet/Makefile
>> +++ b/lib/libnet/Makefile
>> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>>  
>>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
>> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
>> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>>  
>>  OBJS = $(SRCS:%.c=%.o)
>>  
>> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
>> new file mode 100644
>> index 0000000..0c0b42e
>> --- /dev/null
>> +++ b/lib/libnet/pxelinux.c
>> @@ -0,0 +1,209 @@
>> +/*****************************************************************************
>> + * pxelinux.cfg-style config file support.
>> + *
>> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
>> + * about the pxelinux config file layout.
>> + *
>> + * Copyright 2018 Red Hat, Inc.
>> + *
>> + * This program and the accompanying materials are made available under the
>> + * terms of the BSD License which accompanies this distribution, and is
>> + * available at http://www.opensource.org/licenses/bsd-license.php
>> + *
>> + * Contributors:
>> + *     Thomas Huth, Red Hat Inc. - initial implementation
>> + *****************************************************************************/
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include "tftp.h"
>> +#include "pxelinux.h"
>> +
>> +/**
>> + * Call tftp() and report errors (excet "file-not-found" errors)
>> + */
>> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
>> +{
>> +	tftp_err_t tftp_err;
>> +	int rc;
>> +
>> +	rc = tftp(fnip, buffer, len, &tftp_err);
>> +
>> +	if (rc > 0) {
>> +		printf("\r  TFTP: Received %s (%d bytes)\n",
>> +		       fnip->filename, rc);
>> +	} else if (rc == -3) {
>> +		/* Ignore file-not-found (since we are probing the files)
>> +		 * and simply erase the "Receiving data:  0 KBytes" string */
>> +		printf("\r                           \r");
>> +	} else {
>> +		const char *errstr = NULL;
>> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
>> +		if (errstr)
>> +			printf("\r  TFTP error: %s\n", errstr);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>> + * Try to load a pxelinux.cfg file by probing the possible file names.
>> + */
>> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
>> +                             char *cfgbuf, int cfgbufsize)
>> +{
>> +	int rc, idx;
>> +	char basedir[sizeof(fn_ip->filename) - 40];
> 
> Why 40? +40 would make sense to accommodate UUID but -40 I do not understand.

The complete filename (e.g. based on the UUID) will be stored in
fn_ip->filename, not in basedir (which is just the directory name
without the file name), so the length of the filename must bigger than
the length of the basedir, i.e.

     sizeof(filename) == sizeof(basedir) + sizeof(uuid)

==>  sizeof(basedir) == sizeof(filename) - sizeof(uuid)

But since the "- 40" is obviously not immediately clear, I'll add a
comment to the code here in the next version.

>> +	char *slash;
>> +
>> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
> 
> This should go to pxelinux_load_parse_cfg().

I don't mind ... can move it.

>> +
>> +	/* Did we get a usable base directory via DHCP? */
>> +	slash = strrchr(fn_ip->filename, '/');
>> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
>> +		slash[1] = 0;
>> +		strcpy(basedir, fn_ip->filename);
>> +	} else {
>> +		strcpy(basedir, "pxelinux.cfg/");
> 
> If a long directory name was returned via DHCP, you'll end up here silently
> using 'pxelinux.cfg/'.

So you'd expect an error message in this case? ... can do that.

>> +	}
>> +
>> +	printf("Trying pxelinux.cfg files...\n");
>> +
>> +	/* Try to load config file with name based on the VM UUID */
>> +	if (uuid) {
>> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
>> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
>> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
>> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
>> +			uuid[12], uuid[13], uuid[14], uuid[15]);
>> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>> +		if (rc > 0) {
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	/* Look for config file with MAC address in its name */
>> +	sprintf(fn_ip->filename, "%s%02x-%02x-%02x-%02x-%02x-%02x", basedir,
>> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
>> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>> +	if (rc > 0) {
>> +		return rc;
>> +	}
>> +
>> +	/* Look for config file with IP address in its name */
>> +	if (fn_ip->ip_version == 4) {
>> +		for (idx = 0; idx <= 7; idx++) {
>> +			sprintf(fn_ip->filename, "%s%02X%02X%02X%02X", basedir,
>> +				(fn_ip->own_ip >> 24) & 0xff,
>> +				(fn_ip->own_ip >> 16) & 0xff,
>> +				(fn_ip->own_ip >> 8) & 0xff,
>> +				fn_ip->own_ip & 0xff);
> 
> You do not need to sprintf every iteration, you can do it once before for()
> and then cut bytes from the end in this loop. Or it may change somewhere in
> lib/libnet/tftp.c? I could not spot where.

I think tftp.c does not change the file name ... so I'll give it a try
to move the sprintf in front of the loop.

>> +			fn_ip->filename[strlen(fn_ip->filename) - idx] = 0;
>> +			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>> +			if (rc > 0) {
>> +				return rc;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Try "default" config file */
>> +	sprintf(fn_ip->filename, "%sdefault", basedir);
>> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>> + * Parse a pxelinux-style configuration file.
>> + * @param cfg          Pointer to the buffer with contents of the config file
>> + * @param cfgsize      Size of the cfg buffer
>> + * @param entries      Pointer to array where the results should be put into
>> + * @param max_entries  Number of available slots in the entries array
>> + * @param def_ent      Used to return the index of the default entry
>> + * @return             Number of valid entries
>> + */
>> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,
> 
> Are you planning on returning errors from this? If not, make "num_entries"
> unsigned and then you can drop " > 0" in "num_entries > 0" below.

No, I did not plan to use negative num_entries values for errors, so I
can do that change in the next version of the patch.

>> +                       int max_entries, int *def_ent)
>> +{
>> +	int num_entries = 0;
>> +	char *ptr = cfg, *nextptr, *eol, *arg;
>> +	char *defaultlabel = NULL;
>> +
>> +	*def_ent = 0;
>> +
>> +	while (ptr < cfg + cfgsize && num_entries < max_entries) {
>> +		eol = strchr(ptr, '\n');
>> +		if (!eol) {
>> +			eol = cfg + cfgsize;
>> +		}
>> +		nextptr = eol + 1;
>> +		do {
>> +			*eol-- = '\0';	/* Remove spaces, tabs and returns */
>> +		} while (eol >= ptr &&
>> +		         (*eol == '\r' || *eol == ' ' || *eol == '\t'));
>> +		while (*ptr == ' ' || *ptr == '\t') {
>> +			ptr++;
>> +		}
>> +		if (*ptr == 0 || *ptr == '#') {
>> +			goto nextline;	/* Ignore comments and empty lines */
>> +		}
>> +		arg = strchr(ptr, ' ');	/* Search space between cmnd and arg */
>> +		if (!arg) {
>> +			arg = strchr(ptr, '\t');
>> +		}
>> +		if (!arg) {
>> +			printf("Failed to parse this line:\n %s\n", ptr);
>> +			goto nextline;
>> +		}
>> +		*arg++ = 0;
>> +		while (*arg == ' ' || *arg == '\t') {
>> +			arg++;
>> +		}
>> +		if (!strcasecmp("default", ptr)) {
>> +			defaultlabel = arg;
>> +		} else if (!strcasecmp("label", ptr)) {
>> +			entries[num_entries].label = arg;
>> +			if (defaultlabel && !strcmp(arg, defaultlabel)) {
>> +				*def_ent = num_entries;
>> +			}
>> +			num_entries++;
>> +		} else if (!strcasecmp("kernel", ptr) && num_entries > 0) {
>> +			entries[num_entries - 1].kernel = arg;
>> +		} else if (!strcasecmp("initrd", ptr) && num_entries > 0) {
>> +			entries[num_entries - 1].initrd = arg;
>> +		} else if (!strcasecmp("append", ptr) && num_entries > 0) {
>> +			entries[num_entries - 1].append = arg;
>> +		} else {
>> +			printf("Command '%s' is not supported.\n", ptr);
>> +		}
>> +nextline:
>> +		ptr = nextptr;
>> +	}
>> +
>> +	return num_entries;
>> +}
>> +
>> +/**
>> + * Try to load and parse a pxelinux-style configuration file.
>> + * @param fn_ip        must contain server and client IP information
>> + * @param mac          MAC address which should be used for probing
>> + * @param uuid         UUID which should be used for probing (can be NULL)
>> + * @param cfgbuf       Pointer to the buffer where config file should be loaded
>> + * @param cfgsize      Size of the cfgbuf buffer
>> + * @param entries      Pointer to array where the results should be put into
>> + * @param max_entries  Number of available slots in the entries array
>> + * @param def_ent      Used to return the index of the default entry
>> + * @return             Number of valid entries
>> + */
>> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
>> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
>> +                            int max_entries, int *def_ent)
>> +{
>> +	int rc;
> 
> "static char cfgbuf[CFG_BUF_SIZE];" should be here (without "static" I
> suspect).

As mentioned in another mail, I'd like to keep the buffer allocation in
the caller, to avoid that I have to strdup() the lkia strings in the
pxelinux_parse_cfg() function.

>> +
>> +	rc = pxelinux_load_cfg(fn_ip, mac, uuid, cfgbuf, cfgsize);
>> +	if (rc < 0)
>> +		return rc;
> 
> And this:
> 
> 	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
> 
> to make it clear what code actually cares about this nul termination.

Ok.

>> +
>> +	return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent);
>> +}
>> diff --git a/lib/libnet/pxelinux.h b/lib/libnet/pxelinux.h
>> new file mode 100644
>> index 0000000..9fd1b2f
>> --- /dev/null
>> +++ b/lib/libnet/pxelinux.h
>> @@ -0,0 +1,33 @@
>> +/*****************************************************************************
>> + * Definitions for pxelinux-style config file support
>> + *
>> + * Copyright 2018 Red Hat, Inc.
>> + *
>> + * This program and the accompanying materials
>> + * are made available under the terms of the BSD License
>> + * which accompanies this distribution, and is available at
>> + * http://www.opensource.org/licenses/bsd-license.php
>> + *
>> + * Contributors:
>> + *     Thomas Huth, Red Hat Inc. - initial implementation
>> + *****************************************************************************/
>> +
>> +#ifndef LIBNET_PXELINUX_H
>> +#define LIBNET_PXELINUX_H
>> +
>> +/* This structure holds the data from one pxelinux.cfg file entry */
>> +struct lkia {
> 
> The name "lkia" is funny, does some other pxelinux implementation use it?
> :) "pxe_cfg_entry" would make more sense imho.

No, there is no other implementation where I've got this from, I just
used it as abbreviation for "label-kernel-initrd-append". But I can
rename that if you prefer pxe_cfg_entry.

Thanks for the reviews!

 Thomas

On 25/5/18 3:49 am, Thomas Huth wrote:
> On 24.05.2018 11:02, Alexey Kardashevskiy wrote:
>> On 19/5/18 1:45 am, Thomas Huth wrote:
>>> Booting a kernel via pxelinux.cfg files is common on x86 and also with
>>> ppc64 bootloaders like petitboot, so it would be nice to support this
>>> in SLOF, too. This patch adds functions for downloading and parsing
>>> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
>>> https://www.syslinux.org/wiki/index.php?title=PXELINUX
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  lib/libnet/Makefile   |   2 +-
>>>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  lib/libnet/pxelinux.h |  33 ++++++++
>>>  3 files changed, 243 insertions(+), 1 deletion(-)
>>>  create mode 100644 lib/libnet/pxelinux.c
>>>  create mode 100644 lib/libnet/pxelinux.h
>>>
>>> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
>>> index dfefea9..a2a6570 100644
>>> --- a/lib/libnet/Makefile
>>> +++ b/lib/libnet/Makefile
>>> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>>>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>>>  
>>>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
>>> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
>>> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>>>  
>>>  OBJS = $(SRCS:%.c=%.o)
>>>  
>>> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
>>> new file mode 100644
>>> index 0000000..0c0b42e
>>> --- /dev/null
>>> +++ b/lib/libnet/pxelinux.c
>>> @@ -0,0 +1,209 @@
>>> +/*****************************************************************************
>>> + * pxelinux.cfg-style config file support.
>>> + *
>>> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
>>> + * about the pxelinux config file layout.
>>> + *
>>> + * Copyright 2018 Red Hat, Inc.
>>> + *
>>> + * This program and the accompanying materials are made available under the
>>> + * terms of the BSD License which accompanies this distribution, and is
>>> + * available at http://www.opensource.org/licenses/bsd-license.php
>>> + *
>>> + * Contributors:
>>> + *     Thomas Huth, Red Hat Inc. - initial implementation
>>> + *****************************************************************************/
>>> +
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include "tftp.h"
>>> +#include "pxelinux.h"
>>> +
>>> +/**
>>> + * Call tftp() and report errors (excet "file-not-found" errors)
>>> + */
>>> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
>>> +{
>>> +	tftp_err_t tftp_err;
>>> +	int rc;
>>> +
>>> +	rc = tftp(fnip, buffer, len, &tftp_err);
>>> +
>>> +	if (rc > 0) {
>>> +		printf("\r  TFTP: Received %s (%d bytes)\n",
>>> +		       fnip->filename, rc);
>>> +	} else if (rc == -3) {
>>> +		/* Ignore file-not-found (since we are probing the files)
>>> +		 * and simply erase the "Receiving data:  0 KBytes" string */
>>> +		printf("\r                           \r");
>>> +	} else {
>>> +		const char *errstr = NULL;
>>> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
>>> +		if (errstr)
>>> +			printf("\r  TFTP error: %s\n", errstr);
>>> +	}
>>> +
>>> +	return rc;
>>> +}
>>> +
>>> +/**
>>> + * Try to load a pxelinux.cfg file by probing the possible file names.
>>> + */
>>> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
>>> +                             char *cfgbuf, int cfgbufsize)
>>> +{
>>> +	int rc, idx;
>>> +	char basedir[sizeof(fn_ip->filename) - 40];
>>
>> Why 40? +40 would make sense to accommodate UUID but -40 I do not understand.
> 
> The complete filename (e.g. based on the UUID) will be stored in
> fn_ip->filename, not in basedir (which is just the directory name
> without the file name), so the length of the filename must bigger than
> the length of the basedir, i.e.
> 
>      sizeof(filename) == sizeof(basedir) + sizeof(uuid)
> 
> ==>  sizeof(basedir) == sizeof(filename) - sizeof(uuid)
> 
> But since the "- 40" is obviously not immediately clear, I'll add a
> comment to the code here in the next version.


Right. My bad. I typed this before I realized that fn_ip->filename contains
useful information when we just enter the function but later used as a
buffer. I'd rather expect pxelinux_load_cfg() to create a copy of fn_ip and
use that. Or filename_ip_t::filename could be a pointer, not an array, just
like new pl_cfgfile. May be a cleanup for another day though.

>>> +	char *slash;
>>> +
>>> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
>>
>> This should go to pxelinux_load_parse_cfg().
> 
> I don't mind ... can move it.
> 
>>> +
>>> +	/* Did we get a usable base directory via DHCP? */
>>> +	slash = strrchr(fn_ip->filename, '/');
>>> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
>>> +		slash[1] = 0;
>>> +		strcpy(basedir, fn_ip->filename);
>>> +	} else {
>>> +		strcpy(basedir, "pxelinux.cfg/");


btw this basedir does not seem necessary, you could just leave base
directory in fn_ip->filename or reset it to "pxelinux.cfg/", make "slash"
point to the end of that and do all these sprintf's below directly to "slash".


>> If a long directory name was returned via DHCP, you'll end up here silently
>> using 'pxelinux.cfg/'.
> 
> So you'd expect an error message in this case? ... can do that.


Who would not expect that? :)

I would probably also use the default only for an empty fn_ip->filename but
I do not know what other pxelinux loaders normally do, i.e what is the
usual expectation.


>>> +	}
>>> +
>>> +	printf("Trying pxelinux.cfg files...\n");
>>> +
>>> +	/* Try to load config file with name based on the VM UUID */
>>> +	if (uuid) {
>>> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
>>> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
>>> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
>>> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
>>> +			uuid[12], uuid[13], uuid[14], uuid[15]);
>>> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>>> +		if (rc > 0) {
>>> +			return rc;
>>> +		}
>>> +	}
>>> +
>>> +	/* Look for config file with MAC address in its name */
>>> +	sprintf(fn_ip->filename, "%s%02x-%02x-%02x-%02x-%02x-%02x", basedir,
>>> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
>>> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>>> +	if (rc > 0) {
>>> +		return rc;
>>> +	}
>>> +
>>> +	/* Look for config file with IP address in its name */
>>> +	if (fn_ip->ip_version == 4) {
>>> +		for (idx = 0; idx <= 7; idx++) {
>>> +			sprintf(fn_ip->filename, "%s%02X%02X%02X%02X", basedir,
>>> +				(fn_ip->own_ip >> 24) & 0xff,
>>> +				(fn_ip->own_ip >> 16) & 0xff,
>>> +				(fn_ip->own_ip >> 8) & 0xff,
>>> +				fn_ip->own_ip & 0xff);
>>
>> You do not need to sprintf every iteration, you can do it once before for()
>> and then cut bytes from the end in this loop. Or it may change somewhere in
>> lib/libnet/tftp.c? I could not spot where.
> 
> I think tftp.c does not change the file name ... so I'll give it a try
> to move the sprintf in front of the loop.
> 
>>> +			fn_ip->filename[strlen(fn_ip->filename) - idx] = 0;
>>> +			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>>> +			if (rc > 0) {
>>> +				return rc;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	/* Try "default" config file */
>>> +	sprintf(fn_ip->filename, "%sdefault", basedir);
>>> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>>> +
>>> +	return rc;
>>> +}
>>> +
>>> +/**
>>> + * Parse a pxelinux-style configuration file.
>>> + * @param cfg          Pointer to the buffer with contents of the config file
>>> + * @param cfgsize      Size of the cfg buffer
>>> + * @param entries      Pointer to array where the results should be put into
>>> + * @param max_entries  Number of available slots in the entries array
>>> + * @param def_ent      Used to return the index of the default entry
>>> + * @return             Number of valid entries
>>> + */
>>> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,
>>
>> Are you planning on returning errors from this? If not, make "num_entries"
>> unsigned and then you can drop " > 0" in "num_entries > 0" below.
> 
> No, I did not plan to use negative num_entries values for errors, so I
> can do that change in the next version of the patch.
> 
>>> +                       int max_entries, int *def_ent)
>>> +{
>>> +	int num_entries = 0;
>>> +	char *ptr = cfg, *nextptr, *eol, *arg;
>>> +	char *defaultlabel = NULL;
>>> +
>>> +	*def_ent = 0;
>>> +
>>> +	while (ptr < cfg + cfgsize && num_entries < max_entries) {
>>> +		eol = strchr(ptr, '\n');
>>> +		if (!eol) {
>>> +			eol = cfg + cfgsize;
>>> +		}
>>> +		nextptr = eol + 1;
>>> +		do {
>>> +			*eol-- = '\0';	/* Remove spaces, tabs and returns */
>>> +		} while (eol >= ptr &&
>>> +		         (*eol == '\r' || *eol == ' ' || *eol == '\t'));
>>> +		while (*ptr == ' ' || *ptr == '\t') {
>>> +			ptr++;
>>> +		}
>>> +		if (*ptr == 0 || *ptr == '#') {
>>> +			goto nextline;	/* Ignore comments and empty lines */
>>> +		}
>>> +		arg = strchr(ptr, ' ');	/* Search space between cmnd and arg */
>>> +		if (!arg) {
>>> +			arg = strchr(ptr, '\t');
>>> +		}
>>> +		if (!arg) {
>>> +			printf("Failed to parse this line:\n %s\n", ptr);
>>> +			goto nextline;
>>> +		}
>>> +		*arg++ = 0;
>>> +		while (*arg == ' ' || *arg == '\t') {
>>> +			arg++;
>>> +		}
>>> +		if (!strcasecmp("default", ptr)) {
>>> +			defaultlabel = arg;
>>> +		} else if (!strcasecmp("label", ptr)) {
>>> +			entries[num_entries].label = arg;
>>> +			if (defaultlabel && !strcmp(arg, defaultlabel)) {
>>> +				*def_ent = num_entries;
>>> +			}
>>> +			num_entries++;
>>> +		} else if (!strcasecmp("kernel", ptr) && num_entries > 0) {
>>> +			entries[num_entries - 1].kernel = arg;
>>> +		} else if (!strcasecmp("initrd", ptr) && num_entries > 0) {
>>> +			entries[num_entries - 1].initrd = arg;
>>> +		} else if (!strcasecmp("append", ptr) && num_entries > 0) {
>>> +			entries[num_entries - 1].append = arg;
>>> +		} else {
>>> +			printf("Command '%s' is not supported.\n", ptr);
>>> +		}
>>> +nextline:
>>> +		ptr = nextptr;
>>> +	}
>>> +
>>> +	return num_entries;
>>> +}
>>> +
>>> +/**
>>> + * Try to load and parse a pxelinux-style configuration file.
>>> + * @param fn_ip        must contain server and client IP information
>>> + * @param mac          MAC address which should be used for probing
>>> + * @param uuid         UUID which should be used for probing (can be NULL)
>>> + * @param cfgbuf       Pointer to the buffer where config file should be loaded
>>> + * @param cfgsize      Size of the cfgbuf buffer
>>> + * @param entries      Pointer to array where the results should be put into
>>> + * @param max_entries  Number of available slots in the entries array
>>> + * @param def_ent      Used to return the index of the default entry
>>> + * @return             Number of valid entries
>>> + */
>>> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
>>> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
>>> +                            int max_entries, int *def_ent)
>>> +{
>>> +	int rc;
>>
>> "static char cfgbuf[CFG_BUF_SIZE];" should be here (without "static" I
>> suspect).
> 
> As mentioned in another mail, I'd like to keep the buffer allocation in
> the caller, to avoid that I have to strdup() the lkia strings in the
> pxelinux_parse_cfg() function.
> 
>>> +
>>> +	rc = pxelinux_load_cfg(fn_ip, mac, uuid, cfgbuf, cfgsize);
>>> +	if (rc < 0)
>>> +		return rc;
>>
>> And this:
>>
>> 	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
>>
>> to make it clear what code actually cares about this nul termination.
> 
> Ok.
> 
>>> +
>>> +	return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent);
>>> +}
>>> diff --git a/lib/libnet/pxelinux.h b/lib/libnet/pxelinux.h
>>> new file mode 100644
>>> index 0000000..9fd1b2f
>>> --- /dev/null
>>> +++ b/lib/libnet/pxelinux.h
>>> @@ -0,0 +1,33 @@
>>> +/*****************************************************************************
>>> + * Definitions for pxelinux-style config file support
>>> + *
>>> + * Copyright 2018 Red Hat, Inc.
>>> + *
>>> + * This program and the accompanying materials
>>> + * are made available under the terms of the BSD License
>>> + * which accompanies this distribution, and is available at
>>> + * http://www.opensource.org/licenses/bsd-license.php
>>> + *
>>> + * Contributors:
>>> + *     Thomas Huth, Red Hat Inc. - initial implementation
>>> + *****************************************************************************/
>>> +
>>> +#ifndef LIBNET_PXELINUX_H
>>> +#define LIBNET_PXELINUX_H
>>> +
>>> +/* This structure holds the data from one pxelinux.cfg file entry */
>>> +struct lkia {
>>
>> The name "lkia" is funny, does some other pxelinux implementation use it?
>> :) "pxe_cfg_entry" would make more sense imho.
> 
> No, there is no other implementation where I've got this from, I just
> used it as abbreviation for "label-kernel-initrd-append". But I can
> rename that if you prefer pxe_cfg_entry.
> 
> Thanks for the reviews!

Thanks for the patches :)

On 19/5/18 1:45 am, Thomas Huth wrote:
> Booting a kernel via pxelinux.cfg files is common on x86 and also with
> ppc64 bootloaders like petitboot, so it would be nice to support this
> in SLOF, too. This patch adds functions for downloading and parsing
> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
> https://www.syslinux.org/wiki/index.php?title=PXELINUX
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/libnet/Makefile   |   2 +-
>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libnet/pxelinux.h |  33 ++++++++
>  3 files changed, 243 insertions(+), 1 deletion(-)
>  create mode 100644 lib/libnet/pxelinux.c
>  create mode 100644 lib/libnet/pxelinux.h
> 
> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
> index dfefea9..a2a6570 100644
> --- a/lib/libnet/Makefile
> +++ b/lib/libnet/Makefile
> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>  
>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>  
>  OBJS = $(SRCS:%.c=%.o)
>  
> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
> new file mode 100644
> index 0000000..0c0b42e
> --- /dev/null
> +++ b/lib/libnet/pxelinux.c
> @@ -0,0 +1,209 @@
> +/*****************************************************************************
> + * pxelinux.cfg-style config file support.
> + *
> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
> + * about the pxelinux config file layout.
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program and the accompanying materials are made available under the
> + * terms of the BSD License which accompanies this distribution, and is
> + * available at http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + *     Thomas Huth, Red Hat Inc. - initial implementation
> + *****************************************************************************/
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include "tftp.h"
> +#include "pxelinux.h"
> +
> +/**
> + * Call tftp() and report errors (excet "file-not-found" errors)
> + */
> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
> +{
> +	tftp_err_t tftp_err;
> +	int rc;
> +
> +	rc = tftp(fnip, buffer, len, &tftp_err);
> +
> +	if (rc > 0) {
> +		printf("\r  TFTP: Received %s (%d bytes)\n",
> +		       fnip->filename, rc);
> +	} else if (rc == -3) {
> +		/* Ignore file-not-found (since we are probing the files)
> +		 * and simply erase the "Receiving data:  0 KBytes" string */
> +		printf("\r                           \r");
> +	} else {
> +		const char *errstr = NULL;
> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
> +		if (errstr)
> +			printf("\r  TFTP error: %s\n", errstr);
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * Try to load a pxelinux.cfg file by probing the possible file names.
> + */
> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                             char *cfgbuf, int cfgbufsize)
> +{
> +	int rc, idx;
> +	char basedir[sizeof(fn_ip->filename) - 40];
> +	char *slash;
> +
> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
> +
> +	/* Did we get a usable base directory via DHCP? */
> +	slash = strrchr(fn_ip->filename, '/');
> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
> +		slash[1] = 0;
> +		strcpy(basedir, fn_ip->filename);
> +	} else {
> +		strcpy(basedir, "pxelinux.cfg/");
> +	}
> +
> +	printf("Trying pxelinux.cfg files...\n");
> +
> +	/* Try to load config file with name based on the VM UUID */
> +	if (uuid) {
> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
> +			uuid[12], uuid[13], uuid[14], uuid[15]);
> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +		if (rc > 0) {
> +			return rc;
> +		}
> +	}


This belongs to 11/11. Also, this UUID comes from QEMU's system-id DT
property which is formatted as xxxx-xx-xx-xx-xxxxxx (UUID_FMT in QEMU)
which is exactly the same as above, cannot you just use the plain string
from the property and avoid parsing/sprintf?



> +
> +	/* Look for config file with MAC address in its name */
> +	sprintf(fn_ip->filename, "%s%02x-%02x-%02x-%02x-%02x-%02x", basedir,
> +		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +	if (rc > 0) {
> +		return rc;
> +	}
> +
> +	/* Look for config file with IP address in its name */
> +	if (fn_ip->ip_version == 4) {
> +		for (idx = 0; idx <= 7; idx++) {
> +			sprintf(fn_ip->filename, "%s%02X%02X%02X%02X", basedir,
> +				(fn_ip->own_ip >> 24) & 0xff,
> +				(fn_ip->own_ip >> 16) & 0xff,
> +				(fn_ip->own_ip >> 8) & 0xff,
> +				fn_ip->own_ip & 0xff);
> +			fn_ip->filename[strlen(fn_ip->filename) - idx] = 0;
> +			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +			if (rc > 0) {
> +				return rc;
> +			}
> +		}
> +	}
> +
> +	/* Try "default" config file */
> +	sprintf(fn_ip->filename, "%sdefault", basedir);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +
> +	return rc;
> +}
> +
> +/**
> + * Parse a pxelinux-style configuration file.
> + * @param cfg          Pointer to the buffer with contents of the config file
> + * @param cfgsize      Size of the cfg buffer
> + * @param entries      Pointer to array where the results should be put into
> + * @param max_entries  Number of available slots in the entries array
> + * @param def_ent      Used to return the index of the default entry
> + * @return             Number of valid entries
> + */
> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,
> +                       int max_entries, int *def_ent)
> +{
> +	int num_entries = 0;
> +	char *ptr = cfg, *nextptr, *eol, *arg;
> +	char *defaultlabel = NULL;
> +
> +	*def_ent = 0;
> +
> +	while (ptr < cfg + cfgsize && num_entries < max_entries) {
> +		eol = strchr(ptr, '\n');
> +		if (!eol) {
> +			eol = cfg + cfgsize;
> +		}
> +		nextptr = eol + 1;
> +		do {
> +			*eol-- = '\0';	/* Remove spaces, tabs and returns */
> +		} while (eol >= ptr &&
> +		         (*eol == '\r' || *eol == ' ' || *eol == '\t'));
> +		while (*ptr == ' ' || *ptr == '\t') {
> +			ptr++;
> +		}
> +		if (*ptr == 0 || *ptr == '#') {
> +			goto nextline;	/* Ignore comments and empty lines */
> +		}
> +		arg = strchr(ptr, ' ');	/* Search space between cmnd and arg */
> +		if (!arg) {
> +			arg = strchr(ptr, '\t');
> +		}
> +		if (!arg) {
> +			printf("Failed to parse this line:\n %s\n", ptr);
> +			goto nextline;
> +		}
> +		*arg++ = 0;
> +		while (*arg == ' ' || *arg == '\t') {
> +			arg++;
> +		}
> +		if (!strcasecmp("default", ptr)) {
> +			defaultlabel = arg;
> +		} else if (!strcasecmp("label", ptr)) {
> +			entries[num_entries].label = arg;
> +			if (defaultlabel && !strcmp(arg, defaultlabel)) {
> +				*def_ent = num_entries;
> +			}
> +			num_entries++;
> +		} else if (!strcasecmp("kernel", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].kernel = arg;
> +		} else if (!strcasecmp("initrd", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].initrd = arg;
> +		} else if (!strcasecmp("append", ptr) && num_entries > 0) {
> +			entries[num_entries - 1].append = arg;
> +		} else {
> +			printf("Command '%s' is not supported.\n", ptr);
> +		}
> +nextline:
> +		ptr = nextptr;
> +	}
> +
> +	return num_entries;
> +}
> +
> +/**
> + * Try to load and parse a pxelinux-style configuration file.
> + * @param fn_ip        must contain server and client IP information
> + * @param mac          MAC address which should be used for probing
> + * @param uuid         UUID which should be used for probing (can be NULL)
> + * @param cfgbuf       Pointer to the buffer where config file should be loaded
> + * @param cfgsize      Size of the cfgbuf buffer
> + * @param entries      Pointer to array where the results should be put into
> + * @param max_entries  Number of available slots in the entries array
> + * @param def_ent      Used to return the index of the default entry
> + * @return             Number of valid entries
> + */
> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
> +                            int max_entries, int *def_ent)
> +{
> +	int rc;
> +
> +	rc = pxelinux_load_cfg(fn_ip, mac, uuid, cfgbuf, cfgsize);
> +	if (rc < 0)
> +		return rc;
> +
> +	return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent);
> +}
> diff --git a/lib/libnet/pxelinux.h b/lib/libnet/pxelinux.h
> new file mode 100644
> index 0000000..9fd1b2f
> --- /dev/null
> +++ b/lib/libnet/pxelinux.h
> @@ -0,0 +1,33 @@
> +/*****************************************************************************
> + * Definitions for pxelinux-style config file support
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program and the accompanying materials
> + * are made available under the terms of the BSD License
> + * which accompanies this distribution, and is available at
> + * http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + *     Thomas Huth, Red Hat Inc. - initial implementation
> + *****************************************************************************/
> +
> +#ifndef LIBNET_PXELINUX_H
> +#define LIBNET_PXELINUX_H
> +
> +/* This structure holds the data from one pxelinux.cfg file entry */
> +struct lkia {
> +    const char *label;
> +    const char *kernel;
> +    const char *initrd;
> +    const char *append;
> +};
> +
> +int pxelinux_parse_cfg(char *cfg, int cfgsize, struct lkia *entries,
> +                       int max_entries, int *def_ent);
> +int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                            char *cfgbuf, int cfgsize, struct lkia *entries,
> +                            int max_entries, int *def_ent);
> +
> +
> +#endif
>

On 25.05.2018 08:45, Alexey Kardashevskiy wrote:
> On 19/5/18 1:45 am, Thomas Huth wrote:
>> Booting a kernel via pxelinux.cfg files is common on x86 and also with
>> ppc64 bootloaders like petitboot, so it would be nice to support this
>> in SLOF, too. This patch adds functions for downloading and parsing
>> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
>> https://www.syslinux.org/wiki/index.php?title=PXELINUX
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  lib/libnet/Makefile   |   2 +-
>>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/libnet/pxelinux.h |  33 ++++++++
>>  3 files changed, 243 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/libnet/pxelinux.c
>>  create mode 100644 lib/libnet/pxelinux.h
>>
>> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
>> index dfefea9..a2a6570 100644
>> --- a/lib/libnet/Makefile
>> +++ b/lib/libnet/Makefile
>> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>>  
>>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
>> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
>> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>>  
>>  OBJS = $(SRCS:%.c=%.o)
>>  
>> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
>> new file mode 100644
>> index 0000000..0c0b42e
>> --- /dev/null
>> +++ b/lib/libnet/pxelinux.c
>> @@ -0,0 +1,209 @@
>> +/*****************************************************************************
>> + * pxelinux.cfg-style config file support.
>> + *
>> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
>> + * about the pxelinux config file layout.
>> + *
>> + * Copyright 2018 Red Hat, Inc.
>> + *
>> + * This program and the accompanying materials are made available under the
>> + * terms of the BSD License which accompanies this distribution, and is
>> + * available at http://www.opensource.org/licenses/bsd-license.php
>> + *
>> + * Contributors:
>> + *     Thomas Huth, Red Hat Inc. - initial implementation
>> + *****************************************************************************/
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include "tftp.h"
>> +#include "pxelinux.h"
>> +
>> +/**
>> + * Call tftp() and report errors (excet "file-not-found" errors)
>> + */
>> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
>> +{
>> +	tftp_err_t tftp_err;
>> +	int rc;
>> +
>> +	rc = tftp(fnip, buffer, len, &tftp_err);
>> +
>> +	if (rc > 0) {
>> +		printf("\r  TFTP: Received %s (%d bytes)\n",
>> +		       fnip->filename, rc);
>> +	} else if (rc == -3) {
>> +		/* Ignore file-not-found (since we are probing the files)
>> +		 * and simply erase the "Receiving data:  0 KBytes" string */
>> +		printf("\r                           \r");
>> +	} else {
>> +		const char *errstr = NULL;
>> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
>> +		if (errstr)
>> +			printf("\r  TFTP error: %s\n", errstr);
>> +	}
>> +
>> +	return rc;
>> +}
>> +
>> +/**
>> + * Try to load a pxelinux.cfg file by probing the possible file names.
>> + */
>> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
>> +                             char *cfgbuf, int cfgbufsize)
>> +{
>> +	int rc, idx;
>> +	char basedir[sizeof(fn_ip->filename) - 40];
>> +	char *slash;
>> +
>> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
>> +
>> +	/* Did we get a usable base directory via DHCP? */
>> +	slash = strrchr(fn_ip->filename, '/');
>> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
>> +		slash[1] = 0;
>> +		strcpy(basedir, fn_ip->filename);
>> +	} else {
>> +		strcpy(basedir, "pxelinux.cfg/");
>> +	}
>> +
>> +	printf("Trying pxelinux.cfg files...\n");
>> +
>> +	/* Try to load config file with name based on the VM UUID */
>> +	if (uuid) {
>> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
>> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
>> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
>> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
>> +			uuid[12], uuid[13], uuid[14], uuid[15]);
>> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
>> +		if (rc > 0) {
>> +			return rc;
>> +		}
>> +	}
> 
> 
> This belongs to 11/11. Also, this UUID comes from QEMU's system-id DT
> property which is formatted as xxxx-xx-xx-xx-xxxxxx (UUID_FMT in QEMU)
> which is exactly the same as above, cannot you just use the plain string
> from the property and avoid parsing/sprintf?

Again, the answer is the s390-ccw bios: There you get the UUID in binary
form, not in string form. But ok, I can also convert the binary UUID to
a string in the s390-ccw bios already and always use a string in
pxelinux.c, so I'll try to do that in the next version of the patch.

 Thomas

On 19/5/18 1:45 am, Thomas Huth wrote:
> Booting a kernel via pxelinux.cfg files is common on x86 and also with
> ppc64 bootloaders like petitboot, so it would be nice to support this
> in SLOF, too. This patch adds functions for downloading and parsing
> such pxelinux.cfg files. See this URL for more details on pxelinux.cfg:
> https://www.syslinux.org/wiki/index.php?title=PXELINUX
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/libnet/Makefile   |   2 +-
>  lib/libnet/pxelinux.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libnet/pxelinux.h |  33 ++++++++
>  3 files changed, 243 insertions(+), 1 deletion(-)
>  create mode 100644 lib/libnet/pxelinux.c
>  create mode 100644 lib/libnet/pxelinux.h
> 
> diff --git a/lib/libnet/Makefile b/lib/libnet/Makefile
> index dfefea9..a2a6570 100644
> --- a/lib/libnet/Makefile
> +++ b/lib/libnet/Makefile
> @@ -19,7 +19,7 @@ include $(TOP)/make.rules
>  CFLAGS += -I. -I.. -I../libc/include -I$(TOP)/include $(FLAG)
>  
>  SRCS =	ethernet.c ipv4.c udp.c tcp.c dns.c bootp.c dhcp.c tftp.c \
> -	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c
> +	ipv6.c dhcpv6.c icmpv6.c ndp.c netload.c ping.c args.c pxelinux.c
>  
>  OBJS = $(SRCS:%.c=%.o)
>  
> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
> new file mode 100644
> index 0000000..0c0b42e
> --- /dev/null
> +++ b/lib/libnet/pxelinux.c
> @@ -0,0 +1,209 @@
> +/*****************************************************************************
> + * pxelinux.cfg-style config file support.
> + *
> + * See https://www.syslinux.org/wiki/index.php?title=PXELINUX for information
> + * about the pxelinux config file layout.
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * This program and the accompanying materials are made available under the
> + * terms of the BSD License which accompanies this distribution, and is
> + * available at http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + *     Thomas Huth, Red Hat Inc. - initial implementation
> + *****************************************************************************/
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include "tftp.h"
> +#include "pxelinux.h"
> +
> +/**
> + * Call tftp() and report errors (excet "file-not-found" errors)
> + */
> +static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len)
> +{
> +	tftp_err_t tftp_err;
> +	int rc;
> +
> +	rc = tftp(fnip, buffer, len, &tftp_err);
> +
> +	if (rc > 0) {
> +		printf("\r  TFTP: Received %s (%d bytes)\n",
> +		       fnip->filename, rc);
> +	} else if (rc == -3) {
> +		/* Ignore file-not-found (since we are probing the files)
> +		 * and simply erase the "Receiving data:  0 KBytes" string */
> +		printf("\r                           \r");
> +	} else {
> +		const char *errstr = NULL;
> +		rc = tftp_get_error_info(fnip, &tftp_err, rc, &errstr, NULL);
> +		if (errstr)
> +			printf("\r  TFTP error: %s\n", errstr);
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * Try to load a pxelinux.cfg file by probing the possible file names.
> + */
> +static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, uint8_t *uuid,
> +                             char *cfgbuf, int cfgbufsize)
> +{
> +	int rc, idx;
> +	char basedir[sizeof(fn_ip->filename) - 40];
> +	char *slash;
> +
> +	cfgbuf[cfgbufsize - 1] = 0;  /* Make sure it is NUL-terminated */
> +
> +	/* Did we get a usable base directory via DHCP? */
> +	slash = strrchr(fn_ip->filename, '/');
> +	if (slash && slash - fn_ip->filename < sizeof(basedir) - 1) {
> +		slash[1] = 0;
> +		strcpy(basedir, fn_ip->filename);
> +	} else {
> +		strcpy(basedir, "pxelinux.cfg/");
> +	}
> +
> +	printf("Trying pxelinux.cfg files...\n");
> +
> +	/* Try to load config file with name based on the VM UUID */
> +	if (uuid) {
> +		sprintf(fn_ip->filename, "%s%02x%02x%02x%02x-%02x%02x-"
> +			"%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x", basedir,
> +			uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
> +			uuid[6], uuid[7], uuid[8], uuid[9], uuid[10], uuid[11],
> +			uuid[12], uuid[13], uuid[14], uuid[15]);
> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1);
> +		if (rc > 0) {
> +			return rc;
> +		}
> +	}
> +
> +	/* Look for config file with MAC address in its name */
> +	sprintf(fn_ip->filename, "%s%02x-%02x-%02x-%02x-%02x-%02x", basedir,


This is missing ARP type "1". Should be: "%s01-%02x-%02x-%02x-%02x-%02x-%02x"