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

Brüns, Stefan
2017-08-14T13:47:48Z

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@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:

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 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)