[U-Boot] [PATCH 1/1] efi_loader: do not add \ in efi_fs_from_name()

[U-Boot] [PATCH 1/1] efi_loader: do not add \ in efi_fs_from_name()

[Heinrich Schuchardt]

In the `efidebug boot add` command we do not want a unsollicitated leading
backslash added to the file name.

There is no good reason to mark a loaded file with a backslash as absolute.
Anyway when reading files the file name will be interpreted as relative to
root directory of the device.

So let's get rid of this backslash.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_device_path.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 98c36e798f5..53b40c8c3c2 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -985,12 +985,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
 	if (!path)
 		return EFI_SUCCESS;
 
-	if (!is_net) {
-		/* Add leading / to fs paths, because they're absolute */
-		snprintf(filename, sizeof(filename), "/%s", path);
-	} else {
-		snprintf(filename, sizeof(filename), "%s", path);
-	}
+	snprintf(filename, sizeof(filename), "%s", path);
 	/* DOS style file path: */
 	s = filename;
 	while ((s = strchr(s, '/')))
-- 
2.20.1

[U-Boot] [PATCH 1/1] efi_loader: do not add \ in efi_fs_from_name()

[Alexander Graf]

> Am 23.02.2019 um 02:20 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
> 
> In the `efidebug boot add` command we do not want a unsollicitated leading
> backslash added to the file name.
> 
> There is no good reason to mark a loaded file with a backslash as absolute.
> Anyway when reading files the file name will be interpreted as relative to
> root directory of the device.
> 
> So let's get rid of this backslash.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> lib/efi_loader/efi_device_path.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 98c36e798f5..53b40c8c3c2 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -985,12 +985,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>    if (!path)
>        return EFI_SUCCESS;
> 
> -    if (!is_net) {
> -        /* Add leading / to fs paths, because they're absolute */
> -        snprintf(filename, sizeof(filename), "/%s", path);
> -    } else {
> -        snprintf(filename, sizeof(filename), "%s", path);
> -    }
> +    snprintf(filename, sizeof(filename), "%s", path);

I'm pretty sure that code was there for a reason. Without, grub would not find its config file iirc.

Alex

>    /* DOS style file path: */
>    s = filename;
>    while ((s = strchr(s, '/')))
> -- 
> 2.20.1
> 

[U-Boot] [PATCH 1/1] efi_loader: do not add \ in efi_fs_from_name()

[Heinrich Schuchardt]

On 2/23/19 2:37 PM, Alexander Graf wrote:
> 
> 
>> Am 23.02.2019 um 02:20 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
>>
>> In the `efidebug boot add` command we do not want a unsollicitated leading
>> backslash added to the file name.
>>
>> There is no good reason to mark a loaded file with a backslash as absolute.
>> Anyway when reading files the file name will be interpreted as relative to
>> root directory of the device.
>>
>> So let's get rid of this backslash.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> lib/efi_loader/efi_device_path.c | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>> index 98c36e798f5..53b40c8c3c2 100644
>> --- a/lib/efi_loader/efi_device_path.c
>> +++ b/lib/efi_loader/efi_device_path.c
>> @@ -985,12 +985,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>>    if (!path)
>>        return EFI_SUCCESS;
>>
>> -    if (!is_net) {
>> -        /* Add leading / to fs paths, because they're absolute */
>> -        snprintf(filename, sizeof(filename), "/%s", path);
>> -    } else {
>> -        snprintf(filename, sizeof(filename), "%s", path);
>> -    }
>> +    snprintf(filename, sizeof(filename), "%s", path);
> 
> I'm pretty sure that code was there for a reason. Without, grub would not find its config file iirc.
> 
> Alex

Alex, you introduced the logic in 492716662fbdc08e254dda2c209b320e2bf6c837.

Typically the grubaa64.efi file is on an EFI partition while the rest of
GRUB is on the boot partition which could be ext2. In which way would it
make a difference to GRUB whether the path to grubaa64.efi is absolute
or relative? The next thing it will load is anyway on a different path.

On my Odroid C2 I built U-Boot with the patch and executed

=> load mmc 0:1 ${fdt_addr_r} dtb
22925 bytes read in 5 ms (4.4 MiB/s)
=> load mmc 0:4 ${kernel_addr_r} EFI/debian/grubaa64.efi
139264 bytes read in 8 ms (16.6 MiB/s)
=> bootefi ${kernel_addr_r} ${fdt_addr_r}

And the system booted normally. grub/ is on mmc 0:1 so on a different
partition of the same deive.

In grubaa64.efi I found a string (,msdos1)/grub. Wouldn't this be the
path GRUB is using on the boot device?

Can you provide any evidence that GRUB is actually using anything else
than the device part of the device path?

Best regards

Heinrich


> 
>>    /* DOS style file path: */
>>    s = filename;
>>    while ((s = strchr(s, '/')))
>> -- 
>> 2.20.1
>>
> 
> 

[U-Boot] [PATCH 1/1] efi_loader: do not add \ in efi_fs_from_name()

[Alexander Graf]

On 23.02.19 18:27, Heinrich Schuchardt wrote:
> On 2/23/19 2:37 PM, Alexander Graf wrote:
>>
>>
>>> Am 23.02.2019 um 02:20 schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>:
>>>
>>> In the `efidebug boot add` command we do not want a unsollicitated leading
>>> backslash added to the file name.
>>>
>>> There is no good reason to mark a loaded file with a backslash as absolute.
>>> Anyway when reading files the file name will be interpreted as relative to
>>> root directory of the device.
>>>
>>> So let's get rid of this backslash.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>> lib/efi_loader/efi_device_path.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
>>> index 98c36e798f5..53b40c8c3c2 100644
>>> --- a/lib/efi_loader/efi_device_path.c
>>> +++ b/lib/efi_loader/efi_device_path.c
>>> @@ -985,12 +985,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>>>    if (!path)
>>>        return EFI_SUCCESS;
>>>
>>> -    if (!is_net) {
>>> -        /* Add leading / to fs paths, because they're absolute */
>>> -        snprintf(filename, sizeof(filename), "/%s", path);
>>> -    } else {
>>> -        snprintf(filename, sizeof(filename), "%s", path);
>>> -    }
>>> +    snprintf(filename, sizeof(filename), "%s", path);
>>
>> I'm pretty sure that code was there for a reason. Without, grub would not find its config file iirc.
>>
>> Alex
> 
> Alex, you introduced the logic in 492716662fbdc08e254dda2c209b320e2bf6c837.
> 
> Typically the grubaa64.efi file is on an EFI partition while the rest of
> GRUB is on the boot partition which could be ext2. In which way would it
> make a difference to GRUB whether the path to grubaa64.efi is absolute
> or relative? The next thing it will load is anyway on a different path.
> 
> On my Odroid C2 I built U-Boot with the patch and executed
> 
> => load mmc 0:1 ${fdt_addr_r} dtb
> 22925 bytes read in 5 ms (4.4 MiB/s)
> => load mmc 0:4 ${kernel_addr_r} EFI/debian/grubaa64.efi
> 139264 bytes read in 8 ms (16.6 MiB/s)
> => bootefi ${kernel_addr_r} ${fdt_addr_r}
> 
> And the system booted normally. grub/ is on mmc 0:1 so on a different
> partition of the same deive.
> 
> In grubaa64.efi I found a string (,msdos1)/grub. Wouldn't this be the
> path GRUB is using on the boot device?
> 
> Can you provide any evidence that GRUB is actually using anything else
> than the device part of the device path?

I can no longer reproduce the problem that York reported. So yes, I'm ok
with simplifying the logic. But beware that there *might* be something
lurking we just don't remember and that eventually there could be a bug
report.


Alex