| Message ID | 1465443946.2948.23.camel@kernel.crashing.org |
|---|---|
| State | Accepted |
| Headers | show |
On 09.06.2016 05:45, Benjamin Herrenschmidt wrote: > From 3800b503e65e19d0769a5a92c6d9fdcc4aa95f28 Mon Sep 17 00:00:00 2001 > From: Benjamin Herrenschmidt <benh@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@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@redhat.com>
On Thu, Jun 09, 2016 at 09:35:42AM +0200, Thomas Huth wrote: > > : (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 ... ;-) Heh. Nothing magic, just BEGIN ... WHILE ... WHILE ... REPEAT ... ELSE ... THEN 1: BEGIN ... WHILE \ if 0, goto 3 ... WHILE \ if 0, goto 2 ... REPEAT \ goto 1 2: ... ELSE \ goto 4 3: ... THEN 4: so just an indeterminate loop with two exits. Segher
On 09.06.2016 10:21, Segher Boessenkool wrote: > On Thu, Jun 09, 2016 at 09:35:42AM +0200, Thomas Huth wrote: >>> : (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 ... ;-) > > Heh. Nothing magic, just > > BEGIN ... WHILE ... WHILE ... REPEAT ... ELSE ... THEN > > 1: BEGIN > ... > WHILE \ if 0, goto 3 > ... > WHILE \ if 0, goto 2 > ... > REPEAT \ goto 1 > 2: ... > ELSE \ goto 4 > 3: ... > THEN > 4: > > so just an indeterminate loop with two exits. Wow ... and I thought I basically would be able nowadays to understand most of the Forth code that you wrote ... looks like I was wrong 8-) Thanks for the explanation! Thomas
On Thu, Jun 09, 2016 at 10:32:03AM +0200, Thomas Huth wrote: > > Heh. Nothing magic, just > > > > BEGIN ... WHILE ... WHILE ... REPEAT ... ELSE ... THEN > > > > 1: BEGIN > > ... > > WHILE \ if 0, goto 3 > > ... > > WHILE \ if 0, goto 2 > > ... > > REPEAT \ goto 1 > > 2: ... > > ELSE \ goto 4 > > 3: ... > > THEN > > 4: > > > > so just an indeterminate loop with two exits. > > Wow ... and I thought I basically would be able nowadays to understand > most of the Forth code that you wrote ... looks like I was wrong 8-) The Forth structure words are not paired (if...then etc.); the compiler does no lookahead, and no explicit nesting. At compile time, structure words that jump forward leave an "orig" on the compile stack; words like THEN and REPEAT resolve that. Words like BEGIN leave a "dest"; words like AGAIN or REPEAT resolve that. Various words juggle the compile stack a bit, too. The easy way to read "BEGIN WHILE WHILE REPEAT" is that it behaves just like BEGIN WHILE REPEAT, except that the first WHILE is still unresolved. So you can put a THEN later, to have that WHILE jump there; or an ELSE THEN if you have some code (between ELSE ... THEN) that you want executed only when that first WHILE breaks out of the loop. AHEAD ... THEN IF ... THEN These are forward jumps, unconditional and conditional; AHEAD and IF leave an orig on the compile stack that THEN resolves. BEGIN ... AGAIN BEGIN ... UNTIL These are backward jumps, BEGIN leaves a dest, that AGAIN and UNTIL resolve. ELSE is AHEAD, then swap the top two compile stacks elts, then THEN. I.e. jump forward (don't know where yet), and then resolve where the previous IF will jump to. WHILE is IF followed by a swap on the compile stack, so jump forwards but leave the dest of the previous BEGIN on the top of the compilation stack. REPEAT is AGAIN followed by THEN, so jump back to the BEGIN and then resolve the WHILE. And that is all really! You can of course also make your own control flow words if you really want... Have you ever seen me use AHEAD directly? Did I ever do [ 3 cs-roll ] ? No, I like to keep things simple! :-) Segher
On 09/06/16 13:45, Benjamin Herrenschmidt wrote: > From 3800b503e65e19d0769a5a92c6d9fdcc4aa95f28 Mon Sep 17 00:00:00 2001 > From: Benjamin Herrenschmidt <benh@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@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 Please use git send-mail as this patch has lots of 0xa0 symbols instead of 0x20 (in this example between '+' and 'fat-type') and "git am" does not like it. Thanks. I applied this one without unrelated empty new lines, thanks.
On Mon, 2016-06-27 at 13:36 +1000, Alexey Kardashevskiy wrote: > > Please use git send-mail as this patch has lots of 0xa0 symbols > instead of 0x20 (in this example between '+' and 'fat-type') and "git > am" does not > like it. Thanks. I have fixed my mailer since then ;-) It was a bug in some versions of Evolution. Cheers, Ben.
On 27/06/16 14:45, Benjamin Herrenschmidt wrote: > On Mon, 2016-06-27 at 13:36 +1000, Alexey Kardashevskiy wrote: >> >> Please use git send-mail as this patch has lots of 0xa0 symbols >> instead of 0x20 (in this example between '+' and 'fat-type') and "git >> am" does not >> like it. Thanks. > > I have fixed my mailer since then ;-) It was a bug in some versions of > Evolution. sooooo you are resending patches soon, right? :)
On Thu, 2016-06-30 at 12:35 +1000, Alexey Kardashevskiy wrote: > On 27/06/16 14:45, Benjamin Herrenschmidt wrote: > > On Mon, 2016-06-27 at 13:36 +1000, Alexey Kardashevskiy wrote: > > > > > > Please use git send-mail as this patch has lots of 0xa0 symbols > > > instead of 0x20 (in this example between '+' and 'fat-type') and > > > "git > > > am" does not > > > like it. Thanks. > > > > I have fixed my mailer since then ;-) It was a bug in some versions > > of > > Evolution. > > sooooo you are resending patches soon, right? :) Caught up with other things, not even sure I didn't accidentally destroy that repo sicne I was working in a stupid submodule... I'll try to look into it next week. Cheers, Ben.
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 + +; + : .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 ; + : .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 ; + : find-file ( dir-cluster name len -- cluster file-len is-dir? true | false ) make-dos-name read-dir BEGIN (find-file) 0= WHILE next-cluster @ WHILE next-cluster @ read-cluster REPEAT false ELSE true THEN ;