[PATCH 1/2] discover: Allow for empty paths

[Jeremy Kerr]

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.

Makes sense! Some comments on the implementation though.

In general, we do need to decide whether this situation gives a NULL
value for ->path, or an empty string for ->path. You've gone for the
former here (which I think is the correct choice), but there are are a
lot of assumptions that ->path will be non-null.

We will need to change the cases that assume ->path is non-null -
currently, there are cases where this will result in a NULL 'b'
argument to join_paths, which tries to strlen(b), and crashes.

--- a/discover/grub2/builtins.c
+++ b/discover/grub2/builtins.c
@@ -216,7 +216,7 @@ static int parse_to_device_path(struct grub2_script
*script,
                return -1;
 
        *devp = dev;
-       *pathp = talloc_strdup(script, file->path);
+       *pathp = !file->path ? NULL : talloc_strdup(script, file-
>path);

talloc_strdup() does the exactly this check, you don't need this
change.

diff --git a/discover/grub2/grub2.c b/discover/grub2/grub2.c
index b176ce2..52c75e9 100644
--- a/discover/grub2/grub2.c
+++ b/discover/grub2/grub2.c
@@ -118,7 +118,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)
@@ -129,6 +128,7 @@ struct grub2_file *grub2_parse_file(struct
grub2_script *script,
        if (*str != '(') {
                /* just a path - no device, return path as-is */
                file->path = talloc_strdup(file, str);
+               file->dev = NULL;

This is already covered by the talloc_zero.

 
        } else {
                /* device plus path - split into components */
@@ -137,17 +137,18 @@ struct grub2_file *grub2_parse_file(struct
grub2_script *script,
 
                /* no closing bracket, or zero-length path? */
                if (!pos || *(pos+1) == '\0') {
-                       talloc_free(file);
-                       return NULL;
+                       file->path = NULL;
+               }
+               else {
+                       file->path = talloc_strdup(file, pos + 1);
+                       file->dev = talloc_strndup(file, str + 1,
(size_t) (pos - str - 1));
                }

You probably want to separate the missing-closing-bracket case from the
zero-length-path case here, since you're making the latter a valid
pathspec.

Regardless, in the zero-length path case, -> path is set to NULL, ->dev
is not set either, so this function still returns NULL. Are you sure
that this fixes the issue in the way that you expect?

Also, super minor, but: curly brackets and else on the same line, and
the last addition there is > 80 chars.

-
-               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 (!file->dev && !file->path) {
+               talloc_free(file);
+               return NULL;
+       }

You've dropped the case where dev_len is zero here - ie., brackets are
present but empty. Then, since file->dev is a valid pointer to an empty
string, the pathspec "()" won't hit that last conditional.

That's not necessarily an issue, but we'd need to be aware of the
change.

Cheers,


Jeremy