[SLOF] [PATCH v2 06/11] libnet: Add functions for downloading and parsing pxelinux.cfg files
Alexey Kardashevskiy
aik at ozlabs.ru
Fri May 25 13:14:56 AEST 2018
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 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.
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 :)
--
Alexey
More information about the SLOF
mailing list