Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Rob Clark]

On Tue, Sep 5, 2017 at 4:56 AM, Simon Glass  wrote:
> Hi Rob,
>
> On 3 September 2017 at 23:08, Łukasz Majewski  wrote:
>> On 09/02/2017 06:37 PM, Rob Clark wrote:
>>>
>>> And drop a whole lot of ugly code!
>
> :-)
>
>>
>>
>> +1
>>
>> Reviewed-by: Łukasz Majewski 
>>
>>
>>
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>>   fs/fat/fat.c  | 723
>>> ++
>>>   include/fat.h |   6 -
>>>   2 files changed, 75 insertions(+), 654 deletions(-)
>>>
>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>> index c72d6ca931..3193290434 100644
>>> --- a/fs/fat/fat.c
>>> +++ b/fs/fat/fat.c
>>> @@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc,
>>> int part_no)
>>>   }
>>> /*
>>> - * Get the first occurence of a directory delimiter ('/' or '\') in a
>>> string.
>>> - * Return index into string if found, -1 otherwise.
>>> - */
>>> -static int dirdelim(char *str)
>>> -{
>>> -   char *start = str;
>>> -
>>> -   while (*str != '\0') {
>>> -   if (ISDIRDELIM(*str))
>>> -   return str - start;
>>> -   str++;
>>> -   }
>>> -   return -1;
>>> -}
>>> -
>>> -/*
>>>* Extract zero terminated short name from a directory entry.
>>>*/
>>>   static void get_name(dir_entry *dirent, char *s_name)
>>> @@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name,
>>> int *idx)
>>> return 0;
>>>   }
>>>   -/*
>>> - * Extract the full long filename starting at 'retdent' (which is really
>>> - * a slot) into 'l_name'. If successful also copy the real directory
>>> entry
>>> - * into 'retdent'
>>> - * Return 0 on success, -1 otherwise.
>>> - */
>>> -static int
>>> -get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
>>> -dir_entry *retdent, char *l_name)
>>> -{
>>> -   dir_entry *realdent;
>>> -   dir_slot *slotptr = (dir_slot *)retdent;
>>> -   __u8 *buflimit = cluster + mydata->sect_size * ((curclust == 0) ?
>>> -   PREFETCH_BLOCKS :
>>> -
>>> mydata->clust_size);
>>> -   __u8 counter = (slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff;
>>> -   int idx = 0;
>>> -
>>> -   if (counter > VFAT_MAXSEQ) {
>>> -   debug("Error: VFAT name is too long\n");
>>> -   return -1;
>>> -   }
>>> -
>>> -   while ((__u8 *)slotptr < buflimit) {
>>> -   if (counter == 0)
>>> -   break;
>>> -   if (((slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff) !=
>>> counter)
>>> -   return -1;
>>> -   slotptr++;
>>> -   counter--;
>>> -   }
>>> -
>>> -   if ((__u8 *)slotptr >= buflimit) {
>>> -   dir_slot *slotptr2;
>>> -
>>> -   if (curclust == 0)
>>> -   return -1;
>>> -   curclust = get_fatent(mydata, curclust);
>>> -   if (CHECK_CLUST(curclust, mydata->fatsize)) {
>>> -   debug("curclust: 0x%x\n", curclust);
>>> -   printf("Invalid FAT entry\n");
>>> -   return -1;
>>> -   }
>>> -
>>> -   if (get_cluster(mydata, curclust,
>>> get_contents_vfatname_block,
>>> -   mydata->clust_size * mydata->sect_size) !=
>>> 0) {
>>> -   debug("Error: reading directory block\n");
>>> -   return -1;
>>> -   }
>>> -
>>> -   slotptr2 = (dir_slot *)get_contents_vfatname_block;
>>> -   while (counter > 0) {
>>> -   if (((slotptr2->id & ~LAST_LONG_ENTRY_MASK)
>>> -   & 0xff) != counter)
>>> -   return -1;
>>> -   slotptr2++;
>>> -   counter--;
>>> -   }
>>> -
>>> -   /* Save the real directory entry */
>>> -   realdent = (dir_entry *)slotptr2;
>>> -   while ((__u8 *)slotptr2 > get_contents_vfatname_block) {
>>> -   slotptr2--;
>>> -   slot2str(slotptr2, l_name, );
>>> -   }
>>> -   } else {
>>> -   /* Save the real directory entry */
>>> -   realdent = (dir_entry *)slotptr;
>>> -   }
>>> -
>>> -   do {
>>> -   slotptr--;
>>> -   if (slot2str(slotptr, l_name, ))
>>> -   break;
>>> -   } while (!(slotptr->id & LAST_LONG_ENTRY_MASK));
>>> -
>>> -   l_name[idx] = '\0';
>>> -   if (*l_name == DELETED_FLAG)
>>> -   *l_name = '\0';
>>> -   else if (*l_name == aRING)
>>> -   *l_name = DELETED_FLAG;
>>> -   downcase(l_name);
>>> -
>>> -   /* Return the real directory entry */
>>> -   memcpy(retdent, realdent, sizeof(dir_entry));
>>> -
>>> -   return 0;
>>> -}
>>> -
>>>   /* 

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Simon Glass]

Hi Rob,

On 3 September 2017 at 23:08, Łukasz Majewski  wrote:
> On 09/02/2017 06:37 PM, Rob Clark wrote:
>>
>> And drop a whole lot of ugly code!

:-)

>
>
> +1
>
> Reviewed-by: Łukasz Majewski 
>
>
>
>>
>> Signed-off-by: Rob Clark 
>> ---
>>   fs/fat/fat.c  | 723
>> ++
>>   include/fat.h |   6 -
>>   2 files changed, 75 insertions(+), 654 deletions(-)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index c72d6ca931..3193290434 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc,
>> int part_no)
>>   }
>> /*
>> - * Get the first occurence of a directory delimiter ('/' or '\') in a
>> string.
>> - * Return index into string if found, -1 otherwise.
>> - */
>> -static int dirdelim(char *str)
>> -{
>> -   char *start = str;
>> -
>> -   while (*str != '\0') {
>> -   if (ISDIRDELIM(*str))
>> -   return str - start;
>> -   str++;
>> -   }
>> -   return -1;
>> -}
>> -
>> -/*
>>* Extract zero terminated short name from a directory entry.
>>*/
>>   static void get_name(dir_entry *dirent, char *s_name)
>> @@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name,
>> int *idx)
>> return 0;
>>   }
>>   -/*
>> - * Extract the full long filename starting at 'retdent' (which is really
>> - * a slot) into 'l_name'. If successful also copy the real directory
>> entry
>> - * into 'retdent'
>> - * Return 0 on success, -1 otherwise.
>> - */
>> -static int
>> -get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
>> -dir_entry *retdent, char *l_name)
>> -{
>> -   dir_entry *realdent;
>> -   dir_slot *slotptr = (dir_slot *)retdent;
>> -   __u8 *buflimit = cluster + mydata->sect_size * ((curclust == 0) ?
>> -   PREFETCH_BLOCKS :
>> -
>> mydata->clust_size);
>> -   __u8 counter = (slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff;
>> -   int idx = 0;
>> -
>> -   if (counter > VFAT_MAXSEQ) {
>> -   debug("Error: VFAT name is too long\n");
>> -   return -1;
>> -   }
>> -
>> -   while ((__u8 *)slotptr < buflimit) {
>> -   if (counter == 0)
>> -   break;
>> -   if (((slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff) !=
>> counter)
>> -   return -1;
>> -   slotptr++;
>> -   counter--;
>> -   }
>> -
>> -   if ((__u8 *)slotptr >= buflimit) {
>> -   dir_slot *slotptr2;
>> -
>> -   if (curclust == 0)
>> -   return -1;
>> -   curclust = get_fatent(mydata, curclust);
>> -   if (CHECK_CLUST(curclust, mydata->fatsize)) {
>> -   debug("curclust: 0x%x\n", curclust);
>> -   printf("Invalid FAT entry\n");
>> -   return -1;
>> -   }
>> -
>> -   if (get_cluster(mydata, curclust,
>> get_contents_vfatname_block,
>> -   mydata->clust_size * mydata->sect_size) !=
>> 0) {
>> -   debug("Error: reading directory block\n");
>> -   return -1;
>> -   }
>> -
>> -   slotptr2 = (dir_slot *)get_contents_vfatname_block;
>> -   while (counter > 0) {
>> -   if (((slotptr2->id & ~LAST_LONG_ENTRY_MASK)
>> -   & 0xff) != counter)
>> -   return -1;
>> -   slotptr2++;
>> -   counter--;
>> -   }
>> -
>> -   /* Save the real directory entry */
>> -   realdent = (dir_entry *)slotptr2;
>> -   while ((__u8 *)slotptr2 > get_contents_vfatname_block) {
>> -   slotptr2--;
>> -   slot2str(slotptr2, l_name, );
>> -   }
>> -   } else {
>> -   /* Save the real directory entry */
>> -   realdent = (dir_entry *)slotptr;
>> -   }
>> -
>> -   do {
>> -   slotptr--;
>> -   if (slot2str(slotptr, l_name, ))
>> -   break;
>> -   } while (!(slotptr->id & LAST_LONG_ENTRY_MASK));
>> -
>> -   l_name[idx] = '\0';
>> -   if (*l_name == DELETED_FLAG)
>> -   *l_name = '\0';
>> -   else if (*l_name == aRING)
>> -   *l_name = DELETED_FLAG;
>> -   downcase(l_name);
>> -
>> -   /* Return the real directory entry */
>> -   memcpy(retdent, realdent, sizeof(dir_entry));
>> -
>> -   return 0;
>> -}
>> -
>>   /* Calculate short name checksum */
>>   static __u8 mkcksum(const char name[8], const char ext[3])
>>   {
>> @@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char
>> ext[3])
>> return ret;
>>   }
>>   

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Łukasz Majewski]

