[7/7] syslinux: add syslinux parser support

Signed-off-by: Brett Grandbois <brett.grandbois@opengear.com>
---
 discover/Makefile.am                         |   3 +-
 discover/syslinux-parser.c                   | 486 +++++++++++++++++++++++++++
 test/parser/Makefile.am                      |  13 +-
 test/parser/data/syslinux-include-nest-1.cfg |   7 +
 test/parser/data/syslinux-include-nest-2.cfg |   6 +
 test/parser/data/syslinux-include-root.cfg   |  18 +
 test/parser/test-syslinux-explicit.c         |  41 +++
 test/parser/test-syslinux-global-append.c    |  56 +++
 test/parser/test-syslinux-nested-config.c    |  41 +++
 test/parser/test-syslinux-single-yocto.c     |  36 ++
 10 files changed, 704 insertions(+), 3 deletions(-)
 create mode 100644 discover/syslinux-parser.c
 create mode 100644 test/parser/data/syslinux-include-nest-1.cfg
 create mode 100644 test/parser/data/syslinux-include-nest-2.cfg
 create mode 100644 test/parser/data/syslinux-include-root.cfg
 create mode 100644 test/parser/test-syslinux-explicit.c
 create mode 100644 test/parser/test-syslinux-global-append.c
 create mode 100644 test/parser/test-syslinux-nested-config.c
 create mode 100644 test/parser/test-syslinux-single-yocto.c

On Thu, 2018-02-08 at 12:07 +1000, Brett Grandbois wrote:
> Signed-off-by: Brett Grandbois <brett.grandbois@opengear.com>
> ---
>  discover/Makefile.am                         |   3 +-
>  discover/syslinux-parser.c                   | 486 +++++++++++++++++++++++++++
>  test/parser/Makefile.am                      |  13 +-
>  test/parser/data/syslinux-include-nest-1.cfg |   7 +
>  test/parser/data/syslinux-include-nest-2.cfg |   6 +
>  test/parser/data/syslinux-include-root.cfg   |  18 +
>  test/parser/test-syslinux-explicit.c         |  41 +++
>  test/parser/test-syslinux-global-append.c    |  56 +++
>  test/parser/test-syslinux-nested-config.c    |  41 +++
>  test/parser/test-syslinux-single-yocto.c     |  36 ++
>  10 files changed, 704 insertions(+), 3 deletions(-)
>  create mode 100644 discover/syslinux-parser.c
>  create mode 100644 test/parser/data/syslinux-include-nest-1.cfg
>  create mode 100644 test/parser/data/syslinux-include-nest-2.cfg
>  create mode 100644 test/parser/data/syslinux-include-root.cfg
>  create mode 100644 test/parser/test-syslinux-explicit.c
>  create mode 100644 test/parser/test-syslinux-global-append.c
>  create mode 100644 test/parser/test-syslinux-nested-config.c
>  create mode 100644 test/parser/test-syslinux-single-yocto.c
> 
> diff --git a/discover/Makefile.am b/discover/Makefile.am
> index 4a6cbd0..ef4c602 100644
> --- a/discover/Makefile.am
> +++ b/discover/Makefile.am
> @@ -52,7 +52,8 @@ discover_pb_discover_SOURCES = \
>  	discover/user-event.h \
>  	discover/kboot-parser.c \
>  	discover/yaboot-parser.c \
> -	discover/pxe-parser.c
> +	discover/pxe-parser.c \
> +	discover/syslinux-parser.c

Technically there's crossover between pxe-parser and syslinux-parser but 
I'm happy to have these as separate implementations at the moment,
especially since we currently abuse the syslinux format a bit in pxe-
parser. Perhaps later on I can have a look at having a syslinux core
parser and putting the bonus bits in pxe-parser on top.

Otherwise looks good, and with tests no less! Some comments below:

>  
>  discover_pb_discover_LDADD = \
>  	discover/grub2/grub2-parser.ro \
> diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
> new file mode 100644
> index 0000000..7225a4f
> --- /dev/null
> +++ b/discover/syslinux-parser.c
> @@ -0,0 +1,486 @@
> +#if defined(HAVE_CONFIG_H)
> +#include "config.h"
> +#endif
> +
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <i18n/i18n.h>
> +#include <libgen.h>
> +
> +#include "log/log.h"
> +#include "list/list.h"
> +#include "file/file.h"
> +#include "talloc/talloc.h"
> +#include "types/types.h"
> +#include "parser-conf.h"
> +#include "parser-utils.h"
> +#include "resource.h"
> +#include "paths.h"
> +
> +struct syslinux_boot_option {
> +	char *label;
> +	char *image;
> +	char *append;
> +	char *initrd;
> +	struct list_item list;
> +};
> +
> +/* by spec 16 is allowed */
> +#define INCLUDE_NEST_LIMIT 16
> +
> +struct syslinux_options {
> +	struct list processed_options;
> +	struct syslinux_boot_option *current_option;
> +	int include_nest_level;
> +	char *cfg_dir;
> +};
> +
> +
> +static const char *const syslinux_conf_files[] = {
> +	"/boot/syslinux/syslinux.cfg",
> +	"/syslinux/syslinux.cfg",
> +	"/syslinux.cfg",
> +	"/BOOT/SYSLINUX/SYSLINUX.CFG",
> +	"/SYSLINUX/SYSLINUX.CFG",
> +	"/SYSLINUX.CFG",
> +	NULL
> +};
> +
> +static const char *const syslinux_kernel_unsupported_extensions[] = {
> +	".0", /* eventually support PXE here? */
> +	".bin",
> +	".bs",
> +	".bss",
> +	".c32",
> +	".cbt",
> +	".com",
> +	".img",
> +	NULL
> +};
> +
> +static const char *const syslinux_ignored_names[] = {
> +	"config",
> +	"sysapend",
> +	"localboot",
> +	"ui",
> +	"prompt",
> +	"noescape",
> +	"nocomplete",
> +	"allowoptions",
> +	"timeout",
> +	"totaltimeout",
> +	"ontimeout",
> +	"onerror",
> +	"serial",
> +	"nohalt",
> +	"console",
> +	"font",
> +	"kbdmap",
> +	"say",
> +	"display",
> +	"f1",
> +	"f2",
> +	"f3",
> +	"f4",
> +	"f5"
> +	"f6",
> +	"f7",
> +	"f8",
> +	"f9",
> +	"f10",
> +	"f11",
> +	"f12",
> +	NULL
> +};
> +
> +static const char *const syslinux_unsupported_boot_names[] = {
> +	"boot",
> +	"bss",
> +	"pxe",
> +	"fdimage",
> +	"comboot",
> +	"com32",
> +	NULL
> +};
> +
> +static struct conf_global_option syslinux_global_options[] = {
> +	{ .name = "default" },
> +	{ .name = "implicit" },
> +	{ .name = "append" },
> +	{ .name = NULL }
> +};
> +
> +
> +#define free_and_continue_on_false(X,Y) if (!(X)) { talloc_free((Y)); continue; }
> +
> +static void finish_boot_option(struct syslinux_options *state,
> +			       bool free_if_unused)
> +{
> +	/*
> +	 * in the normal this function signals a new image block which means
> +	 * move the current block to the list of processed items
> +	 * the special case is a label before an image block which we need to
> +	 * know whether to keep it for further processing or junk it
> +	 */
> +	if (state->current_option) {
> +		if (state->current_option->image) {
> +			list_add(&state->processed_options,
> +				 &state->current_option->list);
> +			state->current_option = NULL;
> +		}
> +		else if (free_if_unused) {
> +			talloc_free(state->current_option);
> +			state->current_option = NULL;
> +		}
> +	}
> +}
> +
> +static bool start_new_option(struct syslinux_options *state)
> +{
> +	bool ret = false;
> +
> +	finish_boot_option(state, false);
> +	if (!state->current_option)
> +		state->current_option = talloc_zero(state, struct syslinux_boot_option);
> +
> +	if (state->current_option)
> +		ret = true;
> +
> +	return ret;
> +}
> +
> +static void syslinux_process_pair(struct conf_context *conf, const char *name, char *value)
> +{
> +	struct syslinux_options *state = conf->parser_info;
> +	int len, rc;
> +	char *buf;
> +	char *pos;
> +	char *path;

Fine to just have
	char *buf, *pos, *path;

> +
> +	do {

We return right after the do { } while(false), would it be neater to just
return instead of breaking to save ourselves a level of indent? It would
also help keep a few of these longer lines under 80 characters.

> +		/* ignore bare values */
> +		if (!name)
> +			break;
> +
> +		if (conf_param_in_list(syslinux_ignored_names, name))
> +			break;
> +
> +		/* a new boot entry needs to terminate any prior one */
> +		if (conf_param_in_list(syslinux_unsupported_boot_names, name)) {
> +			finish_boot_option(state, true);
> +			break;
> +		}
> +
> +		if (streq(name, "label")) {
> +			finish_boot_option(state, true);
> +			state->current_option = talloc_zero(state,
> +						    struct syslinux_boot_option);
> +			if (state->current_option)
> +				state->current_option->label = talloc_strdup(state, value);
> +			break;
> +		}
> +
> +		if (streq(name, "linux")) {
> +
> +			if (start_new_option(state)) {
> +				state->current_option->image = talloc_strdup(state, value);
> +				if (!state->current_option->image) {
> +					talloc_free(state->current_option);
> +					state->current_option = NULL;
> +				}
> +			}
> +
> +			break;
> +		}
> +
> +		if (streq(name, "kernel")) {
> +
> +			if (start_new_option(state)) {
> +			/*
> +			 * by spec a linux image can not have any of these
> +			 * extensions, it can have no extension or anything not
> +			 * in this list
> +			 */
> +				pos = strrchr(value, '.');
> +				if (!pos ||
> +				!conf_param_in_list(syslinux_kernel_unsupported_extensions, pos)) {
> +					state->current_option->image = talloc_strdup(state, value);
> +					if (!state->current_option->image) {
> +						talloc_free(state->current_option);
> +						state->current_option = NULL;
> +					}
> +				}
> +				else	/* clean up any possible trailing label */
> +					finish_boot_option(state, true);
> +			}
> +			break;
> +		}
> +
> +
> +
> +		/* APPEND can be global and/or local so need special handling */
> +		if (streq(name, "append")) {
> +			if (state->current_option) {
> +				/* by spec only take last if multiple APPENDs */
> +				if (state->current_option->append)
> +					talloc_free(state->current_option->append);
> +				state->current_option->append = talloc_strdup(state, value);
> +				if (!state->current_option->append) {
> +					talloc_free(state->current_option);
> +					state->current_option = NULL;
> +				}
> +			}
> +			else {
> +				finish_boot_option(state, true);
> +				conf_set_global_option(conf, name, value);
> +			}
> +			break;
> +		}
> +
> +		/* now the general globals */
> +		if (conf_set_global_option(conf, name, value)) {
> +			finish_boot_option(state, true);
> +			break;
> +		}
> +
> +		if (streq(name, "initrd")) {
> +			if (state->current_option) {
> +				state->current_option->initrd = talloc_strdup(state, value);
> +				if (!state->current_option->initrd) {
> +					talloc_free(state->current_option);
> +					state->current_option = NULL;
> +				}
> +			}
> +			break;
> +		}
> +
> +		if (streq(name, "include")) {
> +			if (state->include_nest_level < INCLUDE_NEST_LIMIT) {
> +				state->include_nest_level++;
> +
> +				/* if absolute in as-is */
> +				if (value[0] == '/')
> +					path = talloc_strdup(state, value);
> +				else /* otherwise relative to the root config file */
> +					path = join_paths(state, state->cfg_dir, value);
> +
> +				rc = parser_request_file(conf->dc, conf->dc->device, path, &buf, &len);
> +				if (!rc) {
> +					conf_parse_buf(conf, buf, len);
> +
> +					device_handler_status_dev_info(conf->dc->handler, conf->dc->device,
> +					_("Parsed nested syslinux configuration from %s"), value);
> +					talloc_free(buf);
> +				}
> +				else {
> +					device_handler_status_dev_info(conf->dc->handler, conf->dc->device,
> +					_("Failed to parse nested syslinux configuration from %s"), value);
> +				}
> +
> +				talloc_free(path);
> +
> +				state->include_nest_level--;
> +			}
> +			else {
> +				device_handler_status_dev_err(conf->dc->handler, conf->dc->device,
> +					_("Nested syslinux INCLUDE exceeds limit...ignored"));
> +			}
> +			break;
> +		}
> +
> +		pb_debug("%s: unknown name: %s\n", __func__, name);
> +	} while(false);
> +}
> +
> +static void syslinux_finalize(struct conf_context *conf)
> +{
> +	struct syslinux_options *state = conf->parser_info;
> +	const char *global_append;
> +	const char *global_default;
> +	struct syslinux_boot_option *syslinux_opt;
> +	const char *image;
> +	const char *label;
> +	bool implicit_image = true;
> +	struct discover_context *dc = conf->dc;
> +	struct discover_boot_option *d_opt;
> +	struct boot_option *opt;
> +	char *args_sigfile_default;
> +
> +	/* clean up any lingering boot entries */
> +	finish_boot_option(state, true);
> +
> +	global_append  = conf_get_global_option(conf, "append");
> +	global_default = conf_get_global_option(conf, "default");
> +
> +	/*
> +	 * by spec '0' means disable
> +	 * note we set the default to '1' (which is by spec) in syslinux_parse
> +	 */
> +	if (conf_get_global_option(conf, "implicit"), "0")
> +		implicit_image = false;
> +
> +	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
> +		/* need a valid image */
> +		if (!syslinux_opt->image)
> +			continue;
> +
> +		image = syslinux_opt->image;
> +		label = syslinux_opt->label;
> +
> +		/* if implicit is disabled we must have a label */
> +		if (!label && !implicit_image)
> +			continue;
> +
> +		d_opt = discover_boot_option_create(dc, dc->device);
> +		if (!d_opt) continue;

Indent this

> +		free_and_continue_on_false(d_opt->option, d_opt);

Despite the big hint in the name this macro still throws me off a little
due to the 'hidden' continue in it. I think I'd be happier trading a
bigger line count to have something like

	static inline bool free_if_null(foo, bar)
	{
		if (!foo)
			talloc_free(bar);
		return !foo;
	}
	
	...

	if (free_if_null(d_opt->option, d_opt))
		continue;

Or in this particular case since 'bar' is always d_opt,

	if (!d_opt->option)
		goto fail;

	...

fail:
	talloc_free(d_opt)
	} /* loop */

Thoughts?

> +
> +		opt = d_opt->option;
> +
> +		if (syslinux_opt->append) {
> +			/* '-' can signal do not use global APPEND */
> +			if (!strcmp(syslinux_opt->append, "-"))
> +				opt->boot_args = talloc_strdup(opt, "");
> +			else
> +				opt->boot_args = talloc_asprintf(opt, "%s %s",
> +								 global_append,
> +								 syslinux_opt->append);
> +		}
> +		else

} else

> +			opt->boot_args = talloc_strdup(opt, global_append);
> +
> +		free_and_continue_on_false(opt->boot_args, d_opt);
> +
> +		opt->id = talloc_asprintf(opt, "%s#%s", dc->device->device->id, image);
> +
> +		free_and_continue_on_false(opt->id, d_opt);
> +
> +		if (label) {
> +			opt->name = talloc_strdup(opt, label);
> +			if (!strcmp(label, global_default))
> +				opt->is_default = true;
> +
> +			opt->description = talloc_asprintf(opt, "(%s) %s", label, image);
> +		}
> +		else {
> +			opt->name = talloc_strdup(opt, image);
> +			opt->description = talloc_strdup(opt, image);
> +		}
> +
> +		free_and_continue_on_false(opt->name, d_opt);
> +
> +		d_opt->boot_image = create_devpath_resource(d_opt, dc->device, image);
> +
> +		free_and_continue_on_false(d_opt->boot_image, d_opt);
> +
> +		if (syslinux_opt->initrd) {
> +			d_opt->initrd = create_devpath_resource(d_opt, dc->device,
> +								syslinux_opt->initrd);
> +			opt->description = talloc_asprintf_append(opt->description, 
> +								  " initrd=%s",
> +								  syslinux_opt->initrd);
> +
> +			free_and_continue_on_false(d_opt->initrd, d_opt);
> +		}
> +
> +		opt->description = talloc_asprintf_append(opt->description,
> +							  " args=%s",
> +							  opt->boot_args);
> +
> +		free_and_continue_on_false(opt->description, d_opt);
> +
> +		args_sigfile_default = talloc_asprintf(d_opt, "%s.cmdline.sig",
> +						       image);
> +		free_and_continue_on_false(args_sigfile_default, d_opt);
> +
> +		d_opt->args_sig_file = create_devpath_resource(d_opt, dc->device,
> +							       args_sigfile_default);
> +		free_and_continue_on_false(d_opt->args_sig_file, d_opt);
> +
> +		talloc_free(args_sigfile_default);
> +
> +		conf_strip_str(opt->boot_args);
> +		conf_strip_str(opt->description);
> +
> +		discover_context_add_boot_option(dc, d_opt);
> +	}
> +}
> +
> +
> +

Super nit-picky but extra space :)

> +static int syslinux_parse(struct discover_context *dc)
> +{
> +	struct conf_context *conf;
> +	const char * const *filename;
> +	char *cfg_dir;
> +	struct syslinux_options *state;
> +	char *buf;
> +	int len, rc;

And we tend to order declarations in descending length, eg:

	struct syslinux_options *state;
	const char * const *filename;
	struct conf_context *conf;
	char *cfg_dir, *buf;
	int len, rc;

> +
> +	/* Support block device boot only at present */
> +	if (dc->event)
> +		return -1;
> +
> +	conf = talloc_zero(dc, struct conf_context);
> +
> +	if (!conf)
> +		return -1;
> +
> +	conf->dc = dc;
> +	conf->global_options = syslinux_global_options,
> +	conf_init_global_options(conf);
> +	conf->get_pair = conf_get_pair_space;
> +	conf->process_pair = syslinux_process_pair;
> +
> +	conf->parser_info = state = talloc_zero(conf, struct syslinux_options);
> +	list_init(&state->processed_options);
> +
> +	/*
> +	 * set the global defaults
> +	 * by spec 'default' defaults to 'linux' and
> +	 * 'implicit' defaults to '1', we also just set
> +	 * and empty string in 'append' to make it easier
> +	 * in syslinux_finish
> +	 */
> +	conf_set_global_option(conf, "default", "linux");
> +	conf_set_global_option(conf, "implicit", "1");
> +	conf_set_global_option(conf, "append", "");
> +
> +	for (filename = syslinux_conf_files; *filename; filename++) {
> +		rc = parser_request_file(dc, dc->device, *filename, &buf, &len);
> +		if (rc)
> +			continue;
> +
> +		/*
> +		 * save location of root config file for possible
> +		 * INCLUDE directives later
> +		 *
> +		 * dirname can overwrite so need local copy to work on
> +		 */
> +		cfg_dir = talloc_strdup(conf, *filename);
> +		state->cfg_dir = talloc_strdup(state, dirname(cfg_dir));
> +		talloc_free(cfg_dir);
> +
> +		conf_parse_buf(conf, buf, len);
> +		device_handler_status_dev_info(dc->handler, dc->device,
> +				_("Parsed syslinux configuration from %s"),
> +				*filename);
> +		talloc_free(buf);
> +
> +		syslinux_finalize(conf);
> +	}
> +
> +	talloc_free(conf);
> +	return 0;
> +}
> +
> +
> +
> +static struct parser syslinux_parser = {
> +	.name			= "syslinux",
> +	.parse			= syslinux_parse,
> +	.resolve_resource	= resolve_devpath_resource,
> +};
> +
> +register_parser(syslinux_parser);
> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
> index 31300f0..a0795db 100644
> --- a/test/parser/Makefile.am
> +++ b/test/parser/Makefile.am
> @@ -73,7 +73,12 @@ parser_TESTS = \
>  	test/parser/test-pxe-discover-bootfile-relative-conffile \
>  	test/parser/test-pxe-discover-bootfile-absolute-conffile \
>  	test/parser/test-pxe-discover-bootfile-async-file \
> -	test/parser/test-unresolved-remove
> +	test/parser/test-unresolved-remove \
> +	test/parser/test-syslinux-single-yocto \
> +	test/parser/test-syslinux-global-append \
> +	test/parser/test-syslinux-explicit \
> +	test/parser/test-syslinux-nested-config
> +
>  
>  TESTS += $(parser_TESTS)
>  check_PROGRAMS += $(parser_TESTS) test/parser/libtest.ro
> @@ -82,7 +87,10 @@ check_DATA += \
>  	test/parser/data/grub2-f18-ppc64.conf \
>  	test/parser/data/grub2-f20-ppc.conf \
>  	test/parser/data/grub2-ubuntu-13_04-x86.conf \
> -	test/parser/data/yaboot-rh8-ppc64.conf
> +	test/parser/data/yaboot-rh8-ppc64.conf \
> +	test/parser/data/syslinux-include-root.cfg \
> +	test/parser/data/syslinux-include-nest-1.cfg \
> +	test/parser/data/syslinux-include-nest-2.cfg
>  
>  $(parser_TESTS): AM_CPPFLAGS += \
>  		-I$(top_srcdir)/discover \
> @@ -107,6 +115,7 @@ test_parser_libtest_ro_SOURCES = \
>  	discover/yaboot-parser.c \
>  	discover/kboot-parser.c \
>  	discover/pxe-parser.c \
> +	discover/syslinux-parser.c \
>  	discover/platform.c \
>  	discover/resource.c \
>  	discover/paths.c \
> diff --git a/test/parser/data/syslinux-include-nest-1.cfg b/test/parser/data/syslinux-include-nest-1.cfg
> new file mode 100644
> index 0000000..65e680e
> --- /dev/null
> +++ b/test/parser/data/syslinux-include-nest-1.cfg
> @@ -0,0 +1,7 @@
> +LABEL com32
> +COM32 /boot/com32.c32
> +
> +INCLUDE syslinux-include-nest-2.cfg
> +
> +LABEL bss
> +KERNEL /boot/test.bss
> diff --git a/test/parser/data/syslinux-include-nest-2.cfg b/test/parser/data/syslinux-include-nest-2.cfg
> new file mode 100644
> index 0000000..26d7cff
> --- /dev/null
> +++ b/test/parser/data/syslinux-include-nest-2.cfg
> @@ -0,0 +1,6 @@
> +LABEL boot
> +KERNEL /bzImage-boot
> +INITRD /initrd-boot
> +APPEND root=/dev/sda
> +
> +DEFAULT boot
> diff --git a/test/parser/data/syslinux-include-root.cfg b/test/parser/data/syslinux-include-root.cfg
> new file mode 100644
> index 0000000..834360c
> --- /dev/null
> +++ b/test/parser/data/syslinux-include-root.cfg
> @@ -0,0 +1,18 @@
> +APPEND console=ttyS0
> +
> +LABEL floppy
> +FDIMAGE /boot/floppy.img
> +
> +LABEL backup
> +KERNEL /backup/vmlinuz
> +APPEND root=/dev/sdb
> +INITRD /boot/initrd
> +
> +LABEL comboot
> +KERNEL /boot/comboot.com
> +
> +INCLUDE /syslinux-include-nest-1.cfg
> +
> +LABEL linux
> +LINUX /boot/bzImage
> +APPEND root=/dev/sdc
> diff --git a/test/parser/test-syslinux-explicit.c b/test/parser/test-syslinux-explicit.c
> new file mode 100644
> index 0000000..5d23f50
> --- /dev/null
> +++ b/test/parser/test-syslinux-explicit.c
> @@ -0,0 +1,41 @@
> +/* test a standard yocto syslinux wic cfg */
> +
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +
> +
> +DEFAULT boot
> +
> +KERNEL /vmlinuz
> +APPEND console=tty0
> +
> +LABEL backup
> +KERNEL /backup/vmlinuz
> +APPEND root=/dev/sdb
> +INITRD /boot/initrd
> +
> +IMPLICIT 0
> +
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_read_conf_embedded(test, "/boot/syslinux/syslinux.cfg");
> +
> +	test_run_parser(test, "syslinux");
> +
> +	ctx = test->ctx;
> +
> +	check_boot_option_count(ctx, 1);
> +
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_name(opt, "backup");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
> +	check_args(opt, " root=/dev/sdb");
> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
> +}
> diff --git a/test/parser/test-syslinux-global-append.c b/test/parser/test-syslinux-global-append.c
> new file mode 100644
> index 0000000..18af99a
> --- /dev/null
> +++ b/test/parser/test-syslinux-global-append.c
> @@ -0,0 +1,56 @@
> +
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +
> +APPEND console=ttyS0
> +
> +LABEL linux
> +LINUX /vmlinuz
> +APPEND console=tty0
> +
> +LABEL backup
> +KERNEL /backup/vmlinuz
> +APPEND root=/dev/sdb
> +INITRD /boot/initrd
> +
> +LABEL hyphen
> +KERNEL /test/vmlinuz
> +APPEND -
> +
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_read_conf_embedded(test, "/syslinux/syslinux.cfg");
> +
> +	test_run_parser(test, "syslinux");
> +
> +	ctx = test->ctx;
> +
> +	check_boot_option_count(ctx, 3);
> +	opt = get_boot_option(ctx, 2);
> +
> +	check_name(opt, "linux");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/vmlinuz");
> +	check_is_default(opt);
> +	check_args(opt, "console=ttyS0 console=tty0");
> +	check_not_present_resource(opt->initrd);
> +
> +	opt = get_boot_option(ctx, 1);
> +
> +	check_name(opt, "backup");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
> +	check_args(opt, "console=ttyS0 root=/dev/sdb");
> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
> +
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_name(opt, "hyphen");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/test/vmlinuz");
> +	check_args(opt, "");
> +	check_not_present_resource(opt->initrd);
> +}
> diff --git a/test/parser/test-syslinux-nested-config.c b/test/parser/test-syslinux-nested-config.c
> new file mode 100644
> index 0000000..73c4774
> --- /dev/null
> +++ b/test/parser/test-syslinux-nested-config.c
> @@ -0,0 +1,41 @@
> +
> +#include "parser-test.h"
> +
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_read_conf_file(test, "syslinux-include-root.cfg", "/boot/syslinux/syslinux.cfg");
> +	test_read_conf_file(test, "syslinux-include-nest-1.cfg", "/syslinux-include-nest-1.cfg");
> +	test_read_conf_file(test, "syslinux-include-nest-2.cfg", "/boot/syslinux/syslinux-include-nest-2.cfg");
> +
> +	test_run_parser(test, "syslinux");
> +
> +	ctx = test->ctx;
> +
> +	check_boot_option_count(ctx, 3);
> +
> +	opt = get_boot_option(ctx, 1);
> +
> +	check_name(opt, "boot");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/bzImage-boot");
> +	check_is_default(opt);
> +	check_args(opt, "console=ttyS0 root=/dev/sda");
> +	check_resolved_local_resource(opt->initrd, ctx->device, "/initrd-boot");
> +
> +	opt = get_boot_option(ctx, 2);
> +
> +	check_name(opt, "backup");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
> +	check_args(opt, "console=ttyS0 root=/dev/sdb");
> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
> +
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_name(opt, "linux");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/boot/bzImage");
> +	check_args(opt, "console=ttyS0 root=/dev/sdc");
> +	check_not_present_resource(opt->initrd);
> +}
> diff --git a/test/parser/test-syslinux-single-yocto.c b/test/parser/test-syslinux-single-yocto.c
> new file mode 100644
> index 0000000..e5e084d
> --- /dev/null
> +++ b/test/parser/test-syslinux-single-yocto.c
> @@ -0,0 +1,36 @@
> +/* test a standard yocto syslinux wic cfg */
> +
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +PROMPT 0
> +TIMEOUT 0
> +
> +ALLOWOPTIONS 1
> +SERIAL 0 115200
> +
> +DEFAULT boot
> +LABEL boot
> +KERNEL /vmlinuz
> +APPEND console=ttyS0,115200n8 console=tty0
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_read_conf_embedded(test, "/syslinux.cfg");
> +
> +	test_run_parser(test, "syslinux");
> +
> +	ctx = test->ctx;
> +
> +	check_boot_option_count(ctx, 1);
> +	opt = get_boot_option(ctx, 0);
> +
> +	check_name(opt, "boot");
> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/vmlinuz");
> +	check_is_default(opt);
> +	check_args(opt, " console=ttyS0,115200n8 console=tty0");
> +}

On 08/02/18 14:11, Samuel Mendoza-Jonas wrote:
> On Thu, 2018-02-08 at 12:07 +1000, Brett Grandbois wrote:
>> Signed-off-by: Brett Grandbois <brett.grandbois@opengear.com>
>> ---
>>   discover/Makefile.am                         |   3 +-
>>   discover/syslinux-parser.c                   | 486 +++++++++++++++++++++++++++
>>   test/parser/Makefile.am                      |  13 +-
>>   test/parser/data/syslinux-include-nest-1.cfg |   7 +
>>   test/parser/data/syslinux-include-nest-2.cfg |   6 +
>>   test/parser/data/syslinux-include-root.cfg   |  18 +
>>   test/parser/test-syslinux-explicit.c         |  41 +++
>>   test/parser/test-syslinux-global-append.c    |  56 +++
>>   test/parser/test-syslinux-nested-config.c    |  41 +++
>>   test/parser/test-syslinux-single-yocto.c     |  36 ++
>>   10 files changed, 704 insertions(+), 3 deletions(-)
>>   create mode 100644 discover/syslinux-parser.c
>>   create mode 100644 test/parser/data/syslinux-include-nest-1.cfg
>>   create mode 100644 test/parser/data/syslinux-include-nest-2.cfg
>>   create mode 100644 test/parser/data/syslinux-include-root.cfg
>>   create mode 100644 test/parser/test-syslinux-explicit.c
>>   create mode 100644 test/parser/test-syslinux-global-append.c
>>   create mode 100644 test/parser/test-syslinux-nested-config.c
>>   create mode 100644 test/parser/test-syslinux-single-yocto.c
>>
>> diff --git a/discover/Makefile.am b/discover/Makefile.am
>> index 4a6cbd0..ef4c602 100644
>> --- a/discover/Makefile.am
>> +++ b/discover/Makefile.am
>> @@ -52,7 +52,8 @@ discover_pb_discover_SOURCES = \
>>   	discover/user-event.h \
>>   	discover/kboot-parser.c \
>>   	discover/yaboot-parser.c \
>> -	discover/pxe-parser.c
>> +	discover/pxe-parser.c \
>> +	discover/syslinux-parser.c
> Technically there's crossover between pxe-parser and syslinux-parser but
> I'm happy to have these as separate implementations at the moment,
> especially since we currently abuse the syslinux format a bit in pxe-
> parser. Perhaps later on I can have a look at having a syslinux core
> parser and putting the bonus bits in pxe-parser on top.

I have to admit I didn't look closely at the pxe-parser.  I wrote this as a means to evaluate petitboot in a legacy
environment of ours and since the pxe-parser wasn't parsing syslinux.cfg I passed over it.  Since they search for
different configuration files they can coexist but probably the best long-term solution would be to merge them.

> Otherwise looks good, and with tests no less! Some comments below:
>
>>   
>>   discover_pb_discover_LDADD = \
>>   	discover/grub2/grub2-parser.ro \
>> diff --git a/discover/syslinux-parser.c b/discover/syslinux-parser.c
>> new file mode 100644
>> index 0000000..7225a4f
>> --- /dev/null
>> +++ b/discover/syslinux-parser.c
>> @@ -0,0 +1,486 @@
>> +#if defined(HAVE_CONFIG_H)
>> +#include "config.h"
>> +#endif
>> +
>> +#include <assert.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <i18n/i18n.h>
>> +#include <libgen.h>
>> +
>> +#include "log/log.h"
>> +#include "list/list.h"
>> +#include "file/file.h"
>> +#include "talloc/talloc.h"
>> +#include "types/types.h"
>> +#include "parser-conf.h"
>> +#include "parser-utils.h"
>> +#include "resource.h"
>> +#include "paths.h"
>> +
>> +struct syslinux_boot_option {
>> +	char *label;
>> +	char *image;
>> +	char *append;
>> +	char *initrd;
>> +	struct list_item list;
>> +};
>> +
>> +/* by spec 16 is allowed */
>> +#define INCLUDE_NEST_LIMIT 16
>> +
>> +struct syslinux_options {
>> +	struct list processed_options;
>> +	struct syslinux_boot_option *current_option;
>> +	int include_nest_level;
>> +	char *cfg_dir;
>> +};
>> +
>> +
>> +static const char *const syslinux_conf_files[] = {
>> +	"/boot/syslinux/syslinux.cfg",
>> +	"/syslinux/syslinux.cfg",
>> +	"/syslinux.cfg",
>> +	"/BOOT/SYSLINUX/SYSLINUX.CFG",
>> +	"/SYSLINUX/SYSLINUX.CFG",
>> +	"/SYSLINUX.CFG",
>> +	NULL
>> +};
>> +
>> +static const char *const syslinux_kernel_unsupported_extensions[] = {
>> +	".0", /* eventually support PXE here? */
>> +	".bin",
>> +	".bs",
>> +	".bss",
>> +	".c32",
>> +	".cbt",
>> +	".com",
>> +	".img",
>> +	NULL
>> +};
>> +
>> +static const char *const syslinux_ignored_names[] = {
>> +	"config",
>> +	"sysapend",
>> +	"localboot",
>> +	"ui",
>> +	"prompt",
>> +	"noescape",
>> +	"nocomplete",
>> +	"allowoptions",
>> +	"timeout",
>> +	"totaltimeout",
>> +	"ontimeout",
>> +	"onerror",
>> +	"serial",
>> +	"nohalt",
>> +	"console",
>> +	"font",
>> +	"kbdmap",
>> +	"say",
>> +	"display",
>> +	"f1",
>> +	"f2",
>> +	"f3",
>> +	"f4",
>> +	"f5"
>> +	"f6",
>> +	"f7",
>> +	"f8",
>> +	"f9",
>> +	"f10",
>> +	"f11",
>> +	"f12",
>> +	NULL
>> +};
>> +
>> +static const char *const syslinux_unsupported_boot_names[] = {
>> +	"boot",
>> +	"bss",
>> +	"pxe",
>> +	"fdimage",
>> +	"comboot",
>> +	"com32",
>> +	NULL
>> +};
>> +
>> +static struct conf_global_option syslinux_global_options[] = {
>> +	{ .name = "default" },
>> +	{ .name = "implicit" },
>> +	{ .name = "append" },
>> +	{ .name = NULL }
>> +};
>> +
>> +
>> +#define free_and_continue_on_false(X,Y) if (!(X)) { talloc_free((Y)); continue; }
>> +
>> +static void finish_boot_option(struct syslinux_options *state,
>> +			       bool free_if_unused)
>> +{
>> +	/*
>> +	 * in the normal this function signals a new image block which means
>> +	 * move the current block to the list of processed items
>> +	 * the special case is a label before an image block which we need to
>> +	 * know whether to keep it for further processing or junk it
>> +	 */
>> +	if (state->current_option) {
>> +		if (state->current_option->image) {
>> +			list_add(&state->processed_options,
>> +				 &state->current_option->list);
>> +			state->current_option = NULL;
>> +		}
>> +		else if (free_if_unused) {
>> +			talloc_free(state->current_option);
>> +			state->current_option = NULL;
>> +		}
>> +	}
>> +}
>> +
>> +static bool start_new_option(struct syslinux_options *state)
>> +{
>> +	bool ret = false;
>> +
>> +	finish_boot_option(state, false);
>> +	if (!state->current_option)
>> +		state->current_option = talloc_zero(state, struct syslinux_boot_option);
>> +
>> +	if (state->current_option)
>> +		ret = true;
>> +
>> +	return ret;
>> +}
>> +
>> +static void syslinux_process_pair(struct conf_context *conf, const char *name, char *value)
>> +{
>> +	struct syslinux_options *state = conf->parser_info;
>> +	int len, rc;
>> +	char *buf;
>> +	char *pos;
>> +	char *path;
> Fine to just have
> 	char *buf, *pos, *path;
>
>> +
>> +	do {
> We return right after the do { } while(false), would it be neater to just
> return instead of breaking to save ourselves a level of indent? It would
> also help keep a few of these longer lines under 80 characters.

I was brought up in a 'You shall only have ONE return from function' environment.  Mainly because at the time
multiple returns were freaking out the complexity analysis tool we were using, but because of that its been so
drilled into me I just unconsciously do it.  If multiple returns is the policy I don't have any problems changing to that.

>
>> +		/* ignore bare values */
>> +		if (!name)
>> +			break;
>> +
>> +		if (conf_param_in_list(syslinux_ignored_names, name))
>> +			break;
>> +
>> +		/* a new boot entry needs to terminate any prior one */
>> +		if (conf_param_in_list(syslinux_unsupported_boot_names, name)) {
>> +			finish_boot_option(state, true);
>> +			break;
>> +		}
>> +
>> +		if (streq(name, "label")) {
>> +			finish_boot_option(state, true);
>> +			state->current_option = talloc_zero(state,
>> +						    struct syslinux_boot_option);
>> +			if (state->current_option)
>> +				state->current_option->label = talloc_strdup(state, value);
>> +			break;
>> +		}
>> +
>> +		if (streq(name, "linux")) {
>> +
>> +			if (start_new_option(state)) {
>> +				state->current_option->image = talloc_strdup(state, value);
>> +				if (!state->current_option->image) {
>> +					talloc_free(state->current_option);
>> +					state->current_option = NULL;
>> +				}
>> +			}
>> +
>> +			break;
>> +		}
>> +
>> +		if (streq(name, "kernel")) {
>> +
>> +			if (start_new_option(state)) {
>> +			/*
>> +			 * by spec a linux image can not have any of these
>> +			 * extensions, it can have no extension or anything not
>> +			 * in this list
>> +			 */
>> +				pos = strrchr(value, '.');
>> +				if (!pos ||
>> +				!conf_param_in_list(syslinux_kernel_unsupported_extensions, pos)) {
>> +					state->current_option->image = talloc_strdup(state, value);
>> +					if (!state->current_option->image) {
>> +						talloc_free(state->current_option);
>> +						state->current_option = NULL;
>> +					}
>> +				}
>> +				else	/* clean up any possible trailing label */
>> +					finish_boot_option(state, true);
>> +			}
>> +			break;
>> +		}
>> +
>> +
>> +
>> +		/* APPEND can be global and/or local so need special handling */
>> +		if (streq(name, "append")) {
>> +			if (state->current_option) {
>> +				/* by spec only take last if multiple APPENDs */
>> +				if (state->current_option->append)
>> +					talloc_free(state->current_option->append);
>> +				state->current_option->append = talloc_strdup(state, value);
>> +				if (!state->current_option->append) {
>> +					talloc_free(state->current_option);
>> +					state->current_option = NULL;
>> +				}
>> +			}
>> +			else {
>> +				finish_boot_option(state, true);
>> +				conf_set_global_option(conf, name, value);
>> +			}
>> +			break;
>> +		}
>> +
>> +		/* now the general globals */
>> +		if (conf_set_global_option(conf, name, value)) {
>> +			finish_boot_option(state, true);
>> +			break;
>> +		}
>> +
>> +		if (streq(name, "initrd")) {
>> +			if (state->current_option) {
>> +				state->current_option->initrd = talloc_strdup(state, value);
>> +				if (!state->current_option->initrd) {
>> +					talloc_free(state->current_option);
>> +					state->current_option = NULL;
>> +				}
>> +			}
>> +			break;
>> +		}
>> +
>> +		if (streq(name, "include")) {
>> +			if (state->include_nest_level < INCLUDE_NEST_LIMIT) {
>> +				state->include_nest_level++;
>> +
>> +				/* if absolute in as-is */
>> +				if (value[0] == '/')
>> +					path = talloc_strdup(state, value);
>> +				else /* otherwise relative to the root config file */
>> +					path = join_paths(state, state->cfg_dir, value);
>> +
>> +				rc = parser_request_file(conf->dc, conf->dc->device, path, &buf, &len);
>> +				if (!rc) {
>> +					conf_parse_buf(conf, buf, len);
>> +
>> +					device_handler_status_dev_info(conf->dc->handler, conf->dc->device,
>> +					_("Parsed nested syslinux configuration from %s"), value);
>> +					talloc_free(buf);
>> +				}
>> +				else {
>> +					device_handler_status_dev_info(conf->dc->handler, conf->dc->device,
>> +					_("Failed to parse nested syslinux configuration from %s"), value);
>> +				}
>> +
>> +				talloc_free(path);
>> +
>> +				state->include_nest_level--;
>> +			}
>> +			else {
>> +				device_handler_status_dev_err(conf->dc->handler, conf->dc->device,
>> +					_("Nested syslinux INCLUDE exceeds limit...ignored"));
>> +			}
>> +			break;
>> +		}
>> +
>> +		pb_debug("%s: unknown name: %s\n", __func__, name);
>> +	} while(false);
>> +}
>> +
>> +static void syslinux_finalize(struct conf_context *conf)
>> +{
>> +	struct syslinux_options *state = conf->parser_info;
>> +	const char *global_append;
>> +	const char *global_default;
>> +	struct syslinux_boot_option *syslinux_opt;
>> +	const char *image;
>> +	const char *label;
>> +	bool implicit_image = true;
>> +	struct discover_context *dc = conf->dc;
>> +	struct discover_boot_option *d_opt;
>> +	struct boot_option *opt;
>> +	char *args_sigfile_default;
>> +
>> +	/* clean up any lingering boot entries */
>> +	finish_boot_option(state, true);
>> +
>> +	global_append  = conf_get_global_option(conf, "append");
>> +	global_default = conf_get_global_option(conf, "default");
>> +
>> +	/*
>> +	 * by spec '0' means disable
>> +	 * note we set the default to '1' (which is by spec) in syslinux_parse
>> +	 */
>> +	if (conf_get_global_option(conf, "implicit"), "0")
>> +		implicit_image = false;
>> +
>> +	list_for_each_entry(&state->processed_options, syslinux_opt, list) {
>> +		/* need a valid image */
>> +		if (!syslinux_opt->image)
>> +			continue;
>> +
>> +		image = syslinux_opt->image;
>> +		label = syslinux_opt->label;
>> +
>> +		/* if implicit is disabled we must have a label */
>> +		if (!label && !implicit_image)
>> +			continue;
>> +
>> +		d_opt = discover_boot_option_create(dc, dc->device);
>> +		if (!d_opt) continue;
> Indent this
>
>> +		free_and_continue_on_false(d_opt->option, d_opt);
> Despite the big hint in the name this macro still throws me off a little
> due to the 'hidden' continue in it. I think I'd be happier trading a
> bigger line count to have something like
>
> 	static inline bool free_if_null(foo, bar)
> 	{
> 		if (!foo)
> 			talloc_free(bar);
> 		return !foo;
> 	}
> 	
> 	...
>
> 	if (free_if_null(d_opt->option, d_opt))
> 		continue;
>
> Or in this particular case since 'bar' is always d_opt,
>
> 	if (!d_opt->option)
> 		goto fail;
>
> 	...
>
> fail:
> 	talloc_free(d_opt)
> 	} /* loop */
>
> Thoughts?

Nope I don't have any problem with that.  I'd probably go with the latter for simplicity.

>> +
>> +		opt = d_opt->option;
>> +
>> +		if (syslinux_opt->append) {
>> +			/* '-' can signal do not use global APPEND */
>> +			if (!strcmp(syslinux_opt->append, "-"))
>> +				opt->boot_args = talloc_strdup(opt, "");
>> +			else
>> +				opt->boot_args = talloc_asprintf(opt, "%s %s",
>> +								 global_append,
>> +								 syslinux_opt->append);
>> +		}
>> +		else
> } else
>
>> +			opt->boot_args = talloc_strdup(opt, global_append);
>> +
>> +		free_and_continue_on_false(opt->boot_args, d_opt);
>> +
>> +		opt->id = talloc_asprintf(opt, "%s#%s", dc->device->device->id, image);
>> +
>> +		free_and_continue_on_false(opt->id, d_opt);
>> +
>> +		if (label) {
>> +			opt->name = talloc_strdup(opt, label);
>> +			if (!strcmp(label, global_default))
>> +				opt->is_default = true;
>> +
>> +			opt->description = talloc_asprintf(opt, "(%s) %s", label, image);
>> +		}
>> +		else {
>> +			opt->name = talloc_strdup(opt, image);
>> +			opt->description = talloc_strdup(opt, image);
>> +		}
>> +
>> +		free_and_continue_on_false(opt->name, d_opt);
>> +
>> +		d_opt->boot_image = create_devpath_resource(d_opt, dc->device, image);
>> +
>> +		free_and_continue_on_false(d_opt->boot_image, d_opt);
>> +
>> +		if (syslinux_opt->initrd) {
>> +			d_opt->initrd = create_devpath_resource(d_opt, dc->device,
>> +								syslinux_opt->initrd);
>> +			opt->description = talloc_asprintf_append(opt->description,
>> +								  " initrd=%s",
>> +								  syslinux_opt->initrd);
>> +
>> +			free_and_continue_on_false(d_opt->initrd, d_opt);
>> +		}
>> +
>> +		opt->description = talloc_asprintf_append(opt->description,
>> +							  " args=%s",
>> +							  opt->boot_args);
>> +
>> +		free_and_continue_on_false(opt->description, d_opt);
>> +
>> +		args_sigfile_default = talloc_asprintf(d_opt, "%s.cmdline.sig",
>> +						       image);
>> +		free_and_continue_on_false(args_sigfile_default, d_opt);
>> +
>> +		d_opt->args_sig_file = create_devpath_resource(d_opt, dc->device,
>> +							       args_sigfile_default);
>> +		free_and_continue_on_false(d_opt->args_sig_file, d_opt);
>> +
>> +		talloc_free(args_sigfile_default);
>> +
>> +		conf_strip_str(opt->boot_args);
>> +		conf_strip_str(opt->description);
>> +
>> +		discover_context_add_boot_option(dc, d_opt);
>> +	}
>> +}
>> +
>> +
>> +
> Super nit-picky but extra space :)
>
>> +static int syslinux_parse(struct discover_context *dc)
>> +{
>> +	struct conf_context *conf;
>> +	const char * const *filename;
>> +	char *cfg_dir;
>> +	struct syslinux_options *state;
>> +	char *buf;
>> +	int len, rc;
> And we tend to order declarations in descending length, eg:
>
> 	struct syslinux_options *state;
> 	const char * const *filename;
> 	struct conf_context *conf;
> 	char *cfg_dir, *buf;
> 	int len, rc;
>
>> +
>> +	/* Support block device boot only at present */
>> +	if (dc->event)
>> +		return -1;
>> +
>> +	conf = talloc_zero(dc, struct conf_context);
>> +
>> +	if (!conf)
>> +		return -1;
>> +
>> +	conf->dc = dc;
>> +	conf->global_options = syslinux_global_options,
>> +	conf_init_global_options(conf);
>> +	conf->get_pair = conf_get_pair_space;
>> +	conf->process_pair = syslinux_process_pair;
>> +
>> +	conf->parser_info = state = talloc_zero(conf, struct syslinux_options);
>> +	list_init(&state->processed_options);
>> +
>> +	/*
>> +	 * set the global defaults
>> +	 * by spec 'default' defaults to 'linux' and
>> +	 * 'implicit' defaults to '1', we also just set
>> +	 * and empty string in 'append' to make it easier
>> +	 * in syslinux_finish
>> +	 */
>> +	conf_set_global_option(conf, "default", "linux");
>> +	conf_set_global_option(conf, "implicit", "1");
>> +	conf_set_global_option(conf, "append", "");
>> +
>> +	for (filename = syslinux_conf_files; *filename; filename++) {
>> +		rc = parser_request_file(dc, dc->device, *filename, &buf, &len);
>> +		if (rc)
>> +			continue;
>> +
>> +		/*
>> +		 * save location of root config file for possible
>> +		 * INCLUDE directives later
>> +		 *
>> +		 * dirname can overwrite so need local copy to work on
>> +		 */
>> +		cfg_dir = talloc_strdup(conf, *filename);
>> +		state->cfg_dir = talloc_strdup(state, dirname(cfg_dir));
>> +		talloc_free(cfg_dir);
>> +
>> +		conf_parse_buf(conf, buf, len);
>> +		device_handler_status_dev_info(dc->handler, dc->device,
>> +				_("Parsed syslinux configuration from %s"),
>> +				*filename);
>> +		talloc_free(buf);
>> +
>> +		syslinux_finalize(conf);
>> +	}
>> +
>> +	talloc_free(conf);
>> +	return 0;
>> +}
>> +
>> +
>> +
>> +static struct parser syslinux_parser = {
>> +	.name			= "syslinux",
>> +	.parse			= syslinux_parse,
>> +	.resolve_resource	= resolve_devpath_resource,
>> +};
>> +
>> +register_parser(syslinux_parser);
>> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
>> index 31300f0..a0795db 100644
>> --- a/test/parser/Makefile.am
>> +++ b/test/parser/Makefile.am
>> @@ -73,7 +73,12 @@ parser_TESTS = \
>>   	test/parser/test-pxe-discover-bootfile-relative-conffile \
>>   	test/parser/test-pxe-discover-bootfile-absolute-conffile \
>>   	test/parser/test-pxe-discover-bootfile-async-file \
>> -	test/parser/test-unresolved-remove
>> +	test/parser/test-unresolved-remove \
>> +	test/parser/test-syslinux-single-yocto \
>> +	test/parser/test-syslinux-global-append \
>> +	test/parser/test-syslinux-explicit \
>> +	test/parser/test-syslinux-nested-config
>> +
>>   
>>   TESTS += $(parser_TESTS)
>>   check_PROGRAMS += $(parser_TESTS) test/parser/libtest.ro
>> @@ -82,7 +87,10 @@ check_DATA += \
>>   	test/parser/data/grub2-f18-ppc64.conf \
>>   	test/parser/data/grub2-f20-ppc.conf \
>>   	test/parser/data/grub2-ubuntu-13_04-x86.conf \
>> -	test/parser/data/yaboot-rh8-ppc64.conf
>> +	test/parser/data/yaboot-rh8-ppc64.conf \
>> +	test/parser/data/syslinux-include-root.cfg \
>> +	test/parser/data/syslinux-include-nest-1.cfg \
>> +	test/parser/data/syslinux-include-nest-2.cfg
>>   
>>   $(parser_TESTS): AM_CPPFLAGS += \
>>   		-I$(top_srcdir)/discover \
>> @@ -107,6 +115,7 @@ test_parser_libtest_ro_SOURCES = \
>>   	discover/yaboot-parser.c \
>>   	discover/kboot-parser.c \
>>   	discover/pxe-parser.c \
>> +	discover/syslinux-parser.c \
>>   	discover/platform.c \
>>   	discover/resource.c \
>>   	discover/paths.c \
>> diff --git a/test/parser/data/syslinux-include-nest-1.cfg b/test/parser/data/syslinux-include-nest-1.cfg
>> new file mode 100644
>> index 0000000..65e680e
>> --- /dev/null
>> +++ b/test/parser/data/syslinux-include-nest-1.cfg
>> @@ -0,0 +1,7 @@
>> +LABEL com32
>> +COM32 /boot/com32.c32
>> +
>> +INCLUDE syslinux-include-nest-2.cfg
>> +
>> +LABEL bss
>> +KERNEL /boot/test.bss
>> diff --git a/test/parser/data/syslinux-include-nest-2.cfg b/test/parser/data/syslinux-include-nest-2.cfg
>> new file mode 100644
>> index 0000000..26d7cff
>> --- /dev/null
>> +++ b/test/parser/data/syslinux-include-nest-2.cfg
>> @@ -0,0 +1,6 @@
>> +LABEL boot
>> +KERNEL /bzImage-boot
>> +INITRD /initrd-boot
>> +APPEND root=/dev/sda
>> +
>> +DEFAULT boot
>> diff --git a/test/parser/data/syslinux-include-root.cfg b/test/parser/data/syslinux-include-root.cfg
>> new file mode 100644
>> index 0000000..834360c
>> --- /dev/null
>> +++ b/test/parser/data/syslinux-include-root.cfg
>> @@ -0,0 +1,18 @@
>> +APPEND console=ttyS0
>> +
>> +LABEL floppy
>> +FDIMAGE /boot/floppy.img
>> +
>> +LABEL backup
>> +KERNEL /backup/vmlinuz
>> +APPEND root=/dev/sdb
>> +INITRD /boot/initrd
>> +
>> +LABEL comboot
>> +KERNEL /boot/comboot.com
>> +
>> +INCLUDE /syslinux-include-nest-1.cfg
>> +
>> +LABEL linux
>> +LINUX /boot/bzImage
>> +APPEND root=/dev/sdc
>> diff --git a/test/parser/test-syslinux-explicit.c b/test/parser/test-syslinux-explicit.c
>> new file mode 100644
>> index 0000000..5d23f50
>> --- /dev/null
>> +++ b/test/parser/test-syslinux-explicit.c
>> @@ -0,0 +1,41 @@
>> +/* test a standard yocto syslinux wic cfg */
>> +
>> +#include "parser-test.h"
>> +
>> +#if 0 /* PARSER_EMBEDDED_CONFIG */
>> +
>> +
>> +DEFAULT boot
>> +
>> +KERNEL /vmlinuz
>> +APPEND console=tty0
>> +
>> +LABEL backup
>> +KERNEL /backup/vmlinuz
>> +APPEND root=/dev/sdb
>> +INITRD /boot/initrd
>> +
>> +IMPLICIT 0
>> +
>> +#endif
>> +
>> +void run_test(struct parser_test *test)
>> +{
>> +	struct discover_boot_option *opt;
>> +	struct discover_context *ctx;
>> +
>> +	test_read_conf_embedded(test, "/boot/syslinux/syslinux.cfg");
>> +
>> +	test_run_parser(test, "syslinux");
>> +
>> +	ctx = test->ctx;
>> +
>> +	check_boot_option_count(ctx, 1);
>> +
>> +	opt = get_boot_option(ctx, 0);
>> +
>> +	check_name(opt, "backup");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
>> +	check_args(opt, " root=/dev/sdb");
>> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
>> +}
>> diff --git a/test/parser/test-syslinux-global-append.c b/test/parser/test-syslinux-global-append.c
>> new file mode 100644
>> index 0000000..18af99a
>> --- /dev/null
>> +++ b/test/parser/test-syslinux-global-append.c
>> @@ -0,0 +1,56 @@
>> +
>> +#include "parser-test.h"
>> +
>> +#if 0 /* PARSER_EMBEDDED_CONFIG */
>> +
>> +APPEND console=ttyS0
>> +
>> +LABEL linux
>> +LINUX /vmlinuz
>> +APPEND console=tty0
>> +
>> +LABEL backup
>> +KERNEL /backup/vmlinuz
>> +APPEND root=/dev/sdb
>> +INITRD /boot/initrd
>> +
>> +LABEL hyphen
>> +KERNEL /test/vmlinuz
>> +APPEND -
>> +
>> +#endif
>> +
>> +void run_test(struct parser_test *test)
>> +{
>> +	struct discover_boot_option *opt;
>> +	struct discover_context *ctx;
>> +
>> +	test_read_conf_embedded(test, "/syslinux/syslinux.cfg");
>> +
>> +	test_run_parser(test, "syslinux");
>> +
>> +	ctx = test->ctx;
>> +
>> +	check_boot_option_count(ctx, 3);
>> +	opt = get_boot_option(ctx, 2);
>> +
>> +	check_name(opt, "linux");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/vmlinuz");
>> +	check_is_default(opt);
>> +	check_args(opt, "console=ttyS0 console=tty0");
>> +	check_not_present_resource(opt->initrd);
>> +
>> +	opt = get_boot_option(ctx, 1);
>> +
>> +	check_name(opt, "backup");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
>> +	check_args(opt, "console=ttyS0 root=/dev/sdb");
>> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
>> +
>> +	opt = get_boot_option(ctx, 0);
>> +
>> +	check_name(opt, "hyphen");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/test/vmlinuz");
>> +	check_args(opt, "");
>> +	check_not_present_resource(opt->initrd);
>> +}
>> diff --git a/test/parser/test-syslinux-nested-config.c b/test/parser/test-syslinux-nested-config.c
>> new file mode 100644
>> index 0000000..73c4774
>> --- /dev/null
>> +++ b/test/parser/test-syslinux-nested-config.c
>> @@ -0,0 +1,41 @@
>> +
>> +#include "parser-test.h"
>> +
>> +
>> +void run_test(struct parser_test *test)
>> +{
>> +	struct discover_boot_option *opt;
>> +	struct discover_context *ctx;
>> +
>> +	test_read_conf_file(test, "syslinux-include-root.cfg", "/boot/syslinux/syslinux.cfg");
>> +	test_read_conf_file(test, "syslinux-include-nest-1.cfg", "/syslinux-include-nest-1.cfg");
>> +	test_read_conf_file(test, "syslinux-include-nest-2.cfg", "/boot/syslinux/syslinux-include-nest-2.cfg");
>> +
>> +	test_run_parser(test, "syslinux");
>> +
>> +	ctx = test->ctx;
>> +
>> +	check_boot_option_count(ctx, 3);
>> +
>> +	opt = get_boot_option(ctx, 1);
>> +
>> +	check_name(opt, "boot");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/bzImage-boot");
>> +	check_is_default(opt);
>> +	check_args(opt, "console=ttyS0 root=/dev/sda");
>> +	check_resolved_local_resource(opt->initrd, ctx->device, "/initrd-boot");
>> +
>> +	opt = get_boot_option(ctx, 2);
>> +
>> +	check_name(opt, "backup");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/backup/vmlinuz");
>> +	check_args(opt, "console=ttyS0 root=/dev/sdb");
>> +	check_resolved_local_resource(opt->initrd, ctx->device, "/boot/initrd");
>> +
>> +	opt = get_boot_option(ctx, 0);
>> +
>> +	check_name(opt, "linux");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/boot/bzImage");
>> +	check_args(opt, "console=ttyS0 root=/dev/sdc");
>> +	check_not_present_resource(opt->initrd);
>> +}
>> diff --git a/test/parser/test-syslinux-single-yocto.c b/test/parser/test-syslinux-single-yocto.c
>> new file mode 100644
>> index 0000000..e5e084d
>> --- /dev/null
>> +++ b/test/parser/test-syslinux-single-yocto.c
>> @@ -0,0 +1,36 @@
>> +/* test a standard yocto syslinux wic cfg */
>> +
>> +#include "parser-test.h"
>> +
>> +#if 0 /* PARSER_EMBEDDED_CONFIG */
>> +PROMPT 0
>> +TIMEOUT 0
>> +
>> +ALLOWOPTIONS 1
>> +SERIAL 0 115200
>> +
>> +DEFAULT boot
>> +LABEL boot
>> +KERNEL /vmlinuz
>> +APPEND console=ttyS0,115200n8 console=tty0
>> +#endif
>> +
>> +void run_test(struct parser_test *test)
>> +{
>> +	struct discover_boot_option *opt;
>> +	struct discover_context *ctx;
>> +
>> +	test_read_conf_embedded(test, "/syslinux.cfg");
>> +
>> +	test_run_parser(test, "syslinux");
>> +
>> +	ctx = test->ctx;
>> +
>> +	check_boot_option_count(ctx, 1);
>> +	opt = get_boot_option(ctx, 0);
>> +
>> +	check_name(opt, "boot");
>> +	check_resolved_local_resource(opt->boot_image, ctx->device, "/vmlinuz");
>> +	check_is_default(opt);
>> +	check_args(opt, " console=ttyS0,115200n8 console=tty0");
>> +}