discover/yaboot: fix device parsing

A couple of fixes for yaboot's device-handling code. Firstly, we need to
use 'device=' rather than 'root=', as the latter is purely for ybin, to
define where the yaboot binary goes.

Secondly, we need to respect global and option-specific device=
parameters. To do this, we keep all boot_image and initrd strings in the
state, and create the actual resources in yaboot_finish.

Add a test for all override cases, and fix the incorrect boot= parsing
in the rh8 test.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

4 files changed, 154 insertions, 76 deletions

diff --git a/discover/yaboot-parser.c b/discover/yaboot-parser.c
index f2f34c1..81ab83d 100644
--- a/discover/yaboot-parser.c
+++ b/discover/yaboot-parser.c
@@ -12,13 +12,15 @@
 #include "resource.h"
 
 struct yaboot_state {
-	char *desc_image;
-	char *desc_initrd;
 	int globals_done;
 	const char *const *known_names;
 
 	/* current option data */
 	struct discover_boot_option *opt;
+	const char *device;
+	const char *partition;
+	const char *boot_image;
+	const char *initrd;
 	const char *initrd_size;
 	const char *literal;
 	const char *ramdisk;
@@ -31,12 +33,12 @@ static struct discover_boot_option *state_start_new_option(
 		struct conf_context *conf,
 		struct yaboot_state *state)
 {
-	state->desc_initrd = NULL;
-
 	state->opt = discover_boot_option_create(conf->dc, conf->dc->device);
 	state->opt->option->boot_args = talloc_strdup(state->opt->option, "");
 
 	/* old allocated values will get freed with the state */
+	state->device = conf_get_global_option(conf, "device");
+	state->partition = conf_get_global_option(conf, "partition");
 	state->initrd_size = conf_get_global_option(conf, "initrd_size");
 	state->literal = conf_get_global_option(conf, "literal");
 	state->ramdisk = conf_get_global_option(conf, "ramdisk");
@@ -45,17 +47,48 @@ static struct discover_boot_option *state_start_new_option(
 	return state->opt;
 }
 
+static struct resource *create_yaboot_devpath_resource(
+		struct yaboot_state *state,
+		struct conf_context *conf,
+		const char *path)
+{
+	struct discover_boot_option *opt = state->opt;
+	const char *dev, *part;
+	struct resource *res;
+	char *devpath;
+
+	dev = state->device;
+	part = state->partition;
+
+	if (!dev)
+		dev = conf_get_global_option(conf, "device");
+	if (!part)
+		part = conf_get_global_option(conf, "partition");
+
+	if (strchr(path, ':')) {
+		devpath = talloc_strdup(conf, path);
+
+	} else if (dev && part) {
+		devpath = talloc_asprintf(conf,
+				"%s%s:%s", dev, part, path);
+	} else if (dev) {
+		devpath = talloc_asprintf(conf, "%s:%s", dev, path);
+	} else {
+		devpath = talloc_strdup(conf, path);
+	}
+
+	res = create_devpath_resource(opt, conf->dc->device, devpath);
+
+	talloc_free(devpath);
+
+	return res;
+}
+
 static void yaboot_finish(struct conf_context *conf)
 {
 	struct yaboot_state *state = conf->parser_info;
-	struct device *dev = conf->dc->device->device;
 	struct boot_option *opt;
 
-	if (!state->desc_image) {
-		pb_log("%s: %s: no image found\n", __func__, dev->id);
-		return;
-	}
-
 	assert(state->opt);
 
 	opt = state->opt->option;
@@ -64,6 +97,13 @@ static void yaboot_finish(struct conf_context *conf)
 	assert(opt->boot_args);
 
 	/* populate the boot option from state data */
+	state->opt->boot_image = create_yaboot_devpath_resource(state,
+				conf, state->boot_image);
+	if (state->initrd) {
+		state->opt->initrd = create_yaboot_devpath_resource(state,
+				conf, state->initrd);
+	}
+
 	if (state->initrd_size) {
 		opt->boot_args = talloc_asprintf(opt, "ramdisk_size=%s %s",
 					state->initrd_size, opt->boot_args);
@@ -96,48 +136,16 @@ static void yaboot_finish(struct conf_context *conf)
 	}
 
 	opt->description = talloc_asprintf(opt, "%s %s %s",
-		state->desc_image,
-		(state->desc_initrd ? state->desc_initrd : ""),
+		state->boot_image,
+		(state->initrd ? state->initrd : ""),
 		opt->boot_args ? opt->boot_args : "");
 
-	talloc_free(state->desc_initrd);
-
 	conf_strip_str(opt->boot_args);
 	conf_strip_str(opt->description);
 
 	discover_context_add_boot_option(conf->dc, state->opt);
 }
 
-static struct resource *create_yaboot_devpath_resource(
-		struct discover_boot_option *opt,
-		struct conf_context *conf,
-		const char *path, char **desc_str)
-{
-	const char *g_boot = conf_get_global_option(conf, "boot");
-	const char *g_part = conf_get_global_option(conf, "partition");
-	struct resource *res;
-	char *devpath;
-
-	if (g_boot && g_part) {
-		devpath = talloc_asprintf(conf,
-				"%s%s:%s", g_boot, g_part, path);
-	} else if (g_boot) {
-		devpath = talloc_asprintf(conf, "%s:%s", g_boot, path);
-	} else {
-		devpath = talloc_strdup(conf, path);
-	}
-
-	res = create_devpath_resource(opt, conf->dc->device, devpath);
-
-	if (desc_str)
-		*desc_str = devpath;
-	else
-		talloc_free(devpath);
-
-	return res;
-}
-
-
 static void yaboot_process_pair(struct conf_context *conf, const char *name,
 		char *value)
 {
@@ -181,8 +189,7 @@ static void yaboot_process_pair(struct conf_context *conf, const char *name,
 		/* Then start the new image. */
 		opt = state_start_new_option(conf, state);
 
-		opt->boot_image = create_yaboot_devpath_resource(opt,
-				conf, value, &state->desc_image);
+		state->boot_image = talloc_strdup(state, value);
 
 		return;
 	}
@@ -205,21 +212,11 @@ static void yaboot_process_pair(struct conf_context *conf, const char *name,
 		opt = state_start_new_option(conf, state);
 
 		if (*value == '/') {
-			opt->boot_image = create_yaboot_devpath_resource(opt,
-					conf, value, &state->desc_image);
+			state->boot_image = talloc_strdup(state, value);
 		} else {
-			char *tmp;
-
-			opt->boot_image = create_yaboot_devpath_resource(opt,
-					conf, suse_fp->image,
-					&state->desc_image);
-
-			opt->initrd = create_yaboot_devpath_resource(opt,
-					conf, suse_fp->initrd, &tmp);
-
-			state->desc_initrd = talloc_asprintf(opt,
-				"initrd=%s", tmp);
-			talloc_free(tmp);
+			state->boot_image = talloc_strdup(state,
+							suse_fp->image);
+			state->initrd = talloc_strdup(state, suse_fp->initrd);
 		}
 
 		return;
@@ -232,16 +229,12 @@ static void yaboot_process_pair(struct conf_context *conf, const char *name,
 	}
 
 	/* initrd */
-
 	if (streq(name, "initrd")) {
-		opt->initrd = create_yaboot_devpath_resource(opt, conf,
-				value, &state->desc_image);
-
+		state->initrd = talloc_strdup(state, value);
 		return;
 	}
 
 	/* label */
-
 	if (streq(name, "label")) {
 		opt->option->id = talloc_asprintf(opt->option, "%s#%s",
 			conf->dc->device->device->id, value);
@@ -250,6 +243,16 @@ static void yaboot_process_pair(struct conf_context *conf, const char *name,
 	}
 
 	/* args */
+	if (streq(name, "device")) {
+		printf("option device : %s", value);
+		state->device = talloc_strdup(state, value);
+		return;
+	}
+
+	if (streq(name, "parititon")) {
+		state->partition = talloc_strdup(state, value);
+		return;
+	}
 
 	if (streq(name, "append")) {
 		opt->option->boot_args = talloc_asprintf_append(
@@ -292,10 +295,10 @@ static void yaboot_process_pair(struct conf_context *conf, const char *name,
 
 static struct conf_global_option yaboot_global_options[] = {
 	{ .name = "root" },
-	{ .name = "boot" },
+	{ .name = "device" },
+	{ .name = "partition" },
 	{ .name = "initrd" },
 	{ .name = "initrd_size" },
-	{ .name = "partition" },
 	{ .name = "video" },
 	{ .name = "literal" },
 	{ .name = "ramdisk" },
@@ -329,6 +332,8 @@ static const char *yaboot_known_names[] = {
 	"read-only",
 	"read-write",
 	"root",
+	"device",
+	"partition",
 	NULL
 };
 
diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
index 4319705..3c6d73b 100644
--- a/test/parser/Makefile.am
+++ b/test/parser/Makefile.am
@@ -36,6 +36,7 @@ TESTS = \
 	test-yaboot-external \
 	test-yaboot-root-global \
 	test-yaboot-root-override \
+	test-yaboot-device-override \
 	test-yaboot-rh8-ppc64 \
 	test-pxe-single \
 	test-pxe-initrd-in-append
diff --git a/test/parser/test-yaboot-device-override.c b/test/parser/test-yaboot-device-override.c
new file mode 100644
index 0000000..5db5788
--- /dev/null
+++ b/test/parser/test-yaboot-device-override.c
@@ -0,0 +1,79 @@
+#include "parser-test.h"
+
+#include <talloc/talloc.h>
+
+#if 0 /* PARSER_EMBEDDED_CONFIG */
+default=
+device=/dev/sda1
+
+image=/vmlinux.1
+	label=linux.1
+	initrd=initrd.1
+
+image=/vmlinux.2
+	device=/dev/sda2
+	label=linux.2
+	initrd=initrd.2
+
+image=sda3:/vmlinux.3
+	device=/dev/sda2
+	label=linux.3
+	initrd=sda3:initrd.3
+
+image=sda4:/vmlinux.4
+	device=/dev/sda3
+	label=linux.4
+	initrd=initrd.4
+#endif
+
+void run_test(struct parser_test *test)
+{
+	struct discover_boot_option *opt[4];
+	struct discover_device *dev[4];
+	struct discover_context *ctx;
+	char *devname;
+	int i;
+
+	test_read_conf_embedded(test);
+	test_run_parser(test, "yaboot");
+
+	ctx = test->ctx;
+
+	check_boot_option_count(ctx, 4);
+
+	for (i = 0; i < 4; i++)
+		opt[i] = get_boot_option(ctx, i);
+
+	check_name(opt[0], "linux.1");
+	check_unresolved_resource(opt[0]->boot_image);
+	check_unresolved_resource(opt[0]->initrd);
+
+	check_name(opt[1], "linux.2");
+	check_unresolved_resource(opt[1]->boot_image);
+	check_unresolved_resource(opt[1]->initrd);
+
+	check_name(opt[2], "linux.3");
+	check_unresolved_resource(opt[2]->boot_image);
+	check_unresolved_resource(opt[2]->initrd);
+
+	check_name(opt[3], "linux.4");
+	check_unresolved_resource(opt[3]->boot_image);
+	check_unresolved_resource(opt[3]->initrd);
+
+	/* hotplug all dependent devices */
+	for (i = 0; i < 4; i++) {
+		devname = talloc_asprintf(test, "sda%d", i + 1);
+		dev[i] = test_create_device(ctx, devname);
+		test_hotplug_device(test, dev[i]);
+	}
+
+	check_resolved_local_resource(opt[0]->boot_image, dev[0], "/vmlinux.1");
+	check_resolved_local_resource(opt[1]->boot_image, dev[1], "/vmlinux.2");
+	check_resolved_local_resource(opt[2]->boot_image, dev[2], "/vmlinux.3");
+	check_resolved_local_resource(opt[3]->boot_image, dev[3], "/vmlinux.4");
+
+	check_resolved_local_resource(opt[0]->initrd, dev[0], "/initrd.1");
+	check_resolved_local_resource(opt[1]->initrd, dev[1], "/initrd.2");
+	check_resolved_local_resource(opt[2]->initrd, dev[2], "/initrd.3");
+	check_resolved_local_resource(opt[3]->initrd, dev[2], "/initrd.4");
+}
diff --git a/test/parser/test-yaboot-rh8-ppc64.c b/test/parser/test-yaboot-rh8-ppc64.c
index 97d0162..8966cfd 100644
--- a/test/parser/test-yaboot-rh8-ppc64.c
+++ b/test/parser/test-yaboot-rh8-ppc64.c
@@ -5,7 +5,6 @@ void run_test(struct parser_test *test)
 {
 	struct discover_boot_option *opt;
 	struct discover_context *ctx;
-	struct discover_device *dev;
 
 	test_read_conf_file(test, "yaboot-rh8-ppc64.conf");
 	test_run_parser(test, "yaboot");
@@ -16,15 +15,9 @@ void run_test(struct parser_test *test)
 
 	opt = get_boot_option(ctx, 0);
 
-	check_unresolved_resource(opt->boot_image);
-	check_unresolved_resource(opt->initrd);
-
-	dev = test_create_device(ctx, "sdb1");
-	test_hotplug_device(test, dev);
-
-	check_resolved_local_resource(opt->boot_image, dev,
+	check_resolved_local_resource(opt->boot_image, test->ctx->device,
 			"/boot/vmlinuz-1.0-20121219-1");
-	check_resolved_local_resource(opt->initrd, dev,
+	check_resolved_local_resource(opt->initrd, test->ctx->device,
 			"/boot/initrd-1.0-20121219-1.img");
 
 	check_args(opt, "root=/dev/sdb2 root=/dev/sdb2 ro crashkernel=auto "