On 09/02/2017 06:37 PM, Rob Clark wrote:

And drop a whole lot of ugly code!


+1

Reviewed-by: Łukasz Majewski 




Signed-off-by: Rob Clark 
---
  fs/fat/fat.c  | 723 ++
  include/fat.h |   6 -
  2 files changed, 75 insertions(+), 654 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index c72d6ca931..3193290434 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int 
part_no)
  }
  
  /*

- * Get the first occurence of a directory delimiter ('/' or '\') in a string.
- * Return index into string if found, -1 otherwise.
- */
-static int dirdelim(char *str)
-{
-   char *start = str;
-
-   while (*str != '\0') {
-   if (ISDIRDELIM(*str))
-   return str - start;
-   str++;
-   }
-   return -1;
-}
-
-/*
   * Extract zero terminated short name from a directory entry.
   */
  static void get_name(dir_entry *dirent, char *s_name)
@@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name, int 
*idx)
return 0;
  }
  
-/*

- * Extract the full long filename starting at 'retdent' (which is really
- * a slot) into 'l_name'. If successful also copy the real directory entry
- * into 'retdent'
- * Return 0 on success, -1 otherwise.
- */
-static int
-get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
-dir_entry *retdent, char *l_name)
-{
-   dir_entry *realdent;
-   dir_slot *slotptr = (dir_slot *)retdent;
-   __u8 *buflimit = cluster + mydata->sect_size * ((curclust == 0) ?
-   PREFETCH_BLOCKS :
-   mydata->clust_size);
-   __u8 counter = (slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff;
-   int idx = 0;
-
-   if (counter > VFAT_MAXSEQ) {
-   debug("Error: VFAT name is too long\n");
-   return -1;
-   }
-
-   while ((__u8 *)slotptr < buflimit) {
-   if (counter == 0)
-   break;
-   if (((slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff) != counter)
-   return -1;
-   slotptr++;
-   counter--;
-   }
-
-   if ((__u8 *)slotptr >= buflimit) {
-   dir_slot *slotptr2;
-
-   if (curclust == 0)
-   return -1;
-   curclust = get_fatent(mydata, curclust);
-   if (CHECK_CLUST(curclust, mydata->fatsize)) {
-   debug("curclust: 0x%x\n", curclust);
-   printf("Invalid FAT entry\n");
-   return -1;
-   }
-
-   if (get_cluster(mydata, curclust, get_contents_vfatname_block,
-   mydata->clust_size * mydata->sect_size) != 0) {
-   debug("Error: reading directory block\n");
-   return -1;
-   }
-
-   slotptr2 = (dir_slot *)get_contents_vfatname_block;
-   while (counter > 0) {
-   if (((slotptr2->id & ~LAST_LONG_ENTRY_MASK)
-   & 0xff) != counter)
-   return -1;
-   slotptr2++;
-   counter--;
-   }
-
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr2;
-   while ((__u8 *)slotptr2 > get_contents_vfatname_block) {
-   slotptr2--;
-   slot2str(slotptr2, l_name, );
-   }
-   } else {
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr;
-   }
-
-   do {
-   slotptr--;
-   if (slot2str(slotptr, l_name, ))
-   break;
-   } while (!(slotptr->id & LAST_LONG_ENTRY_MASK));
-
-   l_name[idx] = '\0';
-   if (*l_name == DELETED_FLAG)
-   *l_name = '\0';
-   else if (*l_name == aRING)
-   *l_name = DELETED_FLAG;
-   downcase(l_name);
-
-   /* Return the real directory entry */
-   memcpy(retdent, realdent, sizeof(dir_entry));
-
-   return 0;
-}
-
  /* Calculate short name checksum */
  static __u8 mkcksum(const char name[8], const char ext[3])
  {
@@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char 
ext[3])
return ret;
  }
  
-/*

- * Get the directory entry associated with 'filename' from the directory
- * starting at 'startsect'
- */
+// These should probably DIAF..
  __u8 get_dentfromdir_block[MAX_CLUSTSIZE]
__aligned(ARCH_DMA_MINALIGN);
-
-static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
- char *filename, dir_entry *retdent,
- int dols)
-{
-   __u16 prevcksum = 0x;
-   __u32 

[U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Rob Clark]

And drop a whole lot of ugly code!

Signed-off-by: Rob Clark 
---
 fs/fat/fat.c  | 723 ++
 include/fat.h |   6 -
 2 files changed, 75 insertions(+), 654 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index c72d6ca931..3193290434 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int 
part_no)
 }
 
 /*
- * Get the first occurence of a directory delimiter ('/' or '\') in a string.
- * Return index into string if found, -1 otherwise.
- */
-static int dirdelim(char *str)
-{
-   char *start = str;
-
-   while (*str != '\0') {
-   if (ISDIRDELIM(*str))
-   return str - start;
-   str++;
-   }
-   return -1;
-}
-
-/*
  * Extract zero terminated short name from a directory entry.
  */
 static void get_name(dir_entry *dirent, char *s_name)
@@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name, int 
*idx)
return 0;
 }
 
-/*
- * Extract the full long filename starting at 'retdent' (which is really
- * a slot) into 'l_name'. If successful also copy the real directory entry
- * into 'retdent'
- * Return 0 on success, -1 otherwise.
- */
-static int
-get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
-dir_entry *retdent, char *l_name)
-{
-   dir_entry *realdent;
-   dir_slot *slotptr = (dir_slot *)retdent;
-   __u8 *buflimit = cluster + mydata->sect_size * ((curclust == 0) ?
-   PREFETCH_BLOCKS :
-   mydata->clust_size);
-   __u8 counter = (slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff;
-   int idx = 0;
-
-   if (counter > VFAT_MAXSEQ) {
-   debug("Error: VFAT name is too long\n");
-   return -1;
-   }
-
-   while ((__u8 *)slotptr < buflimit) {
-   if (counter == 0)
-   break;
-   if (((slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff) != counter)
-   return -1;
-   slotptr++;
-   counter--;
-   }
-
-   if ((__u8 *)slotptr >= buflimit) {
-   dir_slot *slotptr2;
-
-   if (curclust == 0)
-   return -1;
-   curclust = get_fatent(mydata, curclust);
-   if (CHECK_CLUST(curclust, mydata->fatsize)) {
-   debug("curclust: 0x%x\n", curclust);
-   printf("Invalid FAT entry\n");
-   return -1;
-   }
-
-   if (get_cluster(mydata, curclust, get_contents_vfatname_block,
-   mydata->clust_size * mydata->sect_size) != 0) {
-   debug("Error: reading directory block\n");
-   return -1;
-   }
-
-   slotptr2 = (dir_slot *)get_contents_vfatname_block;
-   while (counter > 0) {
-   if (((slotptr2->id & ~LAST_LONG_ENTRY_MASK)
-   & 0xff) != counter)
-   return -1;
-   slotptr2++;
-   counter--;
-   }
-
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr2;
-   while ((__u8 *)slotptr2 > get_contents_vfatname_block) {
-   slotptr2--;
-   slot2str(slotptr2, l_name, );
-   }
-   } else {
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr;
-   }
-
-   do {
-   slotptr--;
-   if (slot2str(slotptr, l_name, ))
-   break;
-   } while (!(slotptr->id & LAST_LONG_ENTRY_MASK));
-
-   l_name[idx] = '\0';
-   if (*l_name == DELETED_FLAG)
-   *l_name = '\0';
-   else if (*l_name == aRING)
-   *l_name = DELETED_FLAG;
-   downcase(l_name);
-
-   /* Return the real directory entry */
-   memcpy(retdent, realdent, sizeof(dir_entry));
-
-   return 0;
-}
-
 /* Calculate short name checksum */
 static __u8 mkcksum(const char name[8], const char ext[3])
 {
@@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char 
ext[3])
return ret;
 }
 
-/*
- * Get the directory entry associated with 'filename' from the directory
- * starting at 'startsect'
- */
+// These should probably DIAF..
 __u8 get_dentfromdir_block[MAX_CLUSTSIZE]
__aligned(ARCH_DMA_MINALIGN);
-
-static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
- char *filename, dir_entry *retdent,
- int dols)
-{
-   __u16 prevcksum = 0x;
-   __u32 curclust = START(retdent);
-   int files = 0, dirs = 0;
-
-   debug("get_dentfromdir: %s\n", filename);
-
-   

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Rob Clark]

