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

[Jeremy Kerr]

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