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

["Brüns, Stefan" <Stefan.Bruens@rwth-aachen.de>]

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)