On Mon, Aug 14, 2017 at 10:39 AM, Rob Clark  wrote:
> On Mon, Aug 14, 2017 at 9:47 AM, Brüns, Stefan
>  wrote:
>
>> Have you checked this case still works? AFAICS this is not covered in fs-
>> test.sh. Examples of suitables sandbox commands are given in the commit
>> message of:
>
> Directories split across clusters is definitely something that I've
> tested.  Not sure if some of them had vfat names spanning a cluster,
> but handling that is inherent in the design, as it never needs more
> than a single dent at a time to parse vfat names.
>
>> 18a10d46f267057ede0490ddba71c106475b4eb1 (fat: handle paths that include ../)
>
> hmm, but it does look like I'm missing something to ../ back to root dir..
>

ok, looks like what I needed was:

@@ -696,7 +696,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
 itr->cursect = itr->fsdata->data_begin +
 (clustnum * itr->fsdata->clust_size);
 } else {
-itr->cursect = 0;
+itr->cursect = parent->fsdata->rootdir_sect;
 }
 itr->dent = NULL;
 itr->remaining = 0;


fixed up locally

BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Rob Clark]

On Mon, Aug 14, 2017 at 9:47 AM, Brüns, Stefan
 wrote:
> On Montag, 14. August 2017 15:16:15 CEST Rob Clark wrote:
>> And drop a whole lot of ugly code!
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  fs/fat/fat.c  | 723
>> ++ include/fat.h |
>>  6 -
>>  2 files changed, 75 insertions(+), 654 deletions(-)
>
> Nice, even after accounting for the ~300 lines added for the iterators :-)
>
> Two comments inline ...
>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 69fa7f4cab..a50a10ba47 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int
>> part_no) }
>>
> [...]
>> -
>> -/*
>>   * Extract zero terminated short name from a directory entry.
>>   */
>>  static void get_name(dir_entry *dirent, char *s_name)
>> @@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name,
>> int *idx) return 0;
>>  }
> [...]
>> -}
>> -
>>  /* Calculate short name checksum */
>>  static __u8 mkcksum(const char name[8], const char ext[3])
>>  {
>> @@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char
>> ext[3]) return ret;
> [...]
>>
>>  /*
>>   * Read boot sector and volume info from a FAT filesystem
>> @@ -877,374 +613,6 @@ static int get_fs_info(fsdata *mydata)
>>   return 0;
>>  }
> [...]
>> -
>> - while (isdir) {
>> - int startsect = mydata->data_begin
>> - + START(dentptr) * mydata->clust_size;
>> - dir_entry dent;
>> - char *nextname = NULL;
>> -
>> - dent = *dentptr;
>> - dentptr = 
>> -
>> - idx = dirdelim(subname);
>> -
>> - if (idx >= 0) {
>> - subname[idx] = '\0';
>> - nextname = subname + idx + 1;
>> - /* Handle multiple delimiters */
>> - while (ISDIRDELIM(*nextname))
>> - nextname++;
>> - if (dols && *nextname == '\0')
>> - firsttime = 0;
>> - } else {
>> - if (dols && firsttime) {
>> - firsttime = 0;
>> - } else {
>> - isdir = 0;
>> - }
>> - }
>> -
>> - if (get_dentfromdir(mydata, startsect, subname, dentptr,
>> -  isdir ? 0 : dols) == NULL) {
>> - if (dols && !isdir)
>> - *size = 0;
>> - goto exit;
>> - }
>> -
>> - if (isdir && !(dentptr->attr & ATTR_DIR))
>> - goto exit;
>> -
>> - /*
>> -  * If we are looking for a directory, and found a directory
>> -  * type entry, and the entry is for the root directory (as
>> -  * denoted by a cluster number of 0), jump back to the start
>> -  * of the function, since at least on FAT12/16, the root dir
>> -  * lives in a hard-coded location and needs special handling
>> -  * to parse, rather than simply following the cluster linked
>> -  * list in the FAT, like other directories.
>> -  */
>
> Have you checked this case still works? AFAICS this is not covered in fs-
> test.sh. Examples of suitables sandbox commands are given in the commit
> message of:

