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

Rob Clark robdclark at gmail.com
Mon Aug 14 14:39:40 UTC 2017


On Mon, Aug 14, 2017 at 9:47 AM, BrĂ¼ns, Stefan
<Stefan.Bruens at rwth-aachen.de> 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 <robdclark at gmail.com>
>> ---
>>  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 = &dent;
>> -
>> -             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;
>> -     }
>> -
>> -     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 do_fat_read(const char *filename, void *buffer, loff_t maxsize, int
>> dols, -               loff_t *actread)
>> -{
>> -     return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0, actread);
>> -}
>> -
>>  int file_fat_detectfs(void)
>>  {
>>       boot_sector bs;
>> @@ -1641,31 +1003,96 @@ int file_fat_detectfs(void)
>>
>>  int file_fat_ls(const char *dir)
>>  {
>> -     loff_t size;
>> +     fsdata fsdata;
>> +     fat_itr itrblock, *itr = &itrblock;
>> +     int files = 0, dirs = 0;
>> +     int ret;
>> +
>> +     ret = fat_itr_root(itr, &fsdata);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = fat_itr_resolve(itr, dir, TYPE_DIR);
>> +     if (ret)
>> +             return ret;
>> +
>> +     while (fat_itr_next(itr)) {
>> +             if (fat_itr_isdir(itr)) {
>> +                     printf("            %s/\n", itr->name);
>> +                     dirs++;
>> +             } else {
>> +                     printf(" %8u   %s\n",
>> +                            FAT2CPU32(itr->dent->size),
>> +                            itr->name);
>> +                     files++;
>> +             }
>> +     }
>> +
>> +     printf("\n%d file(s), %d dir(s)\n\n", files, dirs);
>>
>> -     return do_fat_read(dir, NULL, 0, LS_YES, &size);
>> +     return 0;
>>  }
>>
>>  int fat_exists(const char *filename)
>>  {
>> +     fsdata fsdata;
>> +     fat_itr itrblock, *itr = &itrblock;
>>       int ret;
>> -     loff_t size;
>>
>> -     ret = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, &size);
>> +     ret = fat_itr_root(itr, &fsdata);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = fat_itr_resolve(itr, filename, TYPE_ANY);
>>       return ret == 0;
>>  }
>>
>>  int fat_size(const char *filename, loff_t *size)
>>  {
>> -     return do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, size);
>> +     fsdata fsdata;
>> +     fat_itr itrblock, *itr = &itrblock;
>> +     int ret;
>> +
>> +     ret = fat_itr_root(itr, &fsdata);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = fat_itr_resolve(itr, filename, TYPE_FILE);
>> +     if (ret) {
>> +             /*
>> +              * Directories don't have size, but fs_size() is not
>> +              * expected to fail if passed a directory path:
>> +              */
>> +             fat_itr_root(itr, &fsdata);
>> +             if (!fat_itr_resolve(itr, filename, TYPE_DIR)) {
>> +                     *size = 0;
>> +                     return 0;
>> +             }
>
> It might be simpler to resolve with (TYPE_ANY) and check the TYPE of the
> returned iterator.
>
>> +             return ret;
>> +     }
>> +
>> +     *size = FAT2CPU32(itr->dent->size);
>> +
>> +     return 0;
>>  }
>>
>>  int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
>>                    loff_t maxsize, loff_t *actread)
>>  {
>> +     fsdata fsdata;
>> +     fat_itr itrblock, *itr = &itrblock;
>> +     int ret;
>> +
>> +     ret = fat_itr_root(itr, &fsdata);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = fat_itr_resolve(itr, filename, TYPE_FILE);
>> +     if (ret)
>> +             return ret;
>> +
>>       printf("reading %s\n", filename);
>> -     return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0,
>> -                           actread);
>> +     return get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
>>  }
>>
>>  int file_fat_read(const char *filename, void *buffer, int maxsize)
>> diff --git a/include/fat.h b/include/fat.h
>> index 6d3fc8e4a6..3e7ab9ea8d 100644
>> --- a/include/fat.h
>> +++ b/include/fat.h
>> @@ -58,12 +58,6 @@
>>   */
>>  #define LAST_LONG_ENTRY_MASK 0x40
>>
>> -/* Flags telling whether we should read a file or list a directory */
>> -#define LS_NO                0
>> -#define LS_YES               1
>> -#define LS_DIR               1
>> -#define LS_ROOT              2
>> -
>>  #define ISDIRDELIM(c)        ((c) == '/' || (c) == '\\')
>>
>>  #define FSTYPE_NONE  (-1)
>


More information about the U-Boot mailing list