| Message ID | 20210120192658.27930-1-klaus@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] discover: Allow for empty paths | expand |
Hi Klaus, Looks good! Just some minor things: > @@ -130,24 +130,33 @@ struct grub2_file *grub2_parse_file(struct grub2_script *script, > /* just a path - no device, return path as-is */ > file->path = talloc_strdup(file, str); > > - } else { > + } > + else { Not sure we want this change. > /* device plus path - split into components */ > - > 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); The formatting of the if-statements doesn't match the rest of the file, you have spaces within the brackets. Not a huge issue, and I'm happy to tweak this during the merge if you prefer. > - dev_len = pos - str - 1; > - if (dev_len) > - file->dev = talloc_strndup(file, str + 1, dev_len); > + /* same as above for device string */ > + if ( pos > (str+1) ) > + file->dev = talloc_strndup(file, str + 1, > + (size_t) (pos - str - 1)); This change looks unnecessary now that you have the case above, or am I missing something? > > + /* if both dev and path are empty, this is probably an error > */ > + if (!file->dev && !file->path) { > + talloc_free(file); > + return NULL; > + } > return file; > } OK, that looks good now, and seems to correctly handle either an empty path or empty device. The change looks good to me - could you add some tests to cover the empty path cases? With this patch, there are not (yet) any legal cases for an empty path, so we'd want to ensure that the path handling code is correct for those. eg source, resources, and the test builtins. For the case where an empty path becomes legal (ie, with the 'test -e' command), then those particular tests could deferred to 2/2. Cheers, Jeremy
Hi Klaus, > The change looks good to me - could you add some tests to cover the > empty path cases? With this patch, there are not (yet) any legal cases > for an empty path, so we'd want to ensure that the path handling code > is correct for those. eg source, resources, and the test builtins. ... and if you like, I'm happy to help out with adding these tests. I understand that this has grown somewhat from just fixing the RHEL boot config! :) Cheers, Jeremy
Hi Jeremy, On 1/20/2021 10:31 PM, Jeremy Kerr wrote: > Hi Klaus, > > Looks good! Just some minor things: >> - } else { >> + } >> + else { > > Not sure we want this change. fixed >> + * NULL as it should be legal) */ >> + if ( *(pos+1) != '\0' ) >> + file->path = talloc_strdup(file, pos + 1); > > The formatting of the if-statements doesn't match the rest of the > file, you have spaces within the brackets. Not a huge issue, and I'm > happy to tweak this during the merge if you prefer. Fixed as well > >> - dev_len = pos - str - 1; >> - if (dev_len) >> - file->dev = talloc_strndup(file, str + 1, dev_len); >> + /* same as above for device string */ >> + if ( pos > (str+1) ) >> + file->dev = talloc_strndup(file, str + 1, >> + (size_t) (pos - str - 1)); > > This change looks unnecessary now that you have the case above, or am I > missing something? You're right. I was trying to make it clearer, but the code should be equivalent. I'll remove this change. >> >> + /* if both dev and path are empty, this is probably an error >> */ >> + if (!file->dev && !file->path) { >> + talloc_free(file); >> + return NULL; >> + } >> return file; >> } > > OK, that looks good now, and seems to correctly handle either an empty > path or empty device. > > The change looks good to me - could you add some tests to cover the > empty path cases? With this patch, there are not (yet) any legal cases > for an empty path, so we'd want to ensure that the path handling code > is correct for those. eg source, resources, and the test builtins. I've added one test to test-grub2-devpath.c that checks for an empty device string 'linux ()/vmlinux' but I'm not sure how we could test for an empty path without something like 'test -e (device)'. resources without a path such as menuentry a { linux (device) } are already covered under the same test-grub2-devpath.c and I'm not sure it's useful to add empty path tests for 'test -f', 'test -s' or 'test -d' since an empty path will always be replaced by '/' in those cases. But I'll look into it and send a v4 soon. > For the case where an empty path becomes legal (ie, with the 'test -e' > command), then those particular tests could deferred to 2/2. Will try to add them as part of v4 too. > Cheers, > > > Jeremy > -Klaus
diff --git a/discover/grub2/builtins.c b/discover/grub2/builtins.c index ab1407a..4b94f99 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,9 @@ 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..7fb6f62 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); @@ -118,7 +119,6 @@ struct grub2_file *grub2_parse_file(struct grub2_script *script, const char *str) { struct grub2_file *file; - size_t dev_len; char *pos; if (!str) @@ -130,24 +130,33 @@ struct grub2_file *grub2_parse_file(struct grub2_script *script, /* just a path - no device, return path as-is */ file->path = talloc_strdup(file, str); - } else { + } + else { /* device plus path - split into components */ - 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); + /* same as above for device string */ + if ( pos > (str+1) ) + file->dev = talloc_strndup(file, str + 1, + (size_t) (pos - str - 1)); } + /* 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/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;
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. Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> --- discover/grub2/builtins.c | 10 ++++++---- discover/grub2/grub2.c | 29 +++++++++++++++++++---------- discover/paths.c | 16 +++++++++++----- discover/paths.h | 6 +++--- test/parser/test-grub2-devpath.c | 28 +++++++++++++++++++--------- test/parser/utils.c | 2 +- 6 files changed, 59 insertions(+), 32 deletions(-)