Directories split across clusters is definitely something that I've
tested.  Not sure if some of them had vfat names spanning a cluster,
but handling that is inherent in the design, as it never needs more
than a single dent at a time to parse vfat names.

> 18a10d46f267057ede0490ddba71c106475b4eb1 (fat: handle paths that include ../)

hmm, but it does look like I'm missing something to ../ back to root dir..

BR,
-R

>> - if (isdir && (dentptr->attr & ATTR_DIR) && !START(dentptr)) {
>> - /*
>> -  * Modify the filename to remove the prefix that gets
>> -  * back to the root directory, so the initial root dir
>> -  * parsing code can continue from where we are without
>> -  * confusion.
>> -  */
>> - strcpy(fnamecopy, nextname ?: "");
>> - /*
>> -  * Set up state the same way as the function does when
>> -  * first started. This is required for the root dir
>> -  * parsing code operates in its expected environment.
>> -  */
>> - subname = "";
>> - cursect = mydata->rootdir_sect;
>> - isdir = 0;
>> - goto root_reparse;
>> - }
>> -
>> - if (idx >= 0)
>> - subname = nextname;
>> - }
>> -
>> 

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Brüns , Stefan]

On Montag, 14. August 2017 15:16:15 CEST Rob Clark wrote:
> And drop a whole lot of ugly code!
> 
> Signed-off-by: Rob Clark 
> ---
>  fs/fat/fat.c  | 723
> ++ include/fat.h | 
>  6 -
>  2 files changed, 75 insertions(+), 654 deletions(-)

