[SLOF] [PATCH v2 06/11] libnet: Add functions for downloading and parsing pxelinux.cfg files
Thomas Huth
thuth at redhat.com
Fri May 25 03:49:14 AEST 2018
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 at 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
More information about the SLOF
mailing list