diff mbox series

[v4,1/2] discover: Allow for empty paths

Message ID 20210121184437.5613-1-klaus@linux.vnet.ibm.com
State New
Headers show
Series [v4,1/2] discover: Allow for empty paths | expand

Commit Message

Klaus Heinrich Kiwi Jan. 21, 2021, 6:44 p.m. UTC
Some grub tests involve a (device)/path structure, where it is
actually legal to have an empty path. Adjust join_path() and
dependant functions to allow for empty pathnames.

Also add / modify a few more tests for this scenario.

Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
---
 discover/grub2/builtins.c              |  9 +++++----
 discover/grub2/grub2.c                 | 17 ++++++++++++----
 discover/paths.c                       | 16 ++++++++++-----
 discover/paths.h                       |  6 +++---
 test/parser/test-grub2-devpath.c       | 28 +++++++++++++++++---------
 test/parser/test-grub2-test-file-ops.c | 19 +++++++++++++++++
 test/parser/utils.c                    |  2 +-
 7 files changed, 71 insertions(+), 26 deletions(-)

Comments

Jeremy Kerr Jan. 26, 2021, 3:17 a.m. UTC | #1
Hi Klaus,

> Some grub tests involve a (device)/path structure, where it is
> actually legal to have an empty path. Adjust join_path() and
> dependant functions to allow for empty pathnames.
> 
> Also add / modify a few more tests for this scenario.

Perfect, thanks! I've applied both of these to master.

Cheers,


Jeremy
Klaus Heinrich Kiwi Jan. 26, 2021, 11:33 a.m. UTC | #2
On 1/26/2021 12:17 AM, Jeremy Kerr wrote:
> Hi Klaus,
> 
>> Some grub tests involve a (device)/path structure, where it is
>> actually legal to have an empty path. Adjust join_path() and
>> dependant functions to allow for empty pathnames.
>>
>> Also add / modify a few more tests for this scenario.
> 
> Perfect, thanks! I've applied both of these to master.
> 

Thanks Jeremy!

Cheers,

  -Klaus
diff mbox series

Patch

diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c
index ab1407a..f32e8e5 100644
--- a/discover/grub2/builtins.c
+++ b/discover/grub2/builtins.c
@@ -247,13 +247,13 @@  static bool builtin_test_op_file(struct grub2_script *script, char op,
 	switch (op) {
 	case 's':
 		/* -s: return true if file exists and has non-zero size */
-		result = statbuf.st_size > 0;
+		result = !path ? false : statbuf.st_size > 0;
 		break;
 	case 'f':
 		/* -f: return true if file exists and is not a directory. This is
 		 * different than the behavior of "test", but is what GRUB does
 		 * (though note as above that we follow symlinks unlike GRUB). */
-		result = !S_ISDIR(statbuf.st_mode);
+		result = !path ? false : !S_ISDIR(statbuf.st_mode);
 		break;
 	default:
 		result = false;
@@ -284,7 +284,7 @@  static bool builtin_test_op_dir(struct grub2_script *script, char op,
 	if (rc)
 		return false;
 
-	return S_ISDIR(statbuf.st_mode);
+	return !path ? false : S_ISDIR(statbuf.st_mode);
 }
 
 static bool builtin_test_op(struct grub2_script *script,
@@ -419,7 +419,8 @@  static int builtin_source(struct grub2_script *script,
 		return false;
 
 	rc = parse_to_device_path(script, argv[1], &dev, &path);
-	if (rc)
+	/* We need to have a valid (non-empty) path for sources */
+	if (rc || !path)
 		return false;
 
 	rc = parser_request_file(script->ctx, dev, path, &buf, &len);
diff --git a/discover/grub2/grub2.c b/discover/grub2/grub2.c
index b176ce2..9be94eb 100644
--- a/discover/grub2/grub2.c
+++ b/discover/grub2/grub2.c
@@ -70,7 +70,8 @@  struct resource *create_grub2_resource(struct grub2_script *script,
 	}
 
 	file = grub2_parse_file(script, path);
-	if (!file)
+	/* We need a non-empty path for resources */
+	if (!file || !file->path)
 		return NULL;
 
 	res = talloc(opt, struct resource);
@@ -135,19 +136,27 @@  struct grub2_file *grub2_parse_file(struct grub2_script *script,
 
 		pos = strchr(str, ')');
 
-		/* no closing bracket, or zero-length path? */
-		if (!pos || *(pos+1) == '\0') {
+		/* no closing bracket is not legal */
+		if (!pos) {
 			talloc_free(file);
 			return NULL;
 		}
 
-		file->path = talloc_strdup(file, pos + 1);
+		/* path is non-empty - copy it (otherwise keep it
+		 * NULL as it should be legal) */
+		if (*(pos+1) != '\0')
+			file->path = talloc_strdup(file, pos + 1);
 
 		dev_len = pos - str - 1;
 		if (dev_len)
 			file->dev = talloc_strndup(file, str + 1, dev_len);
 	}
 
+	/* if both dev and path are empty, this is probably an error */
+	if (!file->dev && !file->path) {
+		talloc_free(file);
+		return NULL;
+	}
 	return file;
 }
 
diff --git a/discover/paths.c b/discover/paths.c
index 16fdd59..3c43bf6 100644
--- a/discover/paths.c
+++ b/discover/paths.c
@@ -51,13 +51,19 @@  const char *mount_base(void)
 char *join_paths(void *alloc_ctx, const char *a, const char *b)
 {
 	char *full_path;
+	size_t a_len = a ? strlen(a) : 0;
+	size_t b_len = b ? strlen(b) : 0;
 
-	full_path = talloc_array(alloc_ctx, char, strlen(a) + strlen(b) + 2);
+	full_path = talloc_zero_array(alloc_ctx, char, a_len + b_len + 2);
 
-	strcpy(full_path, a);
-	if (b[0] != '/' && a[strlen(a) - 1] != '/')
-		strcat(full_path, "/");
-	strcat(full_path, b);
+	if (a_len)
+		strcpy(full_path, a);
+
+	if (b_len) {
+		if (a_len && a[a_len - 1] != '/' && b[0] != '/')
+			strcat(full_path, "/");
+		strcat(full_path, b);
+	}
 
 	return full_path;
 }
diff --git a/discover/paths.h b/discover/paths.h
index 67fe8a3..6694877 100644
--- a/discover/paths.h
+++ b/discover/paths.h
@@ -6,10 +6,10 @@ 
 #include <process/process.h>
 
 /**
- * Utility function for joining two paths. Adds a / between a and b if
- * required.
+ * Utility function for joining two paths. Allows either or
+ * both paths to be NULL. Adds a / between a and b if required.
  *
- * Returns a newly-allocated string.
+ * Returns a newly-allocated string (empty if both paths NULL)
  */
 char *join_paths(void *alloc_ctx, const char *a, const char *b);
 
diff --git a/test/parser/test-grub2-devpath.c b/test/parser/test-grub2-devpath.c
index d1d00f1..2571078 100644
--- a/test/parser/test-grub2-devpath.c
+++ b/test/parser/test-grub2-devpath.c
@@ -9,35 +9,40 @@  menuentry a {
 	linux /vmlinux
 }
 
+# local, with an empty device-string
+menuentry b {
+	linux ()/vmlinux
+}
+
 # local, specified by root env var
 root=00000000-0000-0000-0000-000000000001
-menuentry b {
+menuentry c {
 	linux /vmlinux
 }
 
 # remote, specified by root env var
 root=00000000-0000-0000-0000-000000000002
-menuentry c {
+menuentry d {
 	linux /vmlinux
 }
 
 # local, full dev+path spec
-menuentry d {
+menuentry e {
 	linux (00000000-0000-0000-0000-000000000001)/vmlinux
 }
 
 # remote, full dev+path spec
-menuentry e {
+menuentry f {
 	linux (00000000-0000-0000-0000-000000000002)/vmlinux
 }
 
 # invalid: incomplete dev+path spec
-menuentry f {
+menuentry g {
 	linux (00000000-0000-0000-0000-000000000001
 }
 
 # invalid: no path
-menuentry g {
+menuentry h {
 	linux (00000000-0000-0000-0000-000000000001)
 }
 
@@ -64,7 +69,7 @@  void run_test(struct parser_test *test)
 
 	test_run_parser(test, "grub2");
 
-	check_boot_option_count(ctx, 5);
+	check_boot_option_count(ctx, 6);
 
 	opt = get_boot_option(ctx, 0);
 	check_name(opt, "a");
@@ -76,13 +81,18 @@  void run_test(struct parser_test *test)
 
 	opt = get_boot_option(ctx, 2);
 	check_name(opt, "c");
-	check_resolved_local_resource(opt->boot_image, dev2, "/vmlinux");
+	check_resolved_local_resource(opt->boot_image, dev1, "/vmlinux");
 
 	opt = get_boot_option(ctx, 3);
 	check_name(opt, "d");
-	check_resolved_local_resource(opt->boot_image, dev1, "/vmlinux");
+	check_resolved_local_resource(opt->boot_image, dev2, "/vmlinux");
 
 	opt = get_boot_option(ctx, 4);
 	check_name(opt, "e");
+	check_resolved_local_resource(opt->boot_image, dev1, "/vmlinux");
+
+	opt = get_boot_option(ctx, 5);
+	check_name(opt, "f");
 	check_resolved_local_resource(opt->boot_image, dev2, "/vmlinux");
+
 }
diff --git a/test/parser/test-grub2-test-file-ops.c b/test/parser/test-grub2-test-file-ops.c
index b614fc1..38214a1 100644
--- a/test/parser/test-grub2-test-file-ops.c
+++ b/test/parser/test-grub2-test-file-ops.c
@@ -13,6 +13,12 @@  fi
 if [ ! -f /empty_file -a $status = success ]
 then status=fail_f_3
 fi
+if [ -f "" -a $status = success ]
+then status=fail_f_4
+fi
+if [ -f / -a $status = success ]
+then status=fail_f_5
+fi
 
 if [ -s /file_that_does_not_exist -a $status = success ]
 then status=fail_s_1
@@ -26,6 +32,12 @@  fi
 if [ ! -s /non_empty_file -a $status = success ]
 then status=fail_s_4
 fi
+if [ -s "" -a $status = success ]
+then status=fail_s_5
+fi
+if [ ! -s / -a $status = success ]
+then status=fail_s_6
+fi
 
 if [ -d /file_that_does_not_exist -a $status = success ]
 then status=fail_d_1
@@ -36,6 +48,12 @@  fi
 if [ -d /empty_file -a $status = success ]
 then status=fail_d_3
 fi
+if [ -d "" -a $status = success ]
+then status=fail_d_4
+fi
+if [ ! -d / -a $status = success ]
+then status=fail_d_5
+fi
 
 menuentry $status {
   linux /vmlinux
@@ -50,6 +68,7 @@  void run_test(struct parser_test *test)
 	ctx = test->ctx;
 
 	test_read_conf_embedded(test, "/grub2/grub.cfg");
+	test_add_dir(test, ctx->device, "/");
 	test_add_file_data(test, ctx->device, "/empty_file", "", 0);
 	test_add_file_data(test, ctx->device, "/non_empty_file", "1", 1);
 	test_add_dir(test, ctx->device, "/dir");
diff --git a/test/parser/utils.c b/test/parser/utils.c
index d8499a4..2705b9a 100644
--- a/test/parser/utils.c
+++ b/test/parser/utils.c
@@ -257,7 +257,7 @@  int parser_stat_path(struct discover_context *ctx,
 	list_for_each_entry(&test->files, file, list) {
 		if (file->dev != dev)
 			continue;
-		if (strcmp(file->name, path))
+		if (path && strcmp(file->name, path))
 			continue;
 
 		statbuf->st_size = (off_t)file->size;