Nice, even after accounting for the ~300 lines added for the iterators :-)

Two comments inline ...
 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 69fa7f4cab..a50a10ba47 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int
> part_no) }
> 
[...]
> -
> -/*
>   * Extract zero terminated short name from a directory entry.
>   */
>  static void get_name(dir_entry *dirent, char *s_name)
> @@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name,
> int *idx) return 0;
>  }
[...]
> -}
> -
>  /* Calculate short name checksum */
>  static __u8 mkcksum(const char name[8], const char ext[3])
>  {
> @@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char
> ext[3]) return ret;
[...]
> 
>  /*
>   * Read boot sector and volume info from a FAT filesystem
> @@ -877,374 +613,6 @@ static int get_fs_info(fsdata *mydata)
>   return 0;
>  }
[...]
> -
> - while (isdir) {
> - int startsect = mydata->data_begin
> - + START(dentptr) * mydata->clust_size;
> - dir_entry dent;
> - char *nextname = NULL;
> -
> - dent = *dentptr;
> - dentptr = 
> -
> - idx = dirdelim(subname);
> -
> - if (idx >= 0) {
> - subname[idx] = '\0';
> - nextname = subname + idx + 1;
> - /* Handle multiple delimiters */
> - while (ISDIRDELIM(*nextname))
> - nextname++;
> - if (dols && *nextname == '\0')
> - firsttime = 0;
> - } else {
> - if (dols && firsttime) {
> - firsttime = 0;
> - } else {
> - isdir = 0;
> - }
> - }
> -
> - if (get_dentfromdir(mydata, startsect, subname, dentptr,
> -  isdir ? 0 : dols) == NULL) {
> - if (dols && !isdir)
> - *size = 0;
> - goto exit;
> - }
> -
> - if (isdir && !(dentptr->attr & ATTR_DIR))
> - goto exit;
> -
> - /*
> -  * If we are looking for a directory, and found a directory
> -  * type entry, and the entry is for the root directory (as
> -  * denoted by a cluster number of 0), jump back to the start
> -  * of the function, since at least on FAT12/16, the root dir
> -  * lives in a hard-coded location and needs special handling
> -  * to parse, rather than simply following the cluster linked
> -  * list in the FAT, like other directories.
> -  */

Have you checked this case still works? AFAICS this is not covered in fs-
test.sh. Examples of suitables sandbox commands are given in the commit 
message of:

18a10d46f267057ede0490ddba71c106475b4eb1 (fat: handle paths that include ../)

> - if (isdir && (dentptr->attr & ATTR_DIR) && !START(dentptr)) {
> - /*
> -  * Modify the filename to remove the prefix that gets
> -  * back to the root directory, so the initial root dir
> -  * parsing code can continue from where we are without
> -  * confusion.
> -  */
> - strcpy(fnamecopy, nextname ?: "");
> - /*
> -  * Set up state the same way as the function does when
> -  * first started. This is required for the root dir
> -  * parsing code operates in its expected environment.
> -  */
> - subname = "";
> - cursect = mydata->rootdir_sect;
> - isdir = 0;
> - goto root_reparse;
> - }
> -
> - if (idx >= 0)
> - subname = nextname;
> - }
> -
> - if (dogetsize) {
> - *size = FAT2CPU32(dentptr->size);
> - ret = 0;
> - } else {
> - ret = get_contents(mydata, dentptr, pos, buffer, maxsize, size);
> - }
> - debug("Size: %u, got: %llu\n", FAT2CPU32(dentptr->size), *size);
> -
> -exit:
> - free(mydata->fatbuf);
> - return ret;
> -}
> -
> 
>  /*
>   * Directory iterator, to simplify filesystem traversal
> @@ -1571,12 +939,6 @@ static int fat_itr_resolve(fat_itr *itr, const char
> *path, unsigned type) return -ENOENT;
>  }
> 
> -int 

[U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Rob Clark]

And drop a whole lot of ugly code!

Signed-off-by: Rob Clark 
---
 fs/fat/fat.c  | 723 ++
 include/fat.h |   6 -
 2 files changed, 75 insertions(+), 654 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 69fa7f4cab..a50a10ba47 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int 
part_no)
 }
 
 /*
- * Get the first occurence of a directory delimiter ('/' or '\') in a string.
- * Return index into string if found, -1 otherwise.
- */
-static int dirdelim(char *str)
-{
-   char *start = str;
-
-   while (*str != '\0') {
-   if (ISDIRDELIM(*str))
-   return str - start;
-   str++;
-   }
-   return -1;
-}
-
-/*
  * Extract zero terminated short name from a directory entry.
  */
 static void get_name(dir_entry *dirent, char *s_name)
@@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name, int 
*idx)
return 0;
 }
 
-/*
- * Extract the full long filename starting at 'retdent' (which is really
- * a slot) into 'l_name'. If successful also copy the real directory entry
- * into 'retdent'
- * Return 0 on success, -1 otherwise.
- */
-static int
-get_vfatname(fsdata *mydata, int curclust, __u8 *cluster,
-dir_entry *retdent, char *l_name)
-{
-   dir_entry *realdent;
-   dir_slot *slotptr = (dir_slot *)retdent;
-   __u8 *buflimit = cluster + mydata->sect_size * ((curclust == 0) ?
-   PREFETCH_BLOCKS :
-   mydata->clust_size);
-   __u8 counter = (slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff;
-   int idx = 0;
-
-   if (counter > VFAT_MAXSEQ) {
-   debug("Error: VFAT name is too long\n");
-   return -1;
-   }
-
-   while ((__u8 *)slotptr < buflimit) {
-   if (counter == 0)
-   break;
-   if (((slotptr->id & ~LAST_LONG_ENTRY_MASK) & 0xff) != counter)
-   return -1;
-   slotptr++;
-   counter--;
-   }
-
-   if ((__u8 *)slotptr >= buflimit) {
-   dir_slot *slotptr2;
-
-   if (curclust == 0)
-   return -1;
-   curclust = get_fatent(mydata, curclust);
-   if (CHECK_CLUST(curclust, mydata->fatsize)) {
-   debug("curclust: 0x%x\n", curclust);
-   printf("Invalid FAT entry\n");
-   return -1;
-   }
-
-   if (get_cluster(mydata, curclust, get_contents_vfatname_block,
-   mydata->clust_size * mydata->sect_size) != 0) {
-   debug("Error: reading directory block\n");
-   return -1;
-   }
-
-   slotptr2 = (dir_slot *)get_contents_vfatname_block;
-   while (counter > 0) {
-   if (((slotptr2->id & ~LAST_LONG_ENTRY_MASK)
-   & 0xff) != counter)
-   return -1;
-   slotptr2++;
-   counter--;
-   }
-
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr2;
-   while ((__u8 *)slotptr2 > get_contents_vfatname_block) {
-   slotptr2--;
-   slot2str(slotptr2, l_name, );
-   }
-   } else {
-   /* Save the real directory entry */
-   realdent = (dir_entry *)slotptr;
-   }
-
-   do {
-   slotptr--;
-   if (slot2str(slotptr, l_name, ))
-   break;
-   } while (!(slotptr->id & LAST_LONG_ENTRY_MASK));
-
-   l_name[idx] = '\0';
-   if (*l_name == DELETED_FLAG)
-   *l_name = '\0';
-   else if (*l_name == aRING)
-   *l_name = DELETED_FLAG;
-   downcase(l_name);
-
-   /* Return the real directory entry */
-   memcpy(retdent, realdent, sizeof(dir_entry));
-
-   return 0;
-}
-
 /* Calculate short name checksum */
 static __u8 mkcksum(const char name[8], const char ext[3])
 {
@@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char 
ext[3])
return ret;
 }
 
-/*
- * Get the directory entry associated with 'filename' from the directory
- * starting at 'startsect'
- */
+// These should probably DIAF..
 __u8 get_dentfromdir_block[MAX_CLUSTSIZE]
__aligned(ARCH_DMA_MINALIGN);
-
-static dir_entry *get_dentfromdir(fsdata *mydata, int startsect,
- char *filename, dir_entry *retdent,
- int dols)
-{
-   __u16 prevcksum = 0x;
-   __u32 curclust = START(retdent);
-   int files = 0, dirs = 0;
-
-   debug("get_dentfromdir: %s\n", filename);
-
-