[PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Rasmus Villemoes via Grub-devel]

Unlike files accessed via a normal file system, the file->read_hook is
not honoured when using blocklist notation.

This means that when trying to use a dedicated, 1KiB, raw partition
for the environment block and hence does something like

  save_env --file=(hd0,gpt9)0+2 X Y Z

this fails with "sparse file not allowed", which is rather unexpected,
as I've explicitly said exactly which blocks should be used. Adding a
little debugging reveals that grub_file_size (file) is 1024 as
expected, but total_length is 0, simply because the callback was never
invoked, so blocklists is an empty list.

Fix that by honouring the ->read_hook set by the caller, also when a
"file" is specified with blocklist notation.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
I've tried to keep the patch at a minimum, but since the
disk->read_hook needs to be cleared on all return paths, it seemed a
little easier to drop the 'return -1' and instead set ret and break.

For readability, it seemed best to introduce the disk local variable to
hold file->device->disk, and then there's no point in not also using
that in the grub_disk_read() call.

 grub-core/kern/fs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
index 7ad0aaf4e..79f1a797c 100644
--- a/grub-core/kern/fs.c
+++ b/grub-core/kern/fs.c
@@ -215,12 +215,15 @@ grub_fs_blocklist_read (grub_file_t file, char *buf, grub_size_t len)
   grub_disk_addr_t sector;
   grub_off_t offset;
   grub_ssize_t ret = 0;
+  grub_disk_t disk = file->device->disk;
 
   if (len > file->size - file->offset)
     len = file->size - file->offset;
 
   sector = (file->offset >> GRUB_DISK_SECTOR_BITS);
   offset = (file->offset & (GRUB_DISK_SECTOR_SIZE - 1));
+  disk->read_hook = file->read_hook;
+  disk->read_hook_data = file->read_hook_data;
   for (p = file->data; p->length && len > 0; p++)
     {
       if (sector < p->length)
@@ -232,9 +235,12 @@ grub_fs_blocklist_read (grub_file_t file, char *buf, grub_size_t len)
 	       >> GRUB_DISK_SECTOR_BITS) > p->length - sector)
 	    size = ((p->length - sector) << GRUB_DISK_SECTOR_BITS) - offset;
 
-	  if (grub_disk_read (file->device->disk, p->offset + sector, offset,
+	  if (grub_disk_read (disk, p->offset + sector, offset,
 			      size, buf) != GRUB_ERR_NONE)
-	    return -1;
+	    {
+	      ret = -1;
+	      break;
+	    }
 
 	  ret += size;
 	  len -= size;
@@ -244,6 +250,7 @@ grub_fs_blocklist_read (grub_file_t file, char *buf, grub_size_t len)
       else
 	sector -= p->length;
     }
+  disk->read_hook = 0;
 
   return ret;
 }
-- 
2.46.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Rasmus Villemoes via Grub-devel]

Rasmus Villemoes <rasmus.villemoes@prevas.dk> writes:

> Unlike files accessed via a normal file system, the file->read_hook is
> not honoured when using blocklist notation.
>
> This means that when trying to use a dedicated, 1KiB, raw partition
> for the environment block and hence does something like
>
>   save_env --file=(hd0,gpt9)0+2 X Y Z
>
> this fails with "sparse file not allowed", which is rather unexpected,
> as I've explicitly said exactly which blocks should be used. Adding a
> little debugging reveals that grub_file_size (file) is 1024 as
> expected, but total_length is 0, simply because the callback was never
> invoked, so blocklists is an empty list.
>
> Fix that by honouring the ->read_hook set by the caller, also when a
> "file" is specified with blocklist notation.

Gentle ping.

This is my first patch to grub so please advise if I have done anything
wrong, including if I should/should not explicitly cc maintainers and/or
if I should just have more patience :)

Rasmus

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 3243 bytes --]

Reviewed-by: phcoder@gmail.com

Le jeu. 29 août 2024, 14:07, Rasmus Villemoes via Grub-devel <
grub-devel@gnu.org> a écrit :

> Unlike files accessed via a normal file system, the file->read_hook is
> not honoured when using blocklist notation.
>
> This means that when trying to use a dedicated, 1KiB, raw partition
> for the environment block and hence does something like
>
>   save_env --file=(hd0,gpt9)0+2 X Y Z
>
> this fails with "sparse file not allowed", which is rather unexpected,
> as I've explicitly said exactly which blocks should be used. Adding a
> little debugging reveals that grub_file_size (file) is 1024 as
> expected, but total_length is 0, simply because the callback was never
> invoked, so blocklists is an empty list.
>
> Fix that by honouring the ->read_hook set by the caller, also when a
> "file" is specified with blocklist notation.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> I've tried to keep the patch at a minimum, but since the
> disk->read_hook needs to be cleared on all return paths, it seemed a
> little easier to drop the 'return -1' and instead set ret and break.
>
> For readability, it seemed best to introduce the disk local variable to
> hold file->device->disk, and then there's no point in not also using
> that in the grub_disk_read() call.
>
>  grub-core/kern/fs.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
> index 7ad0aaf4e..79f1a797c 100644
> --- a/grub-core/kern/fs.c
> +++ b/grub-core/kern/fs.c
> @@ -215,12 +215,15 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
> grub_size_t len)
>    grub_disk_addr_t sector;
>    grub_off_t offset;
>    grub_ssize_t ret = 0;
> +  grub_disk_t disk = file->device->disk;
>
>    if (len > file->size - file->offset)
>      len = file->size - file->offset;
>
>    sector = (file->offset >> GRUB_DISK_SECTOR_BITS);
>    offset = (file->offset & (GRUB_DISK_SECTOR_SIZE - 1));
> +  disk->read_hook = file->read_hook;
> +  disk->read_hook_data = file->read_hook_data;
>    for (p = file->data; p->length && len > 0; p++)
>      {
>        if (sector < p->length)
> @@ -232,9 +235,12 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
> grub_size_t len)
>                >> GRUB_DISK_SECTOR_BITS) > p->length - sector)
>             size = ((p->length - sector) << GRUB_DISK_SECTOR_BITS) -
> offset;
>
> -         if (grub_disk_read (file->device->disk, p->offset + sector,
> offset,
> +         if (grub_disk_read (disk, p->offset + sector, offset,
>                               size, buf) != GRUB_ERR_NONE)
> -           return -1;
> +           {
> +             ret = -1;
> +             break;
> +           }
>
>           ret += size;
>           len -= size;
> @@ -244,6 +250,7 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
> grub_size_t len)
>        else
>         sector -= p->length;
>      }
> +  disk->read_hook = 0;
>
>    return ret;
>  }
> --
> 2.46.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 4257 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Rasmus Villemoes via Grub-devel]

"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> writes:

> Reviewed-by: phcoder@gmail.com
>

Thanks. Can this be picked up, please?

Rasmus


> Le jeu. 29 août 2024, 14:07, Rasmus Villemoes via Grub-devel <
> grub-devel@gnu.org> a écrit :
>
>> Unlike files accessed via a normal file system, the file->read_hook is
>> not honoured when using blocklist notation.
>>
>> This means that when trying to use a dedicated, 1KiB, raw partition
>> for the environment block and hence does something like
>>
>>   save_env --file=(hd0,gpt9)0+2 X Y Z
>>
>> this fails with "sparse file not allowed", which is rather unexpected,
>> as I've explicitly said exactly which blocks should be used. Adding a
>> little debugging reveals that grub_file_size (file) is 1024 as
>> expected, but total_length is 0, simply because the callback was never
>> invoked, so blocklists is an empty list.
>>
>> Fix that by honouring the ->read_hook set by the caller, also when a
>> "file" is specified with blocklist notation.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>> I've tried to keep the patch at a minimum, but since the
>> disk->read_hook needs to be cleared on all return paths, it seemed a
>> little easier to drop the 'return -1' and instead set ret and break.
>>
>> For readability, it seemed best to introduce the disk local variable to
>> hold file->device->disk, and then there's no point in not also using
>> that in the grub_disk_read() call.
>>
>>  grub-core/kern/fs.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
>> index 7ad0aaf4e..79f1a797c 100644
>> --- a/grub-core/kern/fs.c
>> +++ b/grub-core/kern/fs.c
>> @@ -215,12 +215,15 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
>> grub_size_t len)
>>    grub_disk_addr_t sector;
>>    grub_off_t offset;
>>    grub_ssize_t ret = 0;
>> +  grub_disk_t disk = file->device->disk;
>>
>>    if (len > file->size - file->offset)
>>      len = file->size - file->offset;
>>
>>    sector = (file->offset >> GRUB_DISK_SECTOR_BITS);
>>    offset = (file->offset & (GRUB_DISK_SECTOR_SIZE - 1));
>> +  disk->read_hook = file->read_hook;
>> +  disk->read_hook_data = file->read_hook_data;
>>    for (p = file->data; p->length && len > 0; p++)
>>      {
>>        if (sector < p->length)
>> @@ -232,9 +235,12 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
>> grub_size_t len)
>>                >> GRUB_DISK_SECTOR_BITS) > p->length - sector)
>>             size = ((p->length - sector) << GRUB_DISK_SECTOR_BITS) -
>> offset;
>>
>> -         if (grub_disk_read (file->device->disk, p->offset + sector,
>> offset,
>> +         if (grub_disk_read (disk, p->offset + sector, offset,
>>                               size, buf) != GRUB_ERR_NONE)
>> -           return -1;
>> +           {
>> +             ret = -1;
>> +             break;
>> +           }
>>
>>           ret += size;
>>           len -= size;
>> @@ -244,6 +250,7 @@ grub_fs_blocklist_read (grub_file_t file, char *buf,
>> grub_size_t len)
>>        else
>>         sector -= p->length;
>>      }
>> +  disk->read_hook = 0;
>>
>>    return ret;
>>  }
>> --
>> 2.46.0
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Daniel Kiper]

On Fri, Oct 11, 2024 at 08:12:59PM +0200, Rasmus Villemoes via Grub-devel wrote:
> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> writes:
>
> > Reviewed-by: phcoder@gmail.com

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

> Thanks. Can this be picked up, please?

I will take it together with other patches in next round...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Rasmus Villemoes via Grub-devel]

On Tue, Oct 15 2024, Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Oct 11, 2024 at 08:12:59PM +0200, Rasmus Villemoes via Grub-devel wrote:
>> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> writes:
>>
>> > Reviewed-by: phcoder@gmail.com
>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
>> Thanks. Can this be picked up, please?
>
> I will take it together with other patches in next round...
>

Sorry for nagging you, but this seems to have fallen through the cracks.

Rasmus

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/fs: honour file->read_hook in grub_fs_blocklist_read()

[Daniel Kiper]

On Mon, Nov 11, 2024 at 12:11:27PM +0100, Rasmus Villemoes via Grub-devel wrote:
> On Tue, Oct 15 2024, Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Fri, Oct 11, 2024 at 08:12:59PM +0200, Rasmus Villemoes via Grub-devel wrote:
> >> "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> writes:
> >>
> >> > Reviewed-by: phcoder@gmail.com
> >
> > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> >> Thanks. Can this be picked up, please?
> >
> > I will take it together with other patches in next round...
>
> Sorry for nagging you, but this seems to have fallen through the cracks.

Ugh... Now it is firmly added to the queue. Sorry for delays...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel