[SLOF] [PATCH v2] fat-files: Fix access to FAT32 dir/files when cluster > 16-bits

[Thomas Huth]

On 09.06.2016 05:45, Benjamin Herrenschmidt wrote:
> From 3800b503e65e19d0769a5a92c6d9fdcc4aa95f28 Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> Date: Thu, 9 Jun 2016 13:01:24 +1000
> Subject: 
> 
> On FAT32, the directory entry contains a new field providing the
> top 16-bits of the cluster number. We didn't use it, thus reading
> the wrong sectors when trying to access files or directories
> beyond block 0x10000.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> ---
> 
> v2. Remove unrelated change
> 
>  slof/fs/packages/fat-files.fs | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/slof/fs/packages/fat-files.fs b/slof/fs/packages/fat-files.fs
> index d919452..5d578f1 100644
> --- a/slof/fs/packages/fat-files.fs
> +++ b/slof/fs/packages/fat-files.fs
> @@ -73,6 +73,14 @@ INSTANCE VARIABLE next-cluster
>      THEN
>  ;
>  
> +\ Read cluster# from directory entry (handle FAT32 extension)
> +: get-cluster ( direntry -- cluster# )
> +  fat-type @ 20 = IF
> +    dup 14 + 2c@ bwjoin 10 lshift
> +  ELSE 0 THEN
> +  swap 1a + 2c@ bwjoin +
> +;

Quoting the Wikipedia entry for 0x14 at
https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system#DIR_OFS_14h :

"High two bytes of first cluster number in FAT32; with the low two bytes
stored at offset 0x1A."

So your change here sounds right!

>  : .time ( x -- )
>    base @ >r decimal
>    b #split 2 0.r [char] : emit  5 #split 2 0.r [char] : emit  2* 2 0.r
> @@ -87,7 +95,7 @@ INSTANCE VARIABLE next-cluster
>    dup 0b + c@ 8 and IF drop EXIT THEN \ volume label, not a file
>    dup c@ e5 = IF drop EXIT THEN \ deleted file
>    cr
> -  dup 1a + 2c@ bwjoin [char] # emit 4 0.r space \ starting cluster
> +  dup get-cluster [char] # emit 8 0.r space \ starting cluster
>    dup 18 + 2c@ bwjoin .date space
>    dup 16 + 2c@ bwjoin .time space
>    dup 1c + 4c@ bljoin base @ decimal swap a .r base ! space \ size in bytes
> @@ -98,6 +106,7 @@ INSTANCE VARIABLE next-cluster
>    drop ;
>  : .dir-entries ( adr n -- )
>    0 ?DO dup i 20 * + dup c@ 0= IF drop LEAVE THEN .dir-entry LOOP drop ;
> +

Nit: Unrelated white space change.

>  : .dir ( cluster# -- )
>    read-dir BEGIN data @ #data @ 20 / .dir-entries next-cluster @ WHILE
>    next-cluster @ read-cluster REPEAT ;
> @@ -114,8 +123,10 @@ CREATE dos-name b allot
>  : (find-file) ( -- cluster file-len is-dir? true | false )
>    data @ BEGIN dup data @ #data @ + < WHILE
>    dup dos-name b comp WHILE 20 + REPEAT
> -  dup 1a + 2c@ bwjoin swap dup 1c + 4c@ bljoin swap 0b + c@ 10 and 0<> true
> +  dup get-cluster
> +  swap dup 1c + 4c@ bljoin swap 0b + c@ 10 and 0<> true
>    ELSE drop false THEN ;

Unrelated to your patch, but: WTF? Where's the IF-statement for that
ELSE? ... must be some magic code by Segher again ... ;-)

Anyway, your changes look fine to me, so:
Reviewed-by: Thomas Huth <thuth at redhat.com>