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

[Klaus Heinrich Kiwi]

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

-- 
Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>