[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[AKASHI Takahiro]

This patch series is an attempt to address FAT write related issues
in an effort of running UEFI SCT (Self-Certification Test) to verify
UEFI support on u-boot.

SCT is a test platform as well as an extentisive collection of test
cases for UEFI specification. It can run all the tests automatically
and save test results to dedicated log files.

AFAIK, what's missing in the current fat file system to safely run
SCT without errors (I don't mean test case failures) are:
* write a file located under sub-directories
* write a file with non-zero offset
* delete a file
* create a directory

Patch#1 to patch#6 are some sort of preparatory ones.
Patch#7 implements write with sub-directories path.
Patch#8 to patch#10 implement write with non-zero offset.
Patch#11 to patch#15 are related to creating a directory.
Patch#16 provides delete, but it doesn't actually delete a file
but instead return 0 with purging a file content, which is good
enough to run SCT for now.
Finally, patch#17 fixes a minor bug in fs-test.sh.

I applied this patch set on top of v2018.07 along with a couple of
yet-to--be-upstreamed UEFI-related patches, and could successfully
run unmodified SCT[1] on qemu-arm.

[1] http://uefi.org/testtools

AKASHI Takahiro (17):
  fs: fat: extend get_fs_info() for write use
  fs: fat: handle "." and ".." of root dir correctly with
    fat_itr_resolve()
  fs: fat: make directory iterator global for write use
  fs: fat: assure iterator's ->dent belongs to ->clust
  fs: fat: check and normailze file name
  fs: fat: write returns error code instead of -1
  fs: fat: support write with sub-directory path
  fs: fat: refactor write interface for a file offset
  fs: fat: support write with non-zero offset
  cmd: fat: add offset parameter to fatwrite
  fs: add mkdir interface
  fs: fat: remember the starting cluster number of directory
  fs: fat: support mkdir
  cmd: fat: add fatmkdir command
  efi_loader: file: support creating a directory
  efi_loader: implement a pseudo "file delete"
  fs-test: fix false positive error at Test Case 12

 cmd/fat.c                 |   22 +-
 fs/fat/fat.c              |   98 ++--
 fs/fat/fat_write.c        | 1051 +++++++++++++++++++++++--------------
 fs/fs.c                   |   46 ++
 include/fat.h             |   37 ++
 include/fs.h              |   10 +
 lib/efi_loader/efi_file.c |   24 +-
 test/fs/fs-test.sh        |    2 +-
 8 files changed, 825 insertions(+), 465 deletions(-)

-- 
2.17.0

[U-Boot] [PATCH 01/17] fs: fat: extend get_fs_info() for write use

[AKASHI Takahiro]

get_fs_info() was introduced in major re-work of read operation by Rob.
We want to reuse this function in write operation by extending it with
additional members in fsdata structure.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c  | 3 +++
 include/fat.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 4efe8a3eda..b48f48a751 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -556,6 +556,9 @@ static int get_fs_info(fsdata *mydata)
 		return ret;
 	}
 
+	mydata->bs_total_sect = bs.total_sect;
+	mydata->bs_fats = bs.fats;
+
 	if (mydata->fatsize == 32) {
 		mydata->fatlength = bs.fat32_length;
 	} else {
diff --git a/include/fat.h b/include/fat.h
index 09e1423685..0c88b59a4a 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -173,6 +173,8 @@ typedef struct {
 	int	fatbufnum;	/* Used by get_fatent, init to -1 */
 	int	rootdir_size;	/* Size of root dir for non-FAT32 */
 	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
+	__u32	bs_total_sect;	/* Boot sector's number of sectors */
+	__u8	bs_fats;	/* Boot sector's number of FATs */
 } fsdata;
 
 static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)
-- 
2.17.0

[U-Boot] [PATCH 01/17] fs: fat: extend get_fs_info() for write use

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> get_fs_info() was introduced in major re-work of read operation by Rob.
> We want to reuse this function in write operation by extending it with
> additional members in fsdata structure.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fat/fat.c  | 3 +++
>  include/fat.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 4efe8a3eda..b48f48a751 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -556,6 +556,9 @@ static int get_fs_info(fsdata *mydata)
>  		return ret;
>  	}
>  
> +	mydata->bs_total_sect = bs.total_sect;
> +	mydata->bs_fats = bs.fats;
> +
>  	if (mydata->fatsize == 32) {
>  		mydata->fatlength = bs.fat32_length;
>  	} else {
> diff --git a/include/fat.h b/include/fat.h
> index 09e1423685..0c88b59a4a 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -173,6 +173,8 @@ typedef struct {
>  	int	fatbufnum;	/* Used by get_fatent, init to -1 */
>  	int	rootdir_size;	/* Size of root dir for non-FAT32 */
>  	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
> +	__u32	bs_total_sect;	/* Boot sector's number of sectors */

How can one sector have multiple sectors?
Please, reword the comment to something more obvious.

Best regards

Heinrich

> +	__u8	bs_fats;	/* Boot sector's number of FATs */
>  } fsdata;
>  
>  static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)
> 

[U-Boot] [PATCH 01/17] fs: fat: extend get_fs_info() for write use

[Heinrich Schuchardt]

On 07/20/2018 08:06 PM, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
>> get_fs_info() was introduced in major re-work of read operation by Rob.
>> We want to reuse this function in write operation by extending it with
>> additional members in fsdata structure.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  fs/fat/fat.c  | 3 +++
>>  include/fat.h | 2 ++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 4efe8a3eda..b48f48a751 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -556,6 +556,9 @@ static int get_fs_info(fsdata *mydata)
>>  		return ret;
>>  	}
>>  
>> +	mydata->bs_total_sect = bs.total_sect;

Please, check here if sectors is non-zero and in this case use that
value. Do not rely on total_sect = 0 in later coding to read sectors.
Anyway there should be only one place in the coding where we determine
the sector count.

The same logic is used in the Linux kernel:

fs/fat/inode.c:1766:   total_sectors = bpb.fat_sectors;
fs/fat/inode.c:1767:   if (total_sectors == 0)
fs/fat/inode.c:1768:           total_sectors = bpb.fat_total_sect;

>> +	mydata->bs_fats = bs.fats;
>> +
>>  	if (mydata->fatsize == 32) {
>>  		mydata->fatlength = bs.fat32_length;
>>  	} else {
>> diff --git a/include/fat.h b/include/fat.h
>> index 09e1423685..0c88b59a4a 100644
>> --- a/include/fat.h
>> +++ b/include/fat.h
>> @@ -173,6 +173,8 @@ typedef struct {
>>  	int	fatbufnum;	/* Used by get_fatent, init to -1 */
>>  	int	rootdir_size;	/* Size of root dir for non-FAT32 */
>>  	__u32	root_cluster;	/* First cluster of root dir for FAT32 */
>> +	__u32	bs_total_sect;	/* Boot sector's number of sectors */

Please, use

__u32 total_sect; /* Number of sectors */

> 
> How can one sector have multiple sectors?
> Please, reword the comment to something more obvious.
> 
> Best regards
> 
> Heinrich
> 
>> +	__u8	bs_fats;	/* Boot sector's number of FATs */

and

__u8 fats; /* Number of FATs */

This is not information about the boot sector but about the partition.

Best regards

Heinich

>>  } fsdata;
>>  
>>  static inline u32 clust_to_sect(fsdata *fsdata, u32 clust)
>>
> 
> 

[U-Boot] [PATCH 02/17] fs: fat: handle "." and ".." of root dir correctly with fat_itr_resolve()

[AKASHI Takahiro]

FAT's root directory does not have "." nor ".."
So care must be taken when scanning root directory with fat_itr_resolve().
Without this patch, any file path starting with "." or ".." will not be
resolved at all.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index b48f48a751..fd6523c66b 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -927,6 +927,27 @@ static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
 	while (next[0] && !ISDIRDELIM(next[0]))
 		next++;
 
+	if (itr->is_root) {
+		/* root dir doesn't have "." nor ".." */
+		if ((((next - path) == 1) && !strncmp(path, ".", 1)) ||
+		    (((next - path) == 2) && !strncmp(path, "..", 2))) {
+			/* point back to itself */
+			itr->clust = itr->fsdata->root_cluster;
+			itr->dent = NULL;
+			itr->remaining = 0;
+			itr->last_cluster = 0;
+
+			if (next[0] == 0) {
+				if (type & TYPE_DIR)
+					return 0;
+				else
+					return -ENOENT;
+			}
+
+			return fat_itr_resolve(itr, next, type);
+		}
+	}
+
 	while (fat_itr_next(itr)) {
 		int match = 0;
 		unsigned n = max(strlen(itr->name), (size_t)(next - path));
-- 
2.17.0

[U-Boot] [PATCH 02/17] fs: fat: handle "." and ".." of root dir correctly with fat_itr_resolve()

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> FAT's root directory does not have "." nor ".."
> So care must be taken when scanning root directory with fat_itr_resolve().
> Without this patch, any file path starting with "." or ".." will not be
> resolved at all.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fat/fat.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index b48f48a751..fd6523c66b 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -927,6 +927,27 @@ static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
>  	while (next[0] && !ISDIRDELIM(next[0]))
>  		next++;
>  
> +	if (itr->is_root) {
> +		/* root dir doesn't have "." nor ".." */

I understand why root has no ../ but  /./ should be valid.

On Linux 'ls /./' displays the root directory.

Best regards

Heinrich

> +		if ((((next - path) == 1) && !strncmp(path, ".", 1)) ||
> +		    (((next - path) == 2) && !strncmp(path, "..", 2))) {
> +			/* point back to itself */
> +			itr->clust = itr->fsdata->root_cluster;
> +			itr->dent = NULL;
> +			itr->remaining = 0;
> +			itr->last_cluster = 0;
> +
> +			if (next[0] == 0) {
> +				if (type & TYPE_DIR)
> +					return 0;
> +				else
> +					return -ENOENT;
> +			}
> +
> +			return fat_itr_resolve(itr, next, type);
> +		}
> +	}
> +
>  	while (fat_itr_next(itr)) {
>  		int match = 0;
>  		unsigned n = max(strlen(itr->name), (size_t)(next - path));
> 

[U-Boot] [PATCH 02/17] fs: fat: handle "." and ".." of root dir correctly with fat_itr_resolve()

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 08:09:00PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > FAT's root directory does not have "." nor ".."
> > So care must be taken when scanning root directory with fat_itr_resolve().
> > Without this patch, any file path starting with "." or ".." will not be
> > resolved at all.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  fs/fat/fat.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index b48f48a751..fd6523c66b 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -927,6 +927,27 @@ static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> >  	while (next[0] && !ISDIRDELIM(next[0]))
> >  		next++;
> >  
> > +	if (itr->is_root) {
> > +		/* root dir doesn't have "." nor ".." */
> 
> I understand why root has no ../ but  /./ should be valid.

What I meant here is that, according to FAT specification, the root
directory has neither "." directory entry nor ".." in its associated
clusters in contrast to ordinary directories.
So when user specifies "." or ".." (and other variants), we will have to
take special care to handle them correctly.

> On Linux 'ls /./' displays the root directory.

My code works with any of the following paths:
 * .
 * ..
 * /
 * /.
 * /.., or
 * even any combination of above, like
     ./.././.././

Note that "ls /.." on linux also shows the root directory.

Please try.

-Takahiro AKASHI


> Best regards
> 
> Heinrich
> 
> > +		if ((((next - path) == 1) && !strncmp(path, ".", 1)) ||
> > +		    (((next - path) == 2) && !strncmp(path, "..", 2))) {
> > +			/* point back to itself */
> > +			itr->clust = itr->fsdata->root_cluster;
> > +			itr->dent = NULL;
> > +			itr->remaining = 0;
> > +			itr->last_cluster = 0;
> > +
> > +			if (next[0] == 0) {
> > +				if (type & TYPE_DIR)
> > +					return 0;
> > +				else
> > +					return -ENOENT;
> > +			}
> > +
> > +			return fat_itr_resolve(itr, next, type);
> > +		}
> > +	}
> > +
> >  	while (fat_itr_next(itr)) {
> >  		int match = 0;
> >  		unsigned n = max(strlen(itr->name), (size_t)(next - path));
> > 
> 

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[AKASHI Takahiro]

Directory iterator was introduced in major re-work of read operation by
Rob. We want to use it for write operation extensively as well.
This patch makes relevant functions, as well as iterator defition, visible
outside of fat.c.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c  | 39 ++++++---------------------------------
 include/fat.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index fd6523c66b..0f82cbe1bd 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
  * For more complete example, see fat_itr_resolve()
  */
 
-typedef struct {
-	fsdata    *fsdata;        /* filesystem parameters */
-	unsigned   clust;         /* current cluster */
-	int        last_cluster;  /* set once we've read last cluster */
-	int        is_root;       /* is iterator at root directory */
-	int        remaining;     /* remaining dent's in current cluster */
-
-	/* current iterator position values: */
-	dir_entry *dent;          /* current directory entry */
-	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
-	char       s_name[14];    /* short 8.3 name */
-	char      *name;          /* l_name if there is one, else s_name */
-
-	/* storage for current cluster in memory: */
-	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
-} fat_itr;
-
-static int fat_itr_isdir(fat_itr *itr);
-
 /**
  * fat_itr_root() - initialize an iterator to start at the root
  * directory
@@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
  * @fsdata: filesystem data for the partition
  * @return 0 on success, else -errno
  */
-static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
+int fat_itr_root(fat_itr *itr, fsdata *fsdata)
 {
 	if (get_fs_info(fsdata))
 		return -ENXIO;
@@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
  * @parent: the iterator pointing at a directory entry in the
  *    parent directory of the directory to iterate
  */
-static void fat_itr_child(fat_itr *itr, fat_itr *parent)
+void fat_itr_child(fat_itr *itr, fat_itr *parent)
 {
 	fsdata *mydata = parent->fsdata;  /* for silly macros */
 	unsigned clustnum = START(parent->dent);
@@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
 	itr->last_cluster = 0;
 }
 
-static void *next_cluster(fat_itr *itr)
+void *next_cluster(fat_itr *itr)
 {
 	fsdata *mydata = itr->fsdata;  /* for silly macros */
 	int ret;
@@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
  * @return boolean, 1 if success or 0 if no more entries in the
  *    current directory
  */
-static int fat_itr_next(fat_itr *itr)
+int fat_itr_next(fat_itr *itr)
 {
 	dir_entry *dent;
 
@@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
  * @itr: the iterator
  * @return true if cursor is at a directory
  */
-static int fat_itr_isdir(fat_itr *itr)
+int fat_itr_isdir(fat_itr *itr)
 {
 	return !!(itr->dent->attr & ATTR_DIR);
 }
 
-/*
- * Helpers:
- */
-
-#define TYPE_FILE 0x1
-#define TYPE_DIR  0x2
-#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
-
 /**
  * fat_itr_resolve() - traverse directory structure to resolve the
  * requested path.
@@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
  * @type: bitmask of allowable file types
  * @return 0 on success or -errno
  */
-static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
+int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
 {
 	const char *next;
 
diff --git a/include/fat.h b/include/fat.h
index 0c88b59a4a..577e6b4592 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
 	return (sect - fsdata->data_begin) / fsdata->clust_size;
 }
 
+/*
+ * Directory iterator
+ */
+
+#define TYPE_FILE 0x1
+#define TYPE_DIR  0x2
+#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
+
+typedef struct {
+	fsdata    *fsdata;        /* filesystem parameters */
+	unsigned   clust;         /* current cluster */
+	int        last_cluster;  /* set once we've read last cluster */
+	int        is_root;       /* is iterator at root directory */
+	int        remaining;     /* remaining dent's in current cluster */
+
+	/* current iterator position values: */
+	dir_entry *dent;          /* current directory entry */
+	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
+	char       s_name[14];    /* short 8.3 name */
+	char      *name;          /* l_name if there is one, else s_name */
+
+	/* storage for current cluster in memory: */
+	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
+} fat_itr;
+
+int fat_itr_root(fat_itr *itr, fsdata *fsdata);
+void fat_itr_child(fat_itr *itr, fat_itr *parent);
+void *next_cluster(fat_itr *itr);
+int fat_itr_next(fat_itr *itr);
+int fat_itr_isdir(fat_itr *itr);
+int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
+
 int file_fat_detectfs(void);
 int fat_exists(const char *filename);
 int fat_size(const char *filename, loff_t *size);
-- 
2.17.0

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> Directory iterator was introduced in major re-work of read operation by
> Rob. We want to use it for write operation extensively as well.
> This patch makes relevant functions, as well as iterator defition, visible
> outside of fat.c.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fat/fat.c  | 39 ++++++---------------------------------
>  include/fat.h | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index fd6523c66b..0f82cbe1bd 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
>   * For more complete example, see fat_itr_resolve()
>   */
>  
> -typedef struct {
> -	fsdata    *fsdata;        /* filesystem parameters */
> -	unsigned   clust;         /* current cluster */
> -	int        last_cluster;  /* set once we've read last cluster */
> -	int        is_root;       /* is iterator at root directory */
> -	int        remaining;     /* remaining dent's in current cluster */
> -
> -	/* current iterator position values: */
> -	dir_entry *dent;          /* current directory entry */
> -	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> -	char       s_name[14];    /* short 8.3 name */
> -	char      *name;          /* l_name if there is one, else s_name */
> -
> -	/* storage for current cluster in memory: */
> -	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> -} fat_itr;
> -
> -static int fat_itr_isdir(fat_itr *itr);
> -
>  /**
>   * fat_itr_root() - initialize an iterator to start at the root
>   * directory
> @@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
>   * @fsdata: filesystem data for the partition
>   * @return 0 on success, else -errno
>   */
> -static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> +int fat_itr_root(fat_itr *itr, fsdata *fsdata)
>  {
>  	if (get_fs_info(fsdata))
>  		return -ENXIO;
> @@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
>   * @parent: the iterator pointing at a directory entry in the
>   *    parent directory of the directory to iterate
>   */
> -static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> +void fat_itr_child(fat_itr *itr, fat_itr *parent)
>  {
>  	fsdata *mydata = parent->fsdata;  /* for silly macros */
>  	unsigned clustnum = START(parent->dent);
> @@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
>  	itr->last_cluster = 0;
>  }
>  
> -static void *next_cluster(fat_itr *itr)
> +void *next_cluster(fat_itr *itr)
>  {
>  	fsdata *mydata = itr->fsdata;  /* for silly macros */
>  	int ret;
> @@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
>   * @return boolean, 1 if success or 0 if no more entries in the
>   *    current directory
>   */
> -static int fat_itr_next(fat_itr *itr)
> +int fat_itr_next(fat_itr *itr)
>  {
>  	dir_entry *dent;
>  
> @@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
>   * @itr: the iterator
>   * @return true if cursor is at a directory
>   */
> -static int fat_itr_isdir(fat_itr *itr)
> +int fat_itr_isdir(fat_itr *itr)
>  {
>  	return !!(itr->dent->attr & ATTR_DIR);
>  }
>  
> -/*
> - * Helpers:
> - */
> -
> -#define TYPE_FILE 0x1
> -#define TYPE_DIR  0x2
> -#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> -
>  /**
>   * fat_itr_resolve() - traverse directory structure to resolve the
>   * requested path.
> @@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
>   * @type: bitmask of allowable file types
>   * @return 0 on success or -errno
>   */
> -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
>  {
>  	const char *next;
>  
> diff --git a/include/fat.h b/include/fat.h
> index 0c88b59a4a..577e6b4592 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
>  	return (sect - fsdata->data_begin) / fsdata->clust_size;
>  }
>  
> +/*
> + * Directory iterator
> + */
> +
> +#define TYPE_FILE 0x1
> +#define TYPE_DIR  0x2
> +#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)

Please, rename these to something more specific like FS_ITER_ANY.

TYPE_ANY already is defined in fs/reiserfs/reiserfs_private.h:

fs/reiserfs/reiserfs_private.h:160:#define TYPE_ANY 15

Best regards

Heinrich

> +
> +typedef struct {
> +	fsdata    *fsdata;        /* filesystem parameters */
> +	unsigned   clust;         /* current cluster */
> +	int        last_cluster;  /* set once we've read last cluster */
> +	int        is_root;       /* is iterator at root directory */
> +	int        remaining;     /* remaining dent's in current cluster */
> +
> +	/* current iterator position values: */
> +	dir_entry *dent;          /* current directory entry */
> +	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> +	char       s_name[14];    /* short 8.3 name */
> +	char      *name;          /* l_name if there is one, else s_name */
> +
> +	/* storage for current cluster in memory: */
> +	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> +} fat_itr;
> +
> +int fat_itr_root(fat_itr *itr, fsdata *fsdata);
> +void fat_itr_child(fat_itr *itr, fat_itr *parent);
> +void *next_cluster(fat_itr *itr);
> +int fat_itr_next(fat_itr *itr);
> +int fat_itr_isdir(fat_itr *itr);
> +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
> +
>  int file_fat_detectfs(void);
>  int fat_exists(const char *filename);
>  int fat_size(const char *filename, loff_t *size);
> 

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 08:02:57PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > Directory iterator was introduced in major re-work of read operation by
> > Rob. We want to use it for write operation extensively as well.
> > This patch makes relevant functions, as well as iterator defition, visible
> > outside of fat.c.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  fs/fat/fat.c  | 39 ++++++---------------------------------
> >  include/fat.h | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+), 33 deletions(-)
> > 
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index fd6523c66b..0f82cbe1bd 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
> >   * For more complete example, see fat_itr_resolve()
> >   */
> >  
> > -typedef struct {
> > -	fsdata    *fsdata;        /* filesystem parameters */
> > -	unsigned   clust;         /* current cluster */
> > -	int        last_cluster;  /* set once we've read last cluster */
> > -	int        is_root;       /* is iterator at root directory */
> > -	int        remaining;     /* remaining dent's in current cluster */
> > -
> > -	/* current iterator position values: */
> > -	dir_entry *dent;          /* current directory entry */
> > -	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > -	char       s_name[14];    /* short 8.3 name */
> > -	char      *name;          /* l_name if there is one, else s_name */
> > -
> > -	/* storage for current cluster in memory: */
> > -	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > -} fat_itr;
> > -
> > -static int fat_itr_isdir(fat_itr *itr);
> > -
> >  /**
> >   * fat_itr_root() - initialize an iterator to start at the root
> >   * directory
> > @@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
> >   * @fsdata: filesystem data for the partition
> >   * @return 0 on success, else -errno
> >   */
> > -static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> > +int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> >  {
> >  	if (get_fs_info(fsdata))
> >  		return -ENXIO;
> > @@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> >   * @parent: the iterator pointing at a directory entry in the
> >   *    parent directory of the directory to iterate
> >   */
> > -static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> > +void fat_itr_child(fat_itr *itr, fat_itr *parent)
> >  {
> >  	fsdata *mydata = parent->fsdata;  /* for silly macros */
> >  	unsigned clustnum = START(parent->dent);
> > @@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> >  	itr->last_cluster = 0;
> >  }
> >  
> > -static void *next_cluster(fat_itr *itr)
> > +void *next_cluster(fat_itr *itr)
> >  {
> >  	fsdata *mydata = itr->fsdata;  /* for silly macros */
> >  	int ret;
> > @@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
> >   * @return boolean, 1 if success or 0 if no more entries in the
> >   *    current directory
> >   */
> > -static int fat_itr_next(fat_itr *itr)
> > +int fat_itr_next(fat_itr *itr)
> >  {
> >  	dir_entry *dent;
> >  
> > @@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
> >   * @itr: the iterator
> >   * @return true if cursor is at a directory
> >   */
> > -static int fat_itr_isdir(fat_itr *itr)
> > +int fat_itr_isdir(fat_itr *itr)
> >  {
> >  	return !!(itr->dent->attr & ATTR_DIR);
> >  }
> >  
> > -/*
> > - * Helpers:
> > - */
> > -
> > -#define TYPE_FILE 0x1
> > -#define TYPE_DIR  0x2
> > -#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> > -
> >  /**
> >   * fat_itr_resolve() - traverse directory structure to resolve the
> >   * requested path.
> > @@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
> >   * @type: bitmask of allowable file types
> >   * @return 0 on success or -errno
> >   */
> > -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> >  {
> >  	const char *next;
> >  
> > diff --git a/include/fat.h b/include/fat.h
> > index 0c88b59a4a..577e6b4592 100644
> > --- a/include/fat.h
> > +++ b/include/fat.h
> > @@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
> >  	return (sect - fsdata->data_begin) / fsdata->clust_size;
> >  }
> >  
> > +/*
> > + * Directory iterator
> > + */
> > +
> > +#define TYPE_FILE 0x1
> > +#define TYPE_DIR  0x2
> > +#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> 
> Please, rename these to something more specific like FS_ITER_ANY.
> 
> TYPE_ANY already is defined in fs/reiserfs/reiserfs_private.h:
> 
> fs/reiserfs/reiserfs_private.h:160:#define TYPE_ANY 15

Good catch though the definition above comes directly from Rob's
original patch.

Since there is only one occurrence of TYPE_ANY in fs/fat,
I'd rather expand it without introducing another definition.

Thanks,
-Takahiro AKASHI


> Best regards
> 
> Heinrich
> 
> > +
> > +typedef struct {
> > +	fsdata    *fsdata;        /* filesystem parameters */
> > +	unsigned   clust;         /* current cluster */
> > +	int        last_cluster;  /* set once we've read last cluster */
> > +	int        is_root;       /* is iterator at root directory */
> > +	int        remaining;     /* remaining dent's in current cluster */
> > +
> > +	/* current iterator position values: */
> > +	dir_entry *dent;          /* current directory entry */
> > +	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > +	char       s_name[14];    /* short 8.3 name */
> > +	char      *name;          /* l_name if there is one, else s_name */
> > +
> > +	/* storage for current cluster in memory: */
> > +	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > +} fat_itr;
> > +
> > +int fat_itr_root(fat_itr *itr, fsdata *fsdata);
> > +void fat_itr_child(fat_itr *itr, fat_itr *parent);
> > +void *next_cluster(fat_itr *itr);
> > +int fat_itr_next(fat_itr *itr);
> > +int fat_itr_isdir(fat_itr *itr);
> > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
> > +
> >  int file_fat_detectfs(void);
> >  int fat_exists(const char *filename);
> >  int fat_size(const char *filename, loff_t *size);
> > 
> 

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[AKASHI Takahiro]

On Mon, Jul 23, 2018 at 05:06:46PM +0900, AKASHI Takahiro wrote:
> On Fri, Jul 20, 2018 at 08:02:57PM +0200, Heinrich Schuchardt wrote:
> > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > > Directory iterator was introduced in major re-work of read operation by
> > > Rob. We want to use it for write operation extensively as well.
> > > This patch makes relevant functions, as well as iterator defition, visible
> > > outside of fat.c.
> > > 
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  fs/fat/fat.c  | 39 ++++++---------------------------------
> > >  include/fat.h | 32 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 38 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > > index fd6523c66b..0f82cbe1bd 100644
> > > --- a/fs/fat/fat.c
> > > +++ b/fs/fat/fat.c
> > > @@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
> > >   * For more complete example, see fat_itr_resolve()
> > >   */
> > >  
> > > -typedef struct {
> > > -	fsdata    *fsdata;        /* filesystem parameters */
> > > -	unsigned   clust;         /* current cluster */
> > > -	int        last_cluster;  /* set once we've read last cluster */
> > > -	int        is_root;       /* is iterator at root directory */
> > > -	int        remaining;     /* remaining dent's in current cluster */
> > > -
> > > -	/* current iterator position values: */
> > > -	dir_entry *dent;          /* current directory entry */
> > > -	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > > -	char       s_name[14];    /* short 8.3 name */
> > > -	char      *name;          /* l_name if there is one, else s_name */
> > > -
> > > -	/* storage for current cluster in memory: */
> > > -	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > > -} fat_itr;
> > > -
> > > -static int fat_itr_isdir(fat_itr *itr);
> > > -
> > >  /**
> > >   * fat_itr_root() - initialize an iterator to start at the root
> > >   * directory
> > > @@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
> > >   * @fsdata: filesystem data for the partition
> > >   * @return 0 on success, else -errno
> > >   */
> > > -static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> > > +int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> > >  {
> > >  	if (get_fs_info(fsdata))
> > >  		return -ENXIO;
> > > @@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> > >   * @parent: the iterator pointing at a directory entry in the
> > >   *    parent directory of the directory to iterate
> > >   */
> > > -static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> > > +void fat_itr_child(fat_itr *itr, fat_itr *parent)
> > >  {
> > >  	fsdata *mydata = parent->fsdata;  /* for silly macros */
> > >  	unsigned clustnum = START(parent->dent);
> > > @@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> > >  	itr->last_cluster = 0;
> > >  }
> > >  
> > > -static void *next_cluster(fat_itr *itr)
> > > +void *next_cluster(fat_itr *itr)
> > >  {
> > >  	fsdata *mydata = itr->fsdata;  /* for silly macros */
> > >  	int ret;
> > > @@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
> > >   * @return boolean, 1 if success or 0 if no more entries in the
> > >   *    current directory
> > >   */
> > > -static int fat_itr_next(fat_itr *itr)
> > > +int fat_itr_next(fat_itr *itr)
> > >  {
> > >  	dir_entry *dent;
> > >  
> > > @@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
> > >   * @itr: the iterator
> > >   * @return true if cursor is at a directory
> > >   */
> > > -static int fat_itr_isdir(fat_itr *itr)
> > > +int fat_itr_isdir(fat_itr *itr)
> > >  {
> > >  	return !!(itr->dent->attr & ATTR_DIR);
> > >  }
> > >  
> > > -/*
> > > - * Helpers:
> > > - */
> > > -
> > > -#define TYPE_FILE 0x1
> > > -#define TYPE_DIR  0x2
> > > -#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> > > -
> > >  /**
> > >   * fat_itr_resolve() - traverse directory structure to resolve the
> > >   * requested path.
> > > @@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
> > >   * @type: bitmask of allowable file types
> > >   * @return 0 on success or -errno
> > >   */
> > > -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> > > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> > >  {
> > >  	const char *next;
> > >  
> > > diff --git a/include/fat.h b/include/fat.h
> > > index 0c88b59a4a..577e6b4592 100644
> > > --- a/include/fat.h
> > > +++ b/include/fat.h
> > > @@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
> > >  	return (sect - fsdata->data_begin) / fsdata->clust_size;
> > >  }
> > >  
> > > +/*
> > > + * Directory iterator
> > > + */
> > > +
> > > +#define TYPE_FILE 0x1
> > > +#define TYPE_DIR  0x2
> > > +#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> > 
> > Please, rename these to something more specific like FS_ITER_ANY.
> > 
> > TYPE_ANY already is defined in fs/reiserfs/reiserfs_private.h:
> > 
> > fs/reiserfs/reiserfs_private.h:160:#define TYPE_ANY 15
> 
> Good catch though the definition above comes directly from Rob's
> original patch.

Oops, looking at the code closely, the duplication will never be hit
because reiserfs_private.h is a private header.

> Since there is only one occurrence of TYPE_ANY in fs/fat,
> I'd rather expand it without introducing another definition.

I will put this change on hold unless a maintainer sticks to this idea.

> Thanks,
> -Takahiro AKASHI
> 
> 
> > Best regards
> > 
> > Heinrich
> > 
> > > +
> > > +typedef struct {
> > > +	fsdata    *fsdata;        /* filesystem parameters */
> > > +	unsigned   clust;         /* current cluster */
> > > +	int        last_cluster;  /* set once we've read last cluster */
> > > +	int        is_root;       /* is iterator at root directory */
> > > +	int        remaining;     /* remaining dent's in current cluster */
> > > +
> > > +	/* current iterator position values: */
> > > +	dir_entry *dent;          /* current directory entry */
> > > +	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > > +	char       s_name[14];    /* short 8.3 name */
> > > +	char      *name;          /* l_name if there is one, else s_name */
> > > +
> > > +	/* storage for current cluster in memory: */
> > > +	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > > +} fat_itr;
> > > +
> > > +int fat_itr_root(fat_itr *itr, fsdata *fsdata);
> > > +void fat_itr_child(fat_itr *itr, fat_itr *parent);
> > > +void *next_cluster(fat_itr *itr);
> > > +int fat_itr_next(fat_itr *itr);
> > > +int fat_itr_isdir(fat_itr *itr);
> > > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
> > > +
> > >  int file_fat_detectfs(void);
> > >  int fat_exists(const char *filename);
> > >  int fat_size(const char *filename, loff_t *size);
> > > 
> > 

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> Directory iterator was introduced in major re-work of read operation by
> Rob. We want to use it for write operation extensively as well.
> This patch makes relevant functions, as well as iterator defition, visible
> outside of fat.c.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fat/fat.c  | 39 ++++++---------------------------------
>  include/fat.h | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index fd6523c66b..0f82cbe1bd 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
>   * For more complete example, see fat_itr_resolve()
>   */
>  
> -typedef struct {
> -	fsdata    *fsdata;        /* filesystem parameters */
> -	unsigned   clust;         /* current cluster */
> -	int        last_cluster;  /* set once we've read last cluster */
> -	int        is_root;       /* is iterator at root directory */
> -	int        remaining;     /* remaining dent's in current cluster */
> -
> -	/* current iterator position values: */
> -	dir_entry *dent;          /* current directory entry */
> -	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> -	char       s_name[14];    /* short 8.3 name */
> -	char      *name;          /* l_name if there is one, else s_name */
> -
> -	/* storage for current cluster in memory: */
> -	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> -} fat_itr;
> -
> -static int fat_itr_isdir(fat_itr *itr);
> -
>  /**
>   * fat_itr_root() - initialize an iterator to start at the root
>   * directory
> @@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
>   * @fsdata: filesystem data for the partition
>   * @return 0 on success, else -errno
>   */
> -static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> +int fat_itr_root(fat_itr *itr, fsdata *fsdata)
>  {
>  	if (get_fs_info(fsdata))
>  		return -ENXIO;
> @@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
>   * @parent: the iterator pointing at a directory entry in the
>   *    parent directory of the directory to iterate
>   */
> -static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> +void fat_itr_child(fat_itr *itr, fat_itr *parent)
>  {
>  	fsdata *mydata = parent->fsdata;  /* for silly macros */
>  	unsigned clustnum = START(parent->dent);
> @@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
>  	itr->last_cluster = 0;
>  }
>  
> -static void *next_cluster(fat_itr *itr)
> +void *next_cluster(fat_itr *itr)
>  {
>  	fsdata *mydata = itr->fsdata;  /* for silly macros */
>  	int ret;
> @@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
>   * @return boolean, 1 if success or 0 if no more entries in the
>   *    current directory
>   */
> -static int fat_itr_next(fat_itr *itr)
> +int fat_itr_next(fat_itr *itr)
>  {
>  	dir_entry *dent;
>  
> @@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
>   * @itr: the iterator
>   * @return true if cursor is at a directory
>   */
> -static int fat_itr_isdir(fat_itr *itr)
> +int fat_itr_isdir(fat_itr *itr)
>  {
>  	return !!(itr->dent->attr & ATTR_DIR);
>  }
>  
> -/*
> - * Helpers:
> - */
> -
> -#define TYPE_FILE 0x1
> -#define TYPE_DIR  0x2
> -#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> -
>  /**
>   * fat_itr_resolve() - traverse directory structure to resolve the
>   * requested path.
> @@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
>   * @type: bitmask of allowable file types
>   * @return 0 on success or -errno
>   */
> -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
>  {
>  	const char *next;
>  
> diff --git a/include/fat.h b/include/fat.h
> index 0c88b59a4a..577e6b4592 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
>  	return (sect - fsdata->data_begin) / fsdata->clust_size;
>  }
>  
> +/*
> + * Directory iterator
> + */
> +
> +#define TYPE_FILE 0x1
> +#define TYPE_DIR  0x2
> +#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> +
> +typedef struct {
> +	fsdata    *fsdata;        /* filesystem parameters */
> +	unsigned   clust;         /* current cluster */
> +	int        last_cluster;  /* set once we've read last cluster */
> +	int        is_root;       /* is iterator at root directory */
> +	int        remaining;     /* remaining dent's in current cluster */
> +
> +	/* current iterator position values: */
> +	dir_entry *dent;          /* current directory entry */
> +	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> +	char       s_name[14];    /* short 8.3 name */
> +	char      *name;          /* l_name if there is one, else s_name */
> +
> +	/* storage for current cluster in memory: */
> +	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);

If CONFIG_FAT is not set, MAX_CLUSTSIZE is empty. And you get a build error:

  CC      fs/fs.o
In file included from fs/fs.c:12:0:
include/fat.h:20:23: error: ‘CONFIG_FS_FAT_MAX_CLUSTSIZE’ undeclared
here (not in a function); did you mean ‘CONFIG_SYS_MAX_FLASH_SECT’?
 #define MAX_CLUSTSIZE CONFIG_FS_FAT_MAX_CLUSTSIZE
                       ^
include/fat.h:214:19: note: in expansion of macro ‘MAX_CLUSTSIZE’
  u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
                   ^~~~~~~~~~~~~
make[1]: *** [scripts/Makefile.build:279: fs/fs.o] Error 1

We should not assume that every board uses FAT.

Best regards

Heinrich

> +} fat_itr;
> +
> +int fat_itr_root(fat_itr *itr, fsdata *fsdata);
> +void fat_itr_child(fat_itr *itr, fat_itr *parent);
> +void *next_cluster(fat_itr *itr);
> +int fat_itr_next(fat_itr *itr);
> +int fat_itr_isdir(fat_itr *itr);
> +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
> +
>  int file_fat_detectfs(void);
>  int fat_exists(const char *filename);
>  int fat_size(const char *filename, loff_t *size);
> 

[U-Boot] [PATCH 03/17] fs: fat: make directory iterator global for write use

[AKASHI Takahiro]

On Sat, Aug 11, 2018 at 03:34:20PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > Directory iterator was introduced in major re-work of read operation by
> > Rob. We want to use it for write operation extensively as well.
> > This patch makes relevant functions, as well as iterator defition, visible
> > outside of fat.c.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  fs/fat/fat.c  | 39 ++++++---------------------------------
> >  include/fat.h | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+), 33 deletions(-)
> > 
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index fd6523c66b..0f82cbe1bd 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -634,25 +634,6 @@ static int get_fs_info(fsdata *mydata)
> >   * For more complete example, see fat_itr_resolve()
> >   */
> >  
> > -typedef struct {
> > -	fsdata    *fsdata;        /* filesystem parameters */
> > -	unsigned   clust;         /* current cluster */
> > -	int        last_cluster;  /* set once we've read last cluster */
> > -	int        is_root;       /* is iterator at root directory */
> > -	int        remaining;     /* remaining dent's in current cluster */
> > -
> > -	/* current iterator position values: */
> > -	dir_entry *dent;          /* current directory entry */
> > -	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > -	char       s_name[14];    /* short 8.3 name */
> > -	char      *name;          /* l_name if there is one, else s_name */
> > -
> > -	/* storage for current cluster in memory: */
> > -	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > -} fat_itr;
> > -
> > -static int fat_itr_isdir(fat_itr *itr);
> > -
> >  /**
> >   * fat_itr_root() - initialize an iterator to start at the root
> >   * directory
> > @@ -661,7 +642,7 @@ static int fat_itr_isdir(fat_itr *itr);
> >   * @fsdata: filesystem data for the partition
> >   * @return 0 on success, else -errno
> >   */
> > -static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> > +int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> >  {
> >  	if (get_fs_info(fsdata))
> >  		return -ENXIO;
> > @@ -693,7 +674,7 @@ static int fat_itr_root(fat_itr *itr, fsdata *fsdata)
> >   * @parent: the iterator pointing at a directory entry in the
> >   *    parent directory of the directory to iterate
> >   */
> > -static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> > +void fat_itr_child(fat_itr *itr, fat_itr *parent)
> >  {
> >  	fsdata *mydata = parent->fsdata;  /* for silly macros */
> >  	unsigned clustnum = START(parent->dent);
> > @@ -713,7 +694,7 @@ static void fat_itr_child(fat_itr *itr, fat_itr *parent)
> >  	itr->last_cluster = 0;
> >  }
> >  
> > -static void *next_cluster(fat_itr *itr)
> > +void *next_cluster(fat_itr *itr)
> >  {
> >  	fsdata *mydata = itr->fsdata;  /* for silly macros */
> >  	int ret;
> > @@ -834,7 +815,7 @@ static dir_entry *extract_vfat_name(fat_itr *itr)
> >   * @return boolean, 1 if success or 0 if no more entries in the
> >   *    current directory
> >   */
> > -static int fat_itr_next(fat_itr *itr)
> > +int fat_itr_next(fat_itr *itr)
> >  {
> >  	dir_entry *dent;
> >  
> > @@ -879,19 +860,11 @@ static int fat_itr_next(fat_itr *itr)
> >   * @itr: the iterator
> >   * @return true if cursor is at a directory
> >   */
> > -static int fat_itr_isdir(fat_itr *itr)
> > +int fat_itr_isdir(fat_itr *itr)
> >  {
> >  	return !!(itr->dent->attr & ATTR_DIR);
> >  }
> >  
> > -/*
> > - * Helpers:
> > - */
> > -
> > -#define TYPE_FILE 0x1
> > -#define TYPE_DIR  0x2
> > -#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> > -
> >  /**
> >   * fat_itr_resolve() - traverse directory structure to resolve the
> >   * requested path.
> > @@ -907,7 +880,7 @@ static int fat_itr_isdir(fat_itr *itr)
> >   * @type: bitmask of allowable file types
> >   * @return 0 on success or -errno
> >   */
> > -static int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
> >  {
> >  	const char *next;
> >  
> > diff --git a/include/fat.h b/include/fat.h
> > index 0c88b59a4a..577e6b4592 100644
> > --- a/include/fat.h
> > +++ b/include/fat.h
> > @@ -187,6 +187,38 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
> >  	return (sect - fsdata->data_begin) / fsdata->clust_size;
> >  }
> >  
> > +/*
> > + * Directory iterator
> > + */
> > +
> > +#define TYPE_FILE 0x1
> > +#define TYPE_DIR  0x2
> > +#define TYPE_ANY  (TYPE_FILE | TYPE_DIR)
> > +
> > +typedef struct {
> > +	fsdata    *fsdata;        /* filesystem parameters */
> > +	unsigned   clust;         /* current cluster */
> > +	int        last_cluster;  /* set once we've read last cluster */
> > +	int        is_root;       /* is iterator at root directory */
> > +	int        remaining;     /* remaining dent's in current cluster */
> > +
> > +	/* current iterator position values: */
> > +	dir_entry *dent;          /* current directory entry */
> > +	char       l_name[VFAT_MAXLEN_BYTES];    /* long (vfat) name */
> > +	char       s_name[14];    /* short 8.3 name */
> > +	char      *name;          /* l_name if there is one, else s_name */
> > +
> > +	/* storage for current cluster in memory: */
> > +	u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> 
> If CONFIG_FAT is not set, MAX_CLUSTSIZE is empty. And you get a build error:
> 
>   CC      fs/fs.o
> In file included from fs/fs.c:12:0:
> include/fat.h:20:23: error: ‘CONFIG_FS_FAT_MAX_CLUSTSIZE’ undeclared
> here (not in a function); did you mean ‘CONFIG_SYS_MAX_FLASH_SECT’?
>  #define MAX_CLUSTSIZE CONFIG_FS_FAT_MAX_CLUSTSIZE
>                        ^
> include/fat.h:214:19: note: in expansion of macro ‘MAX_CLUSTSIZE’
>   u8         block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
>                    ^~~~~~~~~~~~~
> make[1]: *** [scripts/Makefile.build:279: fs/fs.o] Error 1

Thank you for catching this.

> We should not assume that every board uses FAT.

The contents of include/fat.h is totally private to fat implementation.
So I'd like to guard this include file like:

#ifndef _FAT_H
#define _FAT_H

#ifdef CONFIG_FS_FAT

<fat definitions>...

#endif /* CONFIG_FS_FAT */

#endif /* _FAT_H */

-Takahiro AKASHI

> Best regards
> 
> Heinrich
> 
> > +} fat_itr;
> > +
> > +int fat_itr_root(fat_itr *itr, fsdata *fsdata);
> > +void fat_itr_child(fat_itr *itr, fat_itr *parent);
> > +void *next_cluster(fat_itr *itr);
> > +int fat_itr_next(fat_itr *itr);
> > +int fat_itr_isdir(fat_itr *itr);
> > +int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type);
> > +
> >  int file_fat_detectfs(void);
> >  int fat_exists(const char *filename);
> >  int fat_size(const char *filename, loff_t *size);
> > 
> 

[U-Boot] [PATCH 04/17] fs: fat: assure iterator's ->dent belongs to ->clust

[AKASHI Takahiro]

In my attempt to re-work write operation, it was revealed that iterator's
"clust" does not always point to a cluster to which a current directory
entry ("dent") belongs.
This patch assures that it is always true by adding "next_clust" which is
used solely for dereferencing a cluster chain.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c  | 24 ++++++++++++++++--------
 include/fat.h |  1 +
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 0f82cbe1bd..d9bfb08d97 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -649,6 +649,7 @@ int fat_itr_root(fat_itr *itr, fsdata *fsdata)
 
 	itr->fsdata = fsdata;
 	itr->clust = fsdata->root_cluster;
+	itr->next_clust = fsdata->root_cluster;
 	itr->dent = NULL;
 	itr->remaining = 0;
 	itr->last_cluster = 0;
@@ -684,9 +685,11 @@ void fat_itr_child(fat_itr *itr, fat_itr *parent)
 	itr->fsdata = parent->fsdata;
 	if (clustnum > 0) {
 		itr->clust = clustnum;
+		itr->next_clust = clustnum;
 		itr->is_root = 0;
 	} else {
 		itr->clust = parent->fsdata->root_cluster;
+		itr->next_clust = parent->fsdata->root_cluster;
 		itr->is_root = 1;
 	}
 	itr->dent = NULL;
@@ -704,7 +707,7 @@ void *next_cluster(fat_itr *itr)
 	if (itr->last_cluster)
 		return NULL;
 
-	sect = clust_to_sect(itr->fsdata, itr->clust);
+	sect = clust_to_sect(itr->fsdata, itr->next_clust);
 
 	debug("FAT read(sect=%d), clust_size=%d, DIRENTSPERBLOCK=%zd\n",
 	      sect, itr->fsdata->clust_size, DIRENTSPERBLOCK);
@@ -725,18 +728,19 @@ void *next_cluster(fat_itr *itr)
 		return NULL;
 	}
 
+	itr->clust = itr->next_clust;
 	if (itr->is_root && itr->fsdata->fatsize != 32) {
-		itr->clust++;
-		sect = clust_to_sect(itr->fsdata, itr->clust);
+		itr->next_clust++;
+		sect = clust_to_sect(itr->fsdata, itr->next_clust);
 		if (sect - itr->fsdata->rootdir_sect >=
 		    itr->fsdata->rootdir_size) {
-			debug("cursect: 0x%x\n", itr->clust);
+			debug("nextclust: 0x%x\n", itr->next_clust);
 			itr->last_cluster = 1;
 		}
 	} else {
-		itr->clust = get_fatent(itr->fsdata, itr->clust);
-		if (CHECK_CLUST(itr->clust, itr->fsdata->fatsize)) {
-			debug("cursect: 0x%x\n", itr->clust);
+		itr->next_clust = get_fatent(itr->fsdata, itr->next_clust);
+		if (CHECK_CLUST(itr->next_clust, itr->fsdata->fatsize)) {
+			debug("nextclust: 0x%x\n", itr->next_clust);
 			itr->last_cluster = 1;
 		}
 	}
@@ -752,8 +756,11 @@ static dir_entry *next_dent(fat_itr *itr)
 			itr->fsdata->clust_size;
 
 		/* have we reached the last cluster? */
-		if (!dent)
+		if (!dent) {
+			/* a sign for no more entries left */
+			itr->dent = NULL;
 			return NULL;
+		}
 
 		itr->remaining = nbytes / sizeof(dir_entry) - 1;
 		itr->dent = dent;
@@ -906,6 +913,7 @@ int fat_itr_resolve(fat_itr *itr, const char *path, unsigned type)
 		    (((next - path) == 2) && !strncmp(path, "..", 2))) {
 			/* point back to itself */
 			itr->clust = itr->fsdata->root_cluster;
+			itr->next_clust = itr->fsdata->root_cluster;
 			itr->dent = NULL;
 			itr->remaining = 0;
 			itr->last_cluster = 0;
diff --git a/include/fat.h b/include/fat.h
index 577e6b4592..bc0f77abb5 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -198,6 +198,7 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
 typedef struct {
 	fsdata    *fsdata;        /* filesystem parameters */
 	unsigned   clust;         /* current cluster */
+	unsigned   next_clust;    /* next cluster if remaining == 0 */
 	int        last_cluster;  /* set once we've read last cluster */
 	int        is_root;       /* is iterator at root directory */
 	int        remaining;     /* remaining dent's in current cluster */
-- 
2.17.0

[U-Boot] [PATCH 05/17] fs: fat: check and normailze file name

[AKASHI Takahiro]

FAT file system's long file name support is a bit complicated and has some
restrictions on its naming. We should be careful about it especially for
write as it may easily end up with wrong file system.

normalize_longname() check for the rules and normalize a file name
if necessary. Please note, however, that this function is yet to be
extended to fully comply with the standard.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat_write.c | 52 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 3b77557b3e..6c715a70f4 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -899,6 +899,44 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect,
 	return NULL;
 }
 
+static int normalize_longname(char *l_filename, const char *filename)
+{
+	const char *p, legal[] = "!#$%&\'()-.@^`_{}~";
+	char c;
+	int name_len;
+
+	/* Check that the filename is valid */
+	for (p = filename; p < filename + strlen(filename); p++) {
+		c = *p;
+
+		if (('0' <= c) && (c <= '9'))
+			continue;
+		if (('A' <= c) && (c <= 'Z'))
+			continue;
+		if (('a' <= c) && (c <= 'z'))
+			continue;
+		if (strchr(legal, c))
+			continue;
+		/* extended code */
+		if ((0x80 <= c) && (c <= 0xff))
+			continue;
+
+		return -1;
+	}
+
+	/* Normalize it */
+	name_len = strlen(filename);
+	if (name_len >= VFAT_MAXLEN_BYTES)
+		/* should return an error? */
+		name_len = VFAT_MAXLEN_BYTES - 1;
+
+	memcpy(l_filename, filename, name_len);
+	l_filename[name_len] = 0; /* terminate the string */
+	downcase(l_filename, INT_MAX);
+
+	return 0;
+}
+
 static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			loff_t *actwrite)
 {
@@ -910,7 +948,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	fsdata datablock;
 	fsdata *mydata = &datablock;
 	int cursect;
-	int ret = -1, name_len;
+	int ret = -1;
 	char l_filename[VFAT_MAXLEN_BYTES];
 
 	*actwrite = size;
@@ -971,13 +1009,11 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	}
 	dentptr = (dir_entry *) do_fat_read_at_block;
 
-	name_len = strlen(filename);
-	if (name_len >= VFAT_MAXLEN_BYTES)
-		name_len = VFAT_MAXLEN_BYTES - 1;
-
-	memcpy(l_filename, filename, name_len);
-	l_filename[name_len] = 0; /* terminate the string */
-	downcase(l_filename, INT_MAX);
+	if (normalize_longname(l_filename, filename)) {
+		printf("FAT: illegal filename (%s)\n", filename);
+		ret = -EINVAL;
+		goto exit;
+	}
 
 	startsect = mydata->rootdir_sect;
 	retdent = find_directory_entry(mydata, startsect,
-- 
2.17.0

[U-Boot] [PATCH 05/17] fs: fat: check and normailze file name

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> FAT file system's long file name support is a bit complicated and has some
> restrictions on its naming. We should be careful about it especially for
> write as it may easily end up with wrong file system.
> 
> normalize_longname() check for the rules and normalize a file name
> if necessary. Please note, however, that this function is yet to be
> extended to fully comply with the standard.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

The patch has to be rebased on current master due to

fs: fat: cannot write to subdirectories
0dc1bfb7302d220a48364263d5632d6d572b069b

which has been merged into U-Boot master.

> ---
>  fs/fat/fat_write.c | 52 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 3b77557b3e..6c715a70f4 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -899,6 +899,44 @@ static dir_entry *find_directory_entry(fsdata *mydata, int startsect,
>  	return NULL;
>  }
>  
> +static int normalize_longname(char *l_filename, const char *filename)
> +{
> +	const char *p, legal[] = "!#$%&\'()-.@^`_{}~";

The logic below gets simpler if you define the illegal characters like
in the aforementioned patch.

Best regards

Heinrich

> +	char c;
> +	int name_len;
> +
> +	/* Check that the filename is valid */
> +	for (p = filename; p < filename + strlen(filename); p++) {
> +		c = *p;
> +
> +		if (('0' <= c) && (c <= '9'))
> +			continue;
> +		if (('A' <= c) && (c <= 'Z'))
> +			continue;
> +		if (('a' <= c) && (c <= 'z'))
> +			continue;
> +		if (strchr(legal, c))
> +			continue;
> +		/* extended code */
> +		if ((0x80 <= c) && (c <= 0xff))
> +			continue;
> +
> +		return -1;
> +	}
> +
<snip />

[U-Boot] [PATCH 06/17] fs: fat: write returns error code instead of -1

[AKASHI Takahiro]

It would be good that FAT write function return error code instead of
just returning -1 as fat_read_file() does.
This patch attempts to address this issue although it is 'best effort
(or estimate)' for now.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat_write.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 6c715a70f4..1e4f5af910 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -956,7 +956,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 
 	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
 		debug("error: reading boot sector\n");
-		return -1;
+		return -EIO;
 	}
 
 	total_sector = bs.total_sect;
@@ -997,7 +997,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	mydata->fatbuf = memalign(ARCH_DMA_MINALIGN, FATBUFSIZE);
 	if (mydata->fatbuf == NULL) {
 		debug("Error: allocating memory\n");
-		return -1;
+		return -ENOMEM;
 	}
 
 	if (disk_read(cursect,
@@ -1005,6 +1005,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 		(mydata->clust_size) :
 		PREFETCH_BLOCKS, do_fat_read_at_block) < 0) {
 		debug("Error: reading rootdir block\n");
+		ret = -EIO;
 		goto exit;
 	}
 	dentptr = (dir_entry *) do_fat_read_at_block;
@@ -1029,6 +1030,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 							size);
 				if (ret) {
 					printf("Error: %llu overflow\n", size);
+					ret = -ENOSPC;
 					goto exit;
 				}
 			}
@@ -1036,6 +1038,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			ret = clear_fatent(mydata, start_cluster);
 			if (ret) {
 				printf("Error: clearing FAT entries\n");
+				ret = -EIO;
 				goto exit;
 			}
 
@@ -1045,12 +1048,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			ret = start_cluster = find_empty_cluster(mydata);
 			if (ret < 0) {
 				printf("Error: finding empty cluster\n");
+				ret = -ENOSPC;
 				goto exit;
 			}
 
 			ret = check_overflow(mydata, start_cluster, size);
 			if (ret) {
 				printf("Error: %llu overflow\n", size);
+				ret = -ENOSPC;
 				goto exit;
 			}
 
@@ -1065,12 +1070,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			ret = start_cluster = find_empty_cluster(mydata);
 			if (ret < 0) {
 				printf("Error: finding empty cluster\n");
+				ret = -ENOSPC;
 				goto exit;
 			}
 
 			ret = check_overflow(mydata, start_cluster, size);
 			if (ret) {
 				printf("Error: %llu overflow\n", size);
+				ret = -ENOSPC;
 				goto exit;
 			}
 		} else {
@@ -1087,6 +1094,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	ret = set_contents(mydata, retdent, buffer, size, actwrite);
 	if (ret < 0) {
 		printf("Error: writing contents\n");
+		ret = -EIO;
 		goto exit;
 	}
 	debug("attempt to write 0x%llx bytes\n", *actwrite);
@@ -1095,14 +1103,17 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	ret = flush_dirty_fat_buffer(mydata);
 	if (ret) {
 		printf("Error: flush fat buffer\n");
+		ret = -EIO;
 		goto exit;
 	}
 
 	/* Write directory table to device */
 	ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block,
 			mydata->clust_size * mydata->sect_size);
-	if (ret)
+	if (ret) {
 		printf("Error: writing directory entry\n");
+		ret = -EIO;
+	}
 
 exit:
 	free(mydata->fatbuf);
@@ -1114,7 +1125,7 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
 {
 	if (offset != 0) {
 		printf("Error: non zero offset is currently not supported.\n");
-		return -1;
+		return -EINVAL;
 	}
 
 	printf("writing %s\n", filename);
-- 
2.17.0

[U-Boot] [PATCH 06/17] fs: fat: write returns error code instead of -1

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> It would be good that FAT write function return error code instead of
> just returning -1 as fat_read_file() does.
> This patch attempts to address this issue although it is 'best effort
> (or estimate)' for now.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Some of the error message are written with debug() others with printf().

Shouldn't we switch everything to debug() so that the EFI console is not
messed up?

Best regards

Heinrich

> ---
>  fs/fat/fat_write.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 6c715a70f4..1e4f5af910 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -956,7 +956,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  
>  	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
>  		debug("error: reading boot sector\n");
> -		return -1;
> +		return -EIO;
>  	}
>  
>  	total_sector = bs.total_sect;
> @@ -997,7 +997,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  	mydata->fatbuf = memalign(ARCH_DMA_MINALIGN, FATBUFSIZE);
>  	if (mydata->fatbuf == NULL) {
>  		debug("Error: allocating memory\n");
> -		return -1;
> +		return -ENOMEM;
>  	}
>  
>  	if (disk_read(cursect,
> @@ -1005,6 +1005,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  		(mydata->clust_size) :
>  		PREFETCH_BLOCKS, do_fat_read_at_block) < 0) {
>  		debug("Error: reading rootdir block\n");
> +		ret = -EIO;
>  		goto exit;
>  	}
>  	dentptr = (dir_entry *) do_fat_read_at_block;
> @@ -1029,6 +1030,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  							size);
>  				if (ret) {
>  					printf("Error: %llu overflow\n", size);
> +					ret = -ENOSPC;
>  					goto exit;
>  				}
>  			}
> @@ -1036,6 +1038,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  			ret = clear_fatent(mydata, start_cluster);
>  			if (ret) {
>  				printf("Error: clearing FAT entries\n");
> +				ret = -EIO;
>  				goto exit;
>  			}
>  
> @@ -1045,12 +1048,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  			ret = start_cluster = find_empty_cluster(mydata);
>  			if (ret < 0) {
>  				printf("Error: finding empty cluster\n");
> +				ret = -ENOSPC;
>  				goto exit;
>  			}
>  
>  			ret = check_overflow(mydata, start_cluster, size);
>  			if (ret) {
>  				printf("Error: %llu overflow\n", size);
> +				ret = -ENOSPC;
>  				goto exit;
>  			}
>  
> @@ -1065,12 +1070,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  			ret = start_cluster = find_empty_cluster(mydata);
>  			if (ret < 0) {
>  				printf("Error: finding empty cluster\n");
> +				ret = -ENOSPC;
>  				goto exit;
>  			}
>  
>  			ret = check_overflow(mydata, start_cluster, size);
>  			if (ret) {
>  				printf("Error: %llu overflow\n", size);
> +				ret = -ENOSPC;
>  				goto exit;
>  			}
>  		} else {
> @@ -1087,6 +1094,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  	ret = set_contents(mydata, retdent, buffer, size, actwrite);
>  	if (ret < 0) {
>  		printf("Error: writing contents\n");
> +		ret = -EIO;
>  		goto exit;
>  	}
>  	debug("attempt to write 0x%llx bytes\n", *actwrite);
> @@ -1095,14 +1103,17 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
>  	ret = flush_dirty_fat_buffer(mydata);
>  	if (ret) {
>  		printf("Error: flush fat buffer\n");
> +		ret = -EIO;
>  		goto exit;
>  	}
>  
>  	/* Write directory table to device */
>  	ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block,
>  			mydata->clust_size * mydata->sect_size);
> -	if (ret)
> +	if (ret) {
>  		printf("Error: writing directory entry\n");
> +		ret = -EIO;
> +	}
>  
>  exit:
>  	free(mydata->fatbuf);
> @@ -1114,7 +1125,7 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
>  {
>  	if (offset != 0) {
>  		printf("Error: non zero offset is currently not supported.\n");
> -		return -1;
> +		return -EINVAL;
>  	}
>  
>  	printf("writing %s\n", filename);
> 

[U-Boot] [PATCH 06/17] fs: fat: write returns error code instead of -1

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 07:55:06PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > It would be good that FAT write function return error code instead of
> > just returning -1 as fat_read_file() does.
> > This patch attempts to address this issue although it is 'best effort
> > (or estimate)' for now.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Some of the error message are written with debug() others with printf().

Right, but as you know, there is already a mixture of debug() and
printf() in the existing code.
Here I tried to do my best to use either one so as to make the usage
consistent with existing code though I don't know the exact discipline.

> Shouldn't we switch everything to debug() so that the EFI console is not
> messed up?

Is this a different issue, isn't it?
(I suppose that one of Rob's patches tried to address this issue.)

So I'd like to listen to any opinion from other folks, too.

Thanks,
-Takahiro AKASHI

> Best regards
> 
> Heinrich
> 
> > ---
> >  fs/fat/fat_write.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 6c715a70f4..1e4f5af910 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -956,7 +956,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  
> >  	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
> >  		debug("error: reading boot sector\n");
> > -		return -1;
> > +		return -EIO;
> >  	}
> >  
> >  	total_sector = bs.total_sect;
> > @@ -997,7 +997,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  	mydata->fatbuf = memalign(ARCH_DMA_MINALIGN, FATBUFSIZE);
> >  	if (mydata->fatbuf == NULL) {
> >  		debug("Error: allocating memory\n");
> > -		return -1;
> > +		return -ENOMEM;
> >  	}
> >  
> >  	if (disk_read(cursect,
> > @@ -1005,6 +1005,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  		(mydata->clust_size) :
> >  		PREFETCH_BLOCKS, do_fat_read_at_block) < 0) {
> >  		debug("Error: reading rootdir block\n");
> > +		ret = -EIO;
> >  		goto exit;
> >  	}
> >  	dentptr = (dir_entry *) do_fat_read_at_block;
> > @@ -1029,6 +1030,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  							size);
> >  				if (ret) {
> >  					printf("Error: %llu overflow\n", size);
> > +					ret = -ENOSPC;
> >  					goto exit;
> >  				}
> >  			}
> > @@ -1036,6 +1038,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  			ret = clear_fatent(mydata, start_cluster);
> >  			if (ret) {
> >  				printf("Error: clearing FAT entries\n");
> > +				ret = -EIO;
> >  				goto exit;
> >  			}
> >  
> > @@ -1045,12 +1048,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  			ret = start_cluster = find_empty_cluster(mydata);
> >  			if (ret < 0) {
> >  				printf("Error: finding empty cluster\n");
> > +				ret = -ENOSPC;
> >  				goto exit;
> >  			}
> >  
> >  			ret = check_overflow(mydata, start_cluster, size);
> >  			if (ret) {
> >  				printf("Error: %llu overflow\n", size);
> > +				ret = -ENOSPC;
> >  				goto exit;
> >  			}
> >  
> > @@ -1065,12 +1070,14 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  			ret = start_cluster = find_empty_cluster(mydata);
> >  			if (ret < 0) {
> >  				printf("Error: finding empty cluster\n");
> > +				ret = -ENOSPC;
> >  				goto exit;
> >  			}
> >  
> >  			ret = check_overflow(mydata, start_cluster, size);
> >  			if (ret) {
> >  				printf("Error: %llu overflow\n", size);
> > +				ret = -ENOSPC;
> >  				goto exit;
> >  			}
> >  		} else {
> > @@ -1087,6 +1094,7 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  	ret = set_contents(mydata, retdent, buffer, size, actwrite);
> >  	if (ret < 0) {
> >  		printf("Error: writing contents\n");
> > +		ret = -EIO;
> >  		goto exit;
> >  	}
> >  	debug("attempt to write 0x%llx bytes\n", *actwrite);
> > @@ -1095,14 +1103,17 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
> >  	ret = flush_dirty_fat_buffer(mydata);
> >  	if (ret) {
> >  		printf("Error: flush fat buffer\n");
> > +		ret = -EIO;
> >  		goto exit;
> >  	}
> >  
> >  	/* Write directory table to device */
> >  	ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block,
> >  			mydata->clust_size * mydata->sect_size);
> > -	if (ret)
> > +	if (ret) {
> >  		printf("Error: writing directory entry\n");
> > +		ret = -EIO;
> > +	}
> >  
> >  exit:
> >  	free(mydata->fatbuf);
> > @@ -1114,7 +1125,7 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
> >  {
> >  	if (offset != 0) {
> >  		printf("Error: non zero offset is currently not supported.\n");
> > -		return -1;
> > +		return -EINVAL;
> >  	}
> >  
> >  	printf("writing %s\n", filename);
> > 
> 

[U-Boot] [PATCH 07/17] fs: fat: support write with sub-directory path

[AKASHI Takahiro]

In this patch, write implementation is overhauled and rewritten by
making full use of directory iterator. The ovbious bonus is that we are
now able to write to a file with a directory path, like /A/B/C/FILE.

Please note that, as there is no notion of "current directory" on u-boot,
a file name specified must contain an absolute directory path. Otherwise,
"/" (root directory) is assumed.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c       |   9 -
 fs/fat/fat_write.c | 469 +++++++++++++++------------------------------
 2 files changed, 157 insertions(+), 321 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index d9bfb08d97..9bafc3a40c 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -464,15 +464,6 @@ static __u8 mkcksum(const char name[8], const char ext[3])
 	return ret;
 }
 
-/*
- * TODO these should go away once fat_write is reworked to use the
- * directory iterator
- */
-__u8 get_dentfromdir_block[MAX_CLUSTSIZE]
-	__aligned(ARCH_DMA_MINALIGN);
-__u8 do_fat_read_at_block[MAX_CLUSTSIZE]
-	__aligned(ARCH_DMA_MINALIGN);
-
 /*
  * Read boot sector and volume info from a FAT filesystem
  */
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 1e4f5af910..96edb6674c 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -99,7 +99,6 @@ static void set_name(dir_entry *dirent, const char *filename)
 	debug("ext : %s\n", dirent->ext);
 }
 
-static __u8 num_of_fats;
 /*
  * Write fat buffer into block device
  */
@@ -128,7 +127,7 @@ static int flush_dirty_fat_buffer(fsdata *mydata)
 		return -1;
 	}
 
-	if (num_of_fats == 2) {
+	if (mydata->bs_fats == 2) {
 		/* Update corresponding second FAT blocks */
 		startblock += mydata->fatlength;
 		if (disk_write(startblock, getsize, bufptr) < 0) {
@@ -210,15 +209,14 @@ name11_12:
 	return 1;
 }
 
-static int is_next_clust(fsdata *mydata, dir_entry *dentptr);
-static void flush_dir_table(fsdata *mydata, dir_entry **dentptr);
+static int flush_dir_table(fat_itr *itr);
 
 /*
  * Fill dir_slot entries with appropriate name, id, and attr
- * The real directory entry is returned by 'dentptr'
+ * 'itr' will point to a next entry
  */
-static void
-fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
+static int
+fill_dir_slot(fat_itr *itr, const char *l_name)
 {
 	__u8 temp_dir_slot_buffer[MAX_LFN_SLOT * sizeof(dir_slot)];
 	dir_slot *slotptr = (dir_slot *)temp_dir_slot_buffer;
@@ -226,7 +224,7 @@ fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 	int idx = 0, ret;
 
 	/* Get short file name checksum value */
-	checksum = mkcksum((*dentptr)->name, (*dentptr)->ext);
+	checksum = mkcksum(itr->dent->name, itr->dent->ext);
 
 	do {
 		memset(slotptr, 0x00, sizeof(dir_slot));
@@ -241,120 +239,21 @@ fill_dir_slot(fsdata *mydata, dir_entry **dentptr, const char *l_name)
 	slotptr->id |= LAST_LONG_ENTRY_MASK;
 
 	while (counter >= 1) {
-		if (is_next_clust(mydata, *dentptr)) {
-			/* A new cluster is allocated for directory table */
-			flush_dir_table(mydata, dentptr);
-		}
-		memcpy(*dentptr, slotptr, sizeof(dir_slot));
-		(*dentptr)++;
+		memcpy(itr->dent, slotptr, sizeof(dir_slot));
 		slotptr--;
 		counter--;
-	}
-
-	if (is_next_clust(mydata, *dentptr)) {
-		/* A new cluster is allocated for directory table */
-		flush_dir_table(mydata, dentptr);
-	}
-}
-
-static __u32 dir_curclust;
-
-/*
- * 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'
- * If additional adjacent cluster for directory entries is read into memory,
- * then 'get_contents_vfatname_block' is copied into 'get_dentfromdir_block' and
- * the location of the real directory entry is returned by 'retdent'
- * Return 0 on success, -1 otherwise.
- */
-static int
-get_long_file_name(fsdata *mydata, int curclust, __u8 *cluster,
-	      dir_entry **retdent, char *l_name)
-{
-	dir_entry *realdent;
-	dir_slot *slotptr = (dir_slot *)(*retdent);
-	dir_slot *slotptr2 = NULL;
-	__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, cur_position = 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) {
-		if (curclust == 0)
-			return -1;
-		curclust = get_fatent(mydata, dir_curclust);
-		if (CHECK_CLUST(curclust, mydata->fatsize)) {
-			debug("curclust: 0x%x\n", curclust);
-			printf("Invalid FAT entry\n");
-			return -1;
-		}
-
-		dir_curclust = curclust;
-
-		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)
+		if (!fat_itr_next(itr))
+			if (!itr->dent && !itr->is_root && flush_dir_table(itr))
 				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, &idx);
-		}
-	} else {
-		/* Save the real directory entry */
-		realdent = (dir_entry *)slotptr;
 	}
 
-	do {
-		slotptr--;
-		if (slot2str(slotptr, l_name, &idx))
-			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, INT_MAX);
-
-	/* Return the real directory entry */
-	*retdent = realdent;
-
-	if (slotptr2) {
-		memcpy(get_dentfromdir_block, get_contents_vfatname_block,
-			mydata->clust_size * mydata->sect_size);
-		cur_position = (__u8 *)realdent - get_contents_vfatname_block;
-		*retdent = (dir_entry *) &get_dentfromdir_block[cur_position];
-	}
+	if (!itr->dent && !itr->is_root)
+		/*
+		 * don't care return value here because we have already
+		 * finished completing an entry with name, only ending up
+		 * no more entry left
+		 */
+		flush_dir_table(itr);
 
 	return 0;
 }
@@ -569,20 +468,20 @@ static int find_empty_cluster(fsdata *mydata)
 }
 
 /*
- * Write directory entries in 'get_dentfromdir_block' to block device
+ * Write directory entries in itr's buffer to block device
  */
-static void flush_dir_table(fsdata *mydata, dir_entry **dentptr)
+static int flush_dir_table(fat_itr *itr)
 {
+	fsdata *mydata = itr->fsdata;
 	int dir_newclust = 0;
+	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
 
-	if (set_cluster(mydata, dir_curclust,
-		    get_dentfromdir_block,
-		    mydata->clust_size * mydata->sect_size) != 0) {
-		printf("error: wrinting directory entry\n");
-		return;
+	if (set_cluster(mydata, itr->clust, itr->block, bytesperclust) != 0) {
+		printf("error: writing directory entry\n");
+		return -1;
 	}
 	dir_newclust = find_empty_cluster(mydata);
-	set_fatent_value(mydata, dir_curclust, dir_newclust);
+	set_fatent_value(mydata, itr->clust, dir_newclust);
 	if (mydata->fatsize == 32)
 		set_fatent_value(mydata, dir_newclust, 0xffffff8);
 	else if (mydata->fatsize == 16)
@@ -590,15 +489,19 @@ static void flush_dir_table(fsdata *mydata, dir_entry **dentptr)
 	else if (mydata->fatsize == 12)
 		set_fatent_value(mydata, dir_newclust, 0xff8);
 
-	dir_curclust = dir_newclust;
+	itr->clust = dir_newclust;
+	itr->next_clust = dir_newclust;
 
 	if (flush_dirty_fat_buffer(mydata) < 0)
-		return;
+		return -1;
+
+	memset(itr->block, 0x00, bytesperclust);
 
-	memset(get_dentfromdir_block, 0x00,
-		mydata->clust_size * mydata->sect_size);
+	itr->dent = (dir_entry *)itr->block;
+	itr->last_cluster = 1;
+	itr->remaining = bytesperclust / sizeof(dir_entry) - 1;
 
-	*dentptr = (dir_entry *) get_dentfromdir_block;
+	return 0;
 }
 
 /*
@@ -764,139 +667,83 @@ static int check_overflow(fsdata *mydata, __u32 clustnum, loff_t size)
 	return 0;
 }
 
-/*
- * Check if adding several entries exceed one cluster boundary
- */
-static int is_next_clust(fsdata *mydata, dir_entry *dentptr)
-{
-	int cur_position;
-
-	cur_position = (__u8 *)dentptr - get_dentfromdir_block;
-
-	if (cur_position >= mydata->clust_size * mydata->sect_size)
-		return 1;
-	else
-		return 0;
-}
-
-static dir_entry *empty_dentptr;
 /*
  * Find a directory entry based on filename or start cluster number
  * If the directory entry is not found,
  * the new position for writing a directory entry will be returned
  */
-static dir_entry *find_directory_entry(fsdata *mydata, int startsect,
-	char *filename, dir_entry *retdent, __u32 start)
+static dir_entry *find_directory_entry(fat_itr *itr, char *filename)
 {
-	__u32 curclust = sect_to_clust(mydata, startsect);
-
-	debug("get_dentfromdir: %s\n", filename);
+	int match = 0;
 
-	while (1) {
-		dir_entry *dentptr;
+	while (fat_itr_next(itr)) {
+		/* check both long and short name: */
+		if (!strcasecmp(filename, itr->name))
+			match = 1;
+		else if (itr->name != itr->s_name &&
+			 !strcasecmp(filename, itr->s_name))
+			match = 1;
 
-		int i;
+		if (!match)
+			continue;
 
-		if (get_cluster(mydata, curclust, get_dentfromdir_block,
-			    mydata->clust_size * mydata->sect_size) != 0) {
-			printf("Error: reading directory block\n");
+		if (itr->dent->name[0] == '\0')
 			return NULL;
-		}
-
-		dentptr = (dir_entry *)get_dentfromdir_block;
-
-		dir_curclust = curclust;
-
-		for (i = 0; i < DIRENTSPERCLUST; i++) {
-			char s_name[14], l_name[VFAT_MAXLEN_BYTES];
-
-			l_name[0] = '\0';
-			if (dentptr->name[0] == DELETED_FLAG) {
-				dentptr++;
-				if (is_next_clust(mydata, dentptr))
-					break;
-				continue;
-			}
-			if ((dentptr->attr & ATTR_VOLUME)) {
-				if ((dentptr->attr & ATTR_VFAT) &&
-				    (dentptr->name[0] & LAST_LONG_ENTRY_MASK)) {
-					get_long_file_name(mydata, curclust,
-						     get_dentfromdir_block,
-						     &dentptr, l_name);
-					debug("vfatname: |%s|\n", l_name);
-				} else {
-					/* Volume label or VFAT entry */
-					dentptr++;
-					if (is_next_clust(mydata, dentptr))
-						break;
-					continue;
-				}
-			}
-			if (dentptr->name[0] == 0) {
-				debug("Dentname == NULL - %d\n", i);
-				empty_dentptr = dentptr;
-				return NULL;
-			}
-
-			get_name(dentptr, s_name);
-
-			if (strncasecmp(filename, s_name, sizeof(s_name)) &&
-			    strncasecmp(filename, l_name, sizeof(l_name))) {
-				debug("Mismatch: |%s|%s|\n",
-					s_name, l_name);
-				dentptr++;
-				if (is_next_clust(mydata, dentptr))
-					break;
-				continue;
-			}
+		else
+			return itr->dent;
+	}
 
-			memcpy(retdent, dentptr, sizeof(dir_entry));
+	if (!itr->dent && !itr->is_root && flush_dir_table(itr))
+		/* indicate that allocating dent failed */
+		itr->dent = NULL;
 
-			debug("DentName: %s", s_name);
-			debug(", start: 0x%x", START(dentptr));
-			debug(", size:  0x%x %s\n",
-			      FAT2CPU32(dentptr->size),
-			      (dentptr->attr & ATTR_DIR) ?
-			      "(DIR)" : "");
+	return NULL;
+}
 
-			return dentptr;
+static int split_filename(char *filename, char **dirname, char **basename)
+{
+	char *p, *last_slash, *last_slash_cont;
+
+again:
+	p = filename;
+	last_slash = NULL;
+	last_slash_cont = NULL;
+	while (*p) {
+		if (ISDIRDELIM(*p)) {
+			last_slash = p;
+			last_slash_cont = p;
+			/* continuous slashes */
+			while (ISDIRDELIM(*p))
+				last_slash_cont = p++;
+			if (!*p)
+				break;
 		}
+		p++;
+	}
 
-		/*
-		 * In FAT16/12, the root dir is locate before data area, shows
-		 * in following:
-		 * -------------------------------------------------------------
-		 * | Boot | FAT1 & 2 | Root dir | Data (start from cluster #2) |
-		 * -------------------------------------------------------------
-		 *
-		 * As a result if curclust is in Root dir, it is a negative
-		 * number or 0, 1.
-		 *
-		 */
-		if (mydata->fatsize != 32 && (int)curclust <= 1) {
-			/* Current clust is in root dir, set to next clust */
-			curclust++;
-			if ((int)curclust <= 1)
-				continue;	/* continue to find */
-
-			/* Reach the end of root dir */
-			empty_dentptr = dentptr;
-			return NULL;
+	if (last_slash) {
+		if (last_slash_cont == (filename + strlen(filename) - 1)) {
+			/* remove trailing slashes */
+			*last_slash = '\0';
+			goto again;
 		}
 
-		curclust = get_fatent(mydata, dir_curclust);
-		if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
-			empty_dentptr = dentptr;
-			return NULL;
-		}
-		if (CHECK_CLUST(curclust, mydata->fatsize)) {
-			debug("curclust: 0x%x\n", curclust);
-			debug("Invalid FAT entry\n");
-			return NULL;
+		if (last_slash == filename) {
+			/* avoid ""(null) directory */
+			*dirname = "/";
+		} else {
+			*last_slash = '\0';
+			*dirname = filename;
 		}
+
+		*last_slash_cont = '\0';
+		*basename = last_slash_cont + 1;
+	} else {
+		*dirname = "/"; /* root by default */
+		*basename = filename;
 	}
 
-	return NULL;
+	return 0;
 }
 
 static int normalize_longname(char *l_filename, const char *filename)
@@ -940,86 +787,60 @@ static int normalize_longname(char *l_filename, const char *filename)
 static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			loff_t *actwrite)
 {
-	dir_entry *dentptr, *retdent;
-	__u32 startsect;
+	dir_entry *retdent;
 	__u32 start_cluster;
-	boot_sector bs;
-	volume_info volinfo;
-	fsdata datablock;
+	fsdata datablock = { .fatbuf = NULL, };
 	fsdata *mydata = &datablock;
-	int cursect;
+	fat_itr *itr = NULL;
 	int ret = -1;
+	char *filename_copy, *parent, *basename;
 	char l_filename[VFAT_MAXLEN_BYTES];
 
-	*actwrite = size;
-	dir_curclust = 0;
+	filename_copy = strdup(filename);
+	if (!filename_copy)
+		return -ENOMEM;
 
-	if (read_bootsectandvi(&bs, &volinfo, &mydata->fatsize)) {
-		debug("error: reading boot sector\n");
-		return -EIO;
+	split_filename(filename_copy, &parent, &basename);
+	if (!strlen(basename)) {
+		ret = -EINVAL;
+		goto exit;
 	}
 
-	total_sector = bs.total_sect;
-	if (total_sector == 0)
-		total_sector = (int)cur_part_info.size; /* cast of lbaint_t */
-
-	if (mydata->fatsize == 32)
-		mydata->fatlength = bs.fat32_length;
-	else
-		mydata->fatlength = bs.fat_length;
-
-	mydata->fat_sect = bs.reserved;
-
-	cursect = mydata->rootdir_sect
-		= mydata->fat_sect + mydata->fatlength * bs.fats;
-	num_of_fats = bs.fats;
-
-	mydata->sect_size = (bs.sector_size[1] << 8) + bs.sector_size[0];
-	mydata->clust_size = bs.cluster_size;
-
-	if (mydata->fatsize == 32) {
-		mydata->data_begin = mydata->rootdir_sect -
-					(mydata->clust_size * 2);
-	} else {
-		int rootdir_size;
-
-		rootdir_size = ((bs.dir_entries[1]  * (int)256 +
-				 bs.dir_entries[0]) *
-				 sizeof(dir_entry)) /
-				 mydata->sect_size;
-		mydata->data_begin = mydata->rootdir_sect +
-					rootdir_size -
-					(mydata->clust_size * 2);
+	filename = basename;
+	if (normalize_longname(l_filename, filename)) {
+		printf("FAT: illegal filename (%s)\n", filename);
+		ret = -EINVAL;
+		goto exit;
 	}
 
-	mydata->fatbufnum = -1;
-	mydata->fat_dirty = 0;
-	mydata->fatbuf = memalign(ARCH_DMA_MINALIGN, FATBUFSIZE);
-	if (mydata->fatbuf == NULL) {
-		debug("Error: allocating memory\n");
-		return -ENOMEM;
+	itr = malloc_cache_aligned(sizeof(fat_itr));
+	if (!itr) {
+		ret = -ENOMEM;
+		goto exit;
 	}
 
-	if (disk_read(cursect,
-		(mydata->fatsize == 32) ?
-		(mydata->clust_size) :
-		PREFETCH_BLOCKS, do_fat_read_at_block) < 0) {
-		debug("Error: reading rootdir block\n");
-		ret = -EIO;
+	ret = fat_itr_root(itr, &datablock);
+	if (ret)
 		goto exit;
-	}
-	dentptr = (dir_entry *) do_fat_read_at_block;
 
-	if (normalize_longname(l_filename, filename)) {
-		printf("FAT: illegal filename (%s)\n", filename);
-		ret = -EINVAL;
+	total_sector = datablock.bs_total_sect;
+	if (total_sector == 0)
+		total_sector = (int)cur_part_info.size; /* cast of lbaint_t */
+
+	ret = fat_itr_resolve(itr, parent, TYPE_DIR);
+	if (ret) {
+		printf("%s: doesn't exist (%d)\n", parent, ret);
 		goto exit;
 	}
 
-	startsect = mydata->rootdir_sect;
-	retdent = find_directory_entry(mydata, startsect,
-				l_filename, dentptr, 0);
+	retdent = find_directory_entry(itr, l_filename);
+
 	if (retdent) {
+		if (fat_itr_isdir(itr)) {
+			ret = -EISDIR;
+			goto exit;
+		}
+
 		/* Update file size and start_cluster in a directory entry */
 		retdent->size = cpu_to_le32(size);
 		start_cluster = START(retdent);
@@ -1062,9 +883,31 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			set_start_cluster(mydata, retdent, start_cluster);
 		}
 	} else {
+		/* Create a new file */
+
+		if (itr->is_root) {
+			/* root dir cannot have "." or ".." */
+			if (!strcmp(l_filename, ".") ||
+			    !strcmp(l_filename, "..")) {
+				ret = -EINVAL;
+				goto exit;
+			}
+		}
+
+		if (!itr->dent) {
+			printf("Error: allocating new dir entry\n");
+			ret = -EIO;
+			goto exit;
+		}
+
+		memset(itr->dent, 0, sizeof(*itr->dent));
+
 		/* Set short name to set alias checksum field in dir_slot */
-		set_name(empty_dentptr, filename);
-		fill_dir_slot(mydata, &empty_dentptr, filename);
+		set_name(itr->dent, filename);
+		if (fill_dir_slot(itr, filename)) {
+			ret = -EIO;
+			goto exit;
+		}
 
 		if (size) {
 			ret = start_cluster = find_empty_cluster(mydata);
@@ -1085,10 +928,10 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 		}
 
 		/* Set attribute as archieve for regular file */
-		fill_dentry(mydata, empty_dentptr, filename,
-			start_cluster, size, 0x20);
+		fill_dentry(itr->fsdata, itr->dent, filename,
+					start_cluster, size, 0x20);
 
-		retdent = empty_dentptr;
+		retdent = itr->dent;
 	}
 
 	ret = set_contents(mydata, retdent, buffer, size, actwrite);
@@ -1108,15 +951,17 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	}
 
 	/* Write directory table to device */
-	ret = set_cluster(mydata, dir_curclust, get_dentfromdir_block,
-			mydata->clust_size * mydata->sect_size);
+	ret = set_cluster(mydata, itr->clust, itr->block,
+				mydata->clust_size * mydata->sect_size);
 	if (ret) {
 		printf("Error: writing directory entry\n");
 		ret = -EIO;
 	}
 
 exit:
+	free(filename_copy);
 	free(mydata->fatbuf);
+	free(itr);
 	return ret;
 }
 
-- 
2.17.0

[U-Boot] [PATCH 08/17] fs: fat: refactor write interface for a file offset

[AKASHI Takahiro]

The current write implementation is quite simple: remove existing clusters
and then allocating new ones and filling them with data. This, inevitably,
enforces always writing from the beginning of a file.

As the first step to lift this restriction, fat_file_write() and
set_contents() are modified to accept an additional parameter, file offset
and further re-factored so that, in the next patch, all the necessary code
will be put into set_contents().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat_write.c | 178 +++++++++++++++++----------------------------
 1 file changed, 65 insertions(+), 113 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 96edb6674c..3a9c53e253 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -528,6 +528,43 @@ static int clear_fatent(fsdata *mydata, __u32 entry)
 	return 0;
 }
 
+/*
+ * Set start cluster in directory entry
+ */
+static void set_start_cluster(const fsdata *mydata, dir_entry *dentptr,
+				__u32 start_cluster)
+{
+	if (mydata->fatsize == 32)
+		dentptr->starthi =
+			cpu_to_le16((start_cluster & 0xffff0000) >> 16);
+	dentptr->start = cpu_to_le16(start_cluster & 0xffff);
+}
+
+/*
+ * Check whether adding a file makes the file system to
+ * exceed the size of the block device
+ * Return -1 when overflow occurs, otherwise return 0
+ */
+static int check_overflow(fsdata *mydata, __u32 clustnum, loff_t size)
+{
+	__u32 startsect, sect_num, offset;
+
+	if (clustnum > 0) {
+		startsect = clust_to_sect(mydata, clustnum);
+	} else {
+		startsect = mydata->rootdir_sect;
+	}
+
+	sect_num = div_u64_rem(size, mydata->sect_size, &offset);
+
+	if (offset != 0)
+		sect_num++;
+
+	if (startsect + sect_num > total_sector)
+		return -1;
+	return 0;
+}
+
 /*
  * Write at most 'maxsize' bytes from 'buffer' into
  * the file associated with 'dentptr'
@@ -535,29 +572,36 @@ static int clear_fatent(fsdata *mydata, __u32 entry)
  * or return -1 on fatal errors.
  */
 static int
-set_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer,
+set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 	      loff_t maxsize, loff_t *gotsize)
 {
-	loff_t filesize = FAT2CPU32(dentptr->size);
+	loff_t filesize;
 	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
 	__u32 curclust = START(dentptr);
 	__u32 endclust = 0, newclust = 0;
 	loff_t actsize;
 
 	*gotsize = 0;
-	debug("Filesize: %llu bytes\n", filesize);
-
-	if (maxsize > 0 && filesize > maxsize)
-		filesize = maxsize;
+	filesize = maxsize;
 
 	debug("%llu bytes\n", filesize);
 
-	if (!curclust) {
-		if (filesize) {
-			debug("error: nonempty clusterless file!\n");
+	if (curclust) {
+		/*
+		 * release already-allocated clusters anyway
+		 */
+		if (clear_fatent(mydata, curclust)) {
+			printf("Error: clearing FAT entries\n");
 			return -1;
 		}
-		return 0;
+	}
+
+	curclust = find_empty_cluster(mydata);
+	set_start_cluster(mydata, dentptr, curclust);
+
+	if (check_overflow(mydata, curclust, filesize)) {
+		printf("Error: no space left: %llu\n", filesize);
+		return -1;
 	}
 
 	actsize = bytesperclust;
@@ -568,6 +612,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, __u8 *buffer,
 			newclust = determine_fatent(mydata, endclust);
 
 			if ((newclust - 1) != endclust)
+				/* write to <curclust..endclust> */
 				goto getit;
 
 			if (CHECK_CLUST(newclust, mydata->fatsize)) {
@@ -614,18 +659,8 @@ getit:
 		actsize = bytesperclust;
 		curclust = endclust = newclust;
 	} while (1);
-}
 
-/*
- * Set start cluster in directory entry
- */
-static void set_start_cluster(const fsdata *mydata, dir_entry *dentptr,
-				__u32 start_cluster)
-{
-	if (mydata->fatsize == 32)
-		dentptr->starthi =
-			cpu_to_le16((start_cluster & 0xffff0000) >> 16);
-	dentptr->start = cpu_to_le16(start_cluster & 0xffff);
+	return 0;
 }
 
 /*
@@ -642,31 +677,6 @@ static void fill_dentry(fsdata *mydata, dir_entry *dentptr,
 	set_name(dentptr, filename);
 }
 
-/*
- * Check whether adding a file makes the file system to
- * exceed the size of the block device
- * Return -1 when overflow occurs, otherwise return 0
- */
-static int check_overflow(fsdata *mydata, __u32 clustnum, loff_t size)
-{
-	__u32 startsect, sect_num, offset;
-
-	if (clustnum > 0) {
-		startsect = clust_to_sect(mydata, clustnum);
-	} else {
-		startsect = mydata->rootdir_sect;
-	}
-
-	sect_num = div_u64_rem(size, mydata->sect_size, &offset);
-
-	if (offset != 0)
-		sect_num++;
-
-	if (startsect + sect_num > total_sector)
-		return -1;
-	return 0;
-}
-
 /*
  * Find a directory entry based on filename or start cluster number
  * If the directory entry is not found,
@@ -784,11 +794,10 @@ static int normalize_longname(char *l_filename, const char *filename)
 	return 0;
 }
 
-static int do_fat_write(const char *filename, void *buffer, loff_t size,
-			loff_t *actwrite)
+int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
+		      loff_t size, loff_t *actwrite)
 {
 	dir_entry *retdent;
-	__u32 start_cluster;
 	fsdata datablock = { .fatbuf = NULL, };
 	fsdata *mydata = &datablock;
 	fat_itr *itr = NULL;
@@ -796,6 +805,8 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	char *filename_copy, *parent, *basename;
 	char l_filename[VFAT_MAXLEN_BYTES];
 
+	debug("writing %s\n", filename);
+
 	filename_copy = strdup(filename);
 	if (!filename_copy)
 		return -ENOMEM;
@@ -841,47 +852,8 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			goto exit;
 		}
 
-		/* Update file size and start_cluster in a directory entry */
-		retdent->size = cpu_to_le32(size);
-		start_cluster = START(retdent);
-
-		if (start_cluster) {
-			if (size) {
-				ret = check_overflow(mydata, start_cluster,
-							size);
-				if (ret) {
-					printf("Error: %llu overflow\n", size);
-					ret = -ENOSPC;
-					goto exit;
-				}
-			}
-
-			ret = clear_fatent(mydata, start_cluster);
-			if (ret) {
-				printf("Error: clearing FAT entries\n");
-				ret = -EIO;
-				goto exit;
-			}
-
-			if (!size)
-				set_start_cluster(mydata, retdent, 0);
-		} else if (size) {
-			ret = start_cluster = find_empty_cluster(mydata);
-			if (ret < 0) {
-				printf("Error: finding empty cluster\n");
-				ret = -ENOSPC;
-				goto exit;
-			}
-
-			ret = check_overflow(mydata, start_cluster, size);
-			if (ret) {
-				printf("Error: %llu overflow\n", size);
-				ret = -ENOSPC;
-				goto exit;
-			}
-
-			set_start_cluster(mydata, retdent, start_cluster);
-		}
+		/* Update file size in a directory entry */
+		retdent->size = cpu_to_le32(pos + size);
 	} else {
 		/* Create a new file */
 
@@ -909,32 +881,13 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 			goto exit;
 		}
 
-		if (size) {
-			ret = start_cluster = find_empty_cluster(mydata);
-			if (ret < 0) {
-				printf("Error: finding empty cluster\n");
-				ret = -ENOSPC;
-				goto exit;
-			}
-
-			ret = check_overflow(mydata, start_cluster, size);
-			if (ret) {
-				printf("Error: %llu overflow\n", size);
-				ret = -ENOSPC;
-				goto exit;
-			}
-		} else {
-			start_cluster = 0;
-		}
-
 		/* Set attribute as archieve for regular file */
-		fill_dentry(itr->fsdata, itr->dent, filename,
-					start_cluster, size, 0x20);
+		fill_dentry(itr->fsdata, itr->dent, filename, 0, size, 0x20);
 
 		retdent = itr->dent;
 	}
 
-	ret = set_contents(mydata, retdent, buffer, size, actwrite);
+	ret = set_contents(mydata, retdent, pos, buffer, size, actwrite);
 	if (ret < 0) {
 		printf("Error: writing contents\n");
 		ret = -EIO;
@@ -973,6 +926,5 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
 		return -EINVAL;
 	}
 
-	printf("writing %s\n", filename);
-	return do_fat_write(filename, buffer, maxsize, actwrite);
+	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
 }
-- 
2.17.0

[U-Boot] [PATCH 09/17] fs: fat: support write with non-zero offset

[AKASHI Takahiro]

In this patch, all the necessary code for allowing for a file offset
at write is implemented. What plays a major roll here is get_set_cluster(),
which, in contrast to its counterpart, set_cluster(), only operates on
already-allocated clusters, overwriting with data.

So, with a file offset specified, set_contents() seeks and writes data
with set_get_cluster() until the end of a file, and, once it reaches
there, continues writing with set_cluster() for the rest.

Please note that a file will be trimmed as a result of write operation if
write ends before reaching file's end. This is an intended behavior
in order to maitain compatibility with the current interface.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat_write.c | 287 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 272 insertions(+), 15 deletions(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 3a9c53e253..cc45a33876 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -450,6 +450,120 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
 	return 0;
 }
 
+static __u8 tmpbuf_cluster[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
+
+/*
+ * Read and modify data on existing and consecutive cluster blocks
+ */
+static int
+get_set_cluster(fsdata *mydata, __u32 clustnum, loff_t pos, __u8 *buffer,
+		loff_t size, loff_t *gotsize)
+{
+	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
+	__u32 startsect;
+	loff_t wsize;
+	int clustcount, i, ret;
+
+	*gotsize = 0;
+	if (!size)
+		return 0;
+
+	assert(pos < bytesperclust);
+	startsect = clust_to_sect(mydata, clustnum);
+
+	debug("clustnum: %d, startsect: %d, pos: %lld\n", clustnum, startsect,
+									pos);
+
+	/* partial write@beginning */
+	if (pos) {
+		wsize = min(bytesperclust - pos, size);
+		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
+		if (ret != mydata->clust_size) {
+			debug("Error reading data (got %d)\n", ret);
+			return -1;
+		}
+
+		memcpy(tmpbuf_cluster + pos, buffer, wsize);
+		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
+		if (ret != mydata->clust_size) {
+			debug("Error writing data (got %d)\n", ret);
+			return -1;
+		}
+
+		size -= wsize;
+		buffer += wsize;
+		*gotsize += wsize;
+
+		startsect += mydata->clust_size;
+
+		if (!size)
+			return 0;
+	}
+
+	/* full-cluster write */
+	if (size >= bytesperclust) {
+		clustcount = lldiv(size, bytesperclust);
+
+		if (!((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1))) {
+			wsize = clustcount * bytesperclust;
+			ret = disk_write(startsect,
+					clustcount * mydata->clust_size,
+					buffer);
+			if (ret != clustcount * mydata->clust_size) {
+				debug("Error writing data (got %d)\n", ret);
+				return -1;
+			}
+
+			size -= wsize;
+			buffer += wsize;
+			*gotsize += wsize;
+
+			startsect += clustcount * mydata->clust_size;
+		} else {
+			for (i = 0; i < clustcount; i++) {
+				memcpy(tmpbuf_cluster, buffer, bytesperclust);
+				ret = disk_write(startsect, mydata->clust_size,
+								tmpbuf_cluster);
+				if (ret != mydata->clust_size) {
+					debug("Error writing data (got %d)\n",
+									ret);
+					return -1;
+				}
+
+				size -= bytesperclust;
+				buffer += bytesperclust;
+				*gotsize += bytesperclust;
+
+				startsect += mydata->clust_size;
+			}
+		}
+	}
+
+	/* partial write at end */
+	if (size) {
+		wsize = size;
+		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
+		if (ret != mydata->clust_size) {
+			debug("Error reading data (got %d)\n", ret);
+			return -1;
+		}
+		memcpy(tmpbuf_cluster, buffer, wsize);
+		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
+		if (ret != mydata->clust_size) {
+			debug("Error writing data (got %d)\n", ret);
+			return -1;
+		}
+
+		size -= wsize;
+		buffer += wsize;
+		*gotsize += wsize;
+	}
+
+	assert(!size);
+
+	return 0;
+}
+
 /*
  * Find the first empty cluster
  */
@@ -579,26 +693,158 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
 	__u32 curclust = START(dentptr);
 	__u32 endclust = 0, newclust = 0;
-	loff_t actsize;
+	loff_t cur_pos, offset, actsize, wsize;
 
 	*gotsize = 0;
-	filesize = maxsize;
+	filesize = pos + maxsize;
 
 	debug("%llu bytes\n", filesize);
 
-	if (curclust) {
-		/*
-		 * release already-allocated clusters anyway
-		 */
-		if (clear_fatent(mydata, curclust)) {
-			printf("Error: clearing FAT entries\n");
+	if (!filesize) {
+		if (!curclust)
+			return 0;
+		if (!CHECK_CLUST(curclust, mydata->fatsize) ||
+		    IS_LAST_CLUST(curclust, mydata->fatsize)) {
+			clear_fatent(mydata, curclust);
+			set_start_cluster(mydata, dentptr, 0);
+			return 0;
+		}
+		debug("curclust: 0x%x\n", curclust);
+		debug("Invalid FAT entry\n");
+		return -1;
+	}
+
+	if (!curclust) {
+		assert(pos == 0);
+		goto set_clusters;
+	}
+
+	/* go to cluster at pos */
+	cur_pos = bytesperclust;
+	while (1) {
+		if (pos <= cur_pos)
+			break;
+		if (IS_LAST_CLUST(curclust, mydata->fatsize))
+			break;
+
+		newclust = get_fatent(mydata, curclust);
+		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
+		    CHECK_CLUST(newclust, mydata->fatsize)) {
+			debug("curclust: 0x%x\n", curclust);
+			debug("Invalid FAT entry\n");
 			return -1;
 		}
+
+		cur_pos += bytesperclust;
+		curclust = newclust;
+	}
+	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
+		assert(pos == cur_pos);
+		goto set_clusters;
 	}
 
-	curclust = find_empty_cluster(mydata);
-	set_start_cluster(mydata, dentptr, curclust);
+	assert(pos < cur_pos);
+	cur_pos -= bytesperclust;
 
+	/* overwrite */
+	assert(IS_LAST_CLUST(curclust, mydata->fatsize) ||
+	       !CHECK_CLUST(curclust, mydata->fatsize));
+
+	while (1) {
+		/* search for allocated consecutive clusters */
+		actsize = bytesperclust;
+		endclust = curclust;
+		while (1) {
+			if (filesize <= (cur_pos + actsize))
+				break;
+
+			newclust = get_fatent(mydata, endclust);
+
+			if (IS_LAST_CLUST(newclust, mydata->fatsize))
+				break;
+			if (CHECK_CLUST(newclust, mydata->fatsize)) {
+				debug("curclust: 0x%x\n", curclust);
+				debug("Invalid FAT entry\n");
+				return -1;
+			}
+
+			actsize += bytesperclust;
+			endclust = newclust;
+		}
+
+		/* overwrite to <curclust..endclust> */
+		if (pos < cur_pos)
+			offset = 0;
+		else
+			offset = pos - cur_pos;
+		wsize = min(cur_pos + actsize, filesize) - pos;
+		if (get_set_cluster(mydata, curclust, offset, buffer, wsize,
+								&actsize)) {
+			printf("Error get-and-setting cluster\n");
+			return -1;
+		}
+		buffer += wsize;
+		*gotsize += wsize;
+		cur_pos += offset + wsize;
+
+		if (filesize <= cur_pos)
+			break;
+
+		/* CHECK: newclust = get_fatent(mydata, endclust); */
+
+		if (IS_LAST_CLUST(newclust, mydata->fatsize))
+			/* no more clusters */
+			break;
+
+		curclust = newclust;
+	}
+
+	if (filesize <= cur_pos) {
+		/* no more write */
+		newclust = get_fatent(mydata, endclust);
+		if (!IS_LAST_CLUST(newclust, mydata->fatsize)) {
+			/* truncate the rest */
+			clear_fatent(mydata, newclust);
+
+			/* Mark end of file in FAT */
+			if (mydata->fatsize == 12)
+				newclust = 0xfff;
+			else if (mydata->fatsize == 16)
+				newclust = 0xffff;
+			else if (mydata->fatsize == 32)
+				newclust = 0xfffffff;
+			set_fatent_value(mydata, endclust, newclust);
+		}
+
+		return 0;
+	}
+
+	curclust = endclust;
+	filesize -= cur_pos;
+	assert(!(cur_pos % bytesperclust));
+
+set_clusters:
+	/* allocate and write */
+	assert(!pos);
+
+	/* Assure that curclust is valid */
+	if (!curclust) {
+		curclust = find_empty_cluster(mydata);
+		set_start_cluster(mydata, dentptr, curclust);
+	} else {
+		newclust = get_fatent(mydata, curclust);
+
+		if (IS_LAST_CLUST(newclust, mydata->fatsize)) {
+			newclust = determine_fatent(mydata, curclust);
+			set_fatent_value(mydata, curclust, newclust);
+			curclust = newclust;
+		} else {
+			debug("error: something wrong\n");
+			return -1;
+		}
+	}
+
+	/* TODO: already partially written */
 	if (check_overflow(mydata, curclust, filesize)) {
 		printf("Error: no space left: %llu\n", filesize);
 		return -1;
@@ -852,6 +1098,16 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
 			goto exit;
 		}
 
+		/* A file exists */
+		if (pos == -1)
+			/* Append to the end */
+			pos = FAT2CPU32(retdent->size);
+		if (pos > retdent->size) {
+			/* No hole allowed */
+			ret = -EINVAL;
+			goto exit;
+		}
+
 		/* Update file size in a directory entry */
 		retdent->size = cpu_to_le32(pos + size);
 	} else {
@@ -872,6 +1128,12 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
 			goto exit;
 		}
 
+		if (pos) {
+			/* No hole allowed */
+			ret = -EINVAL;
+			goto exit;
+		}
+
 		memset(itr->dent, 0, sizeof(*itr->dent));
 
 		/* Set short name to set alias checksum field in dir_slot */
@@ -921,10 +1183,5 @@ exit:
 int file_fat_write(const char *filename, void *buffer, loff_t offset,
 		   loff_t maxsize, loff_t *actwrite)
 {
-	if (offset != 0) {
-		printf("Error: non zero offset is currently not supported.\n");
-		return -EINVAL;
-	}
-
 	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
 }
-- 
2.17.0

[U-Boot] [PATCH 09/17] fs: fat: support write with non-zero offset

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> In this patch, all the necessary code for allowing for a file offset
> at write is implemented. What plays a major roll here is get_set_cluster(),
> which, in contrast to its counterpart, set_cluster(), only operates on
> already-allocated clusters, overwriting with data.
> 
> So, with a file offset specified, set_contents() seeks and writes data
> with set_get_cluster() until the end of a file, and, once it reaches
> there, continues writing with set_cluster() for the rest.
> 
> Please note that a file will be trimmed as a result of write operation if
> write ends before reaching file's end. This is an intended behavior
> in order to maitain compatibility with the current interface.

This does not match the EFI spec.

> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

When testing I observed that I could not write at an offset larger than
the file size. This does not match the EFI spec:

EFI_FILE_PROTOCOL.SetPosition():
seeking past the end of the file is allowed (a subsequent write would
grow the file).

Best regards

Heinrich


> ---
>  fs/fat/fat_write.c | 287 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 272 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 3a9c53e253..cc45a33876 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -450,6 +450,120 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
>  	return 0;
>  }
>  
> +static __u8 tmpbuf_cluster[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> +
> +/*
> + * Read and modify data on existing and consecutive cluster blocks
> + */
> +static int
> +get_set_cluster(fsdata *mydata, __u32 clustnum, loff_t pos, __u8 *buffer,
> +		loff_t size, loff_t *gotsize)
> +{
> +	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
> +	__u32 startsect;
> +	loff_t wsize;
> +	int clustcount, i, ret;
> +
> +	*gotsize = 0;
> +	if (!size)
> +		return 0;
> +
> +	assert(pos < bytesperclust);
> +	startsect = clust_to_sect(mydata, clustnum);
> +
> +	debug("clustnum: %d, startsect: %d, pos: %lld\n", clustnum, startsect,
> +									pos);
> +
> +	/* partial write at beginning */
> +	if (pos) {
> +		wsize = min(bytesperclust - pos, size);
> +		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
> +		if (ret != mydata->clust_size) {
> +			debug("Error reading data (got %d)\n", ret);
> +			return -1;
> +		}
> +
> +		memcpy(tmpbuf_cluster + pos, buffer, wsize);
> +		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
> +		if (ret != mydata->clust_size) {
> +			debug("Error writing data (got %d)\n", ret);
> +			return -1;
> +		}
> +
> +		size -= wsize;
> +		buffer += wsize;
> +		*gotsize += wsize;
> +
> +		startsect += mydata->clust_size;
> +
> +		if (!size)
> +			return 0;
> +	}
> +
> +	/* full-cluster write */
> +	if (size >= bytesperclust) {
> +		clustcount = lldiv(size, bytesperclust);
> +
> +		if (!((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1))) {
> +			wsize = clustcount * bytesperclust;
> +			ret = disk_write(startsect,
> +					clustcount * mydata->clust_size,
> +					buffer);
> +			if (ret != clustcount * mydata->clust_size) {
> +				debug("Error writing data (got %d)\n", ret);
> +				return -1;
> +			}
> +
> +			size -= wsize;
> +			buffer += wsize;
> +			*gotsize += wsize;
> +
> +			startsect += clustcount * mydata->clust_size;
> +		} else {
> +			for (i = 0; i < clustcount; i++) {
> +				memcpy(tmpbuf_cluster, buffer, bytesperclust);
> +				ret = disk_write(startsect, mydata->clust_size,
> +								tmpbuf_cluster);
> +				if (ret != mydata->clust_size) {
> +					debug("Error writing data (got %d)\n",
> +									ret);
> +					return -1;
> +				}
> +
> +				size -= bytesperclust;
> +				buffer += bytesperclust;
> +				*gotsize += bytesperclust;
> +
> +				startsect += mydata->clust_size;
> +			}
> +		}
> +	}
> +
> +	/* partial write at end */
> +	if (size) {
> +		wsize = size;
> +		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
> +		if (ret != mydata->clust_size) {
> +			debug("Error reading data (got %d)\n", ret);
> +			return -1;
> +		}
> +		memcpy(tmpbuf_cluster, buffer, wsize);
> +		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
> +		if (ret != mydata->clust_size) {
> +			debug("Error writing data (got %d)\n", ret);
> +			return -1;
> +		}
> +
> +		size -= wsize;
> +		buffer += wsize;
> +		*gotsize += wsize;
> +	}
> +
> +	assert(!size);
> +
> +	return 0;
> +}
> +
>  /*
>   * Find the first empty cluster
>   */
> @@ -579,26 +693,158 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
>  	__u32 curclust = START(dentptr);
>  	__u32 endclust = 0, newclust = 0;
> -	loff_t actsize;
> +	loff_t cur_pos, offset, actsize, wsize;
>  
>  	*gotsize = 0;
> -	filesize = maxsize;
> +	filesize = pos + maxsize;
>  
>  	debug("%llu bytes\n", filesize);
>  
> -	if (curclust) {
> -		/*
> -		 * release already-allocated clusters anyway
> -		 */
> -		if (clear_fatent(mydata, curclust)) {
> -			printf("Error: clearing FAT entries\n");
> +	if (!filesize) {
> +		if (!curclust)
> +			return 0;
> +		if (!CHECK_CLUST(curclust, mydata->fatsize) ||
> +		    IS_LAST_CLUST(curclust, mydata->fatsize)) {
> +			clear_fatent(mydata, curclust);
> +			set_start_cluster(mydata, dentptr, 0);
> +			return 0;
> +		}
> +		debug("curclust: 0x%x\n", curclust);
> +		debug("Invalid FAT entry\n");
> +		return -1;
> +	}
> +
> +	if (!curclust) {
> +		assert(pos == 0);
> +		goto set_clusters;
> +	}
> +
> +	/* go to cluster at pos */
> +	cur_pos = bytesperclust;
> +	while (1) {
> +		if (pos <= cur_pos)
> +			break;
> +		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> +			break;
> +
> +		newclust = get_fatent(mydata, curclust);
> +		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> +		    CHECK_CLUST(newclust, mydata->fatsize)) {
> +			debug("curclust: 0x%x\n", curclust);
> +			debug("Invalid FAT entry\n");
>  			return -1;
>  		}
> +
> +		cur_pos += bytesperclust;
> +		curclust = newclust;
> +	}
> +	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> +		assert(pos == cur_pos);
> +		goto set_clusters;
>  	}
>  
> -	curclust = find_empty_cluster(mydata);
> -	set_start_cluster(mydata, dentptr, curclust);
> +	assert(pos < cur_pos);
> +	cur_pos -= bytesperclust;
>  
> +	/* overwrite */
> +	assert(IS_LAST_CLUST(curclust, mydata->fatsize) ||
> +	       !CHECK_CLUST(curclust, mydata->fatsize));
> +
> +	while (1) {
> +		/* search for allocated consecutive clusters */
> +		actsize = bytesperclust;
> +		endclust = curclust;
> +		while (1) {
> +			if (filesize <= (cur_pos + actsize))
> +				break;
> +
> +			newclust = get_fatent(mydata, endclust);
> +
> +			if (IS_LAST_CLUST(newclust, mydata->fatsize))
> +				break;
> +			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> +				debug("curclust: 0x%x\n", curclust);
> +				debug("Invalid FAT entry\n");
> +				return -1;
> +			}
> +
> +			actsize += bytesperclust;
> +			endclust = newclust;
> +		}
> +
> +		/* overwrite to <curclust..endclust> */
> +		if (pos < cur_pos)
> +			offset = 0;
> +		else
> +			offset = pos - cur_pos;
> +		wsize = min(cur_pos + actsize, filesize) - pos;
> +		if (get_set_cluster(mydata, curclust, offset, buffer, wsize,
> +								&actsize)) {
> +			printf("Error get-and-setting cluster\n");
> +			return -1;
> +		}
> +		buffer += wsize;
> +		*gotsize += wsize;
> +		cur_pos += offset + wsize;
> +
> +		if (filesize <= cur_pos)
> +			break;
> +
> +		/* CHECK: newclust = get_fatent(mydata, endclust); */
> +
> +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
> +			/* no more clusters */
> +			break;
> +
> +		curclust = newclust;
> +	}
> +
> +	if (filesize <= cur_pos) {
> +		/* no more write */
> +		newclust = get_fatent(mydata, endclust);
> +		if (!IS_LAST_CLUST(newclust, mydata->fatsize)) {
> +			/* truncate the rest */
> +			clear_fatent(mydata, newclust);
> +
> +			/* Mark end of file in FAT */
> +			if (mydata->fatsize == 12)
> +				newclust = 0xfff;
> +			else if (mydata->fatsize == 16)
> +				newclust = 0xffff;
> +			else if (mydata->fatsize == 32)
> +				newclust = 0xfffffff;
> +			set_fatent_value(mydata, endclust, newclust);
> +		}
> +
> +		return 0;
> +	}
> +
> +	curclust = endclust;
> +	filesize -= cur_pos;
> +	assert(!(cur_pos % bytesperclust));
> +
> +set_clusters:
> +	/* allocate and write */
> +	assert(!pos);
> +
> +	/* Assure that curclust is valid */
> +	if (!curclust) {
> +		curclust = find_empty_cluster(mydata);
> +		set_start_cluster(mydata, dentptr, curclust);
> +	} else {
> +		newclust = get_fatent(mydata, curclust);
> +
> +		if (IS_LAST_CLUST(newclust, mydata->fatsize)) {
> +			newclust = determine_fatent(mydata, curclust);
> +			set_fatent_value(mydata, curclust, newclust);
> +			curclust = newclust;
> +		} else {
> +			debug("error: something wrong\n");
> +			return -1;
> +		}
> +	}
> +
> +	/* TODO: already partially written */
>  	if (check_overflow(mydata, curclust, filesize)) {
>  		printf("Error: no space left: %llu\n", filesize);
>  		return -1;
> @@ -852,6 +1098,16 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
>  			goto exit;
>  		}
>  
> +		/* A file exists */
> +		if (pos == -1)
> +			/* Append to the end */
> +			pos = FAT2CPU32(retdent->size);
> +		if (pos > retdent->size) {
> +			/* No hole allowed */
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
>  		/* Update file size in a directory entry */
>  		retdent->size = cpu_to_le32(pos + size);
>  	} else {
> @@ -872,6 +1128,12 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
>  			goto exit;
>  		}
>  
> +		if (pos) {
> +			/* No hole allowed */
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
>  		memset(itr->dent, 0, sizeof(*itr->dent));
>  
>  		/* Set short name to set alias checksum field in dir_slot */
> @@ -921,10 +1183,5 @@ exit:
>  int file_fat_write(const char *filename, void *buffer, loff_t offset,
>  		   loff_t maxsize, loff_t *actwrite)
>  {
> -	if (offset != 0) {
> -		printf("Error: non zero offset is currently not supported.\n");
> -		return -EINVAL;
> -	}
> -
>  	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
>  }
> 

[U-Boot] [PATCH 09/17] fs: fat: support write with non-zero offset

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 07:46:49PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > In this patch, all the necessary code for allowing for a file offset
> > at write is implemented. What plays a major roll here is get_set_cluster(),
> > which, in contrast to its counterpart, set_cluster(), only operates on
> > already-allocated clusters, overwriting with data.
> > 
> > So, with a file offset specified, set_contents() seeks and writes data
> > with set_get_cluster() until the end of a file, and, once it reaches
> > there, continues writing with set_cluster() for the rest.
> > 
> > Please note that a file will be trimmed as a result of write operation if
> > write ends before reaching file's end. This is an intended behavior
> > in order to maitain compatibility with the current interface.
> 
> This does not match the EFI spec.

First, please understand my standpoint that I mentioned in a reply
to your comment on my cover letter.

> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> When testing I observed that I could not write at an offset larger than
> the file size. This does not match the EFI spec:

So this is an intended behavior.

> EFI_FILE_PROTOCOL.SetPosition():
> seeking past the end of the file is allowed (a subsequent write would
> grow the file).

Surely, this is a discussion point.
Any comment from other folks?

-Takahiro AKASHI

> Best regards
> 
> Heinrich
> 
> 
> > ---
> >  fs/fat/fat_write.c | 287 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 272 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 3a9c53e253..cc45a33876 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -450,6 +450,120 @@ set_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer,
> >  	return 0;
> >  }
> >  
> > +static __u8 tmpbuf_cluster[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
> > +
> > +/*
> > + * Read and modify data on existing and consecutive cluster blocks
> > + */
> > +static int
> > +get_set_cluster(fsdata *mydata, __u32 clustnum, loff_t pos, __u8 *buffer,
> > +		loff_t size, loff_t *gotsize)
> > +{
> > +	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
> > +	__u32 startsect;
> > +	loff_t wsize;
> > +	int clustcount, i, ret;
> > +
> > +	*gotsize = 0;
> > +	if (!size)
> > +		return 0;
> > +
> > +	assert(pos < bytesperclust);
> > +	startsect = clust_to_sect(mydata, clustnum);
> > +
> > +	debug("clustnum: %d, startsect: %d, pos: %lld\n", clustnum, startsect,
> > +									pos);
> > +
> > +	/* partial write at beginning */
> > +	if (pos) {
> > +		wsize = min(bytesperclust - pos, size);
> > +		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
> > +		if (ret != mydata->clust_size) {
> > +			debug("Error reading data (got %d)\n", ret);
> > +			return -1;
> > +		}
> > +
> > +		memcpy(tmpbuf_cluster + pos, buffer, wsize);
> > +		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
> > +		if (ret != mydata->clust_size) {
> > +			debug("Error writing data (got %d)\n", ret);
> > +			return -1;
> > +		}
> > +
> > +		size -= wsize;
> > +		buffer += wsize;
> > +		*gotsize += wsize;
> > +
> > +		startsect += mydata->clust_size;
> > +
> > +		if (!size)
> > +			return 0;
> > +	}
> > +
> > +	/* full-cluster write */
> > +	if (size >= bytesperclust) {
> > +		clustcount = lldiv(size, bytesperclust);
> > +
> > +		if (!((unsigned long)buffer & (ARCH_DMA_MINALIGN - 1))) {
> > +			wsize = clustcount * bytesperclust;
> > +			ret = disk_write(startsect,
> > +					clustcount * mydata->clust_size,
> > +					buffer);
> > +			if (ret != clustcount * mydata->clust_size) {
> > +				debug("Error writing data (got %d)\n", ret);
> > +				return -1;
> > +			}
> > +
> > +			size -= wsize;
> > +			buffer += wsize;
> > +			*gotsize += wsize;
> > +
> > +			startsect += clustcount * mydata->clust_size;
> > +		} else {
> > +			for (i = 0; i < clustcount; i++) {
> > +				memcpy(tmpbuf_cluster, buffer, bytesperclust);
> > +				ret = disk_write(startsect, mydata->clust_size,
> > +								tmpbuf_cluster);
> > +				if (ret != mydata->clust_size) {
> > +					debug("Error writing data (got %d)\n",
> > +									ret);
> > +					return -1;
> > +				}
> > +
> > +				size -= bytesperclust;
> > +				buffer += bytesperclust;
> > +				*gotsize += bytesperclust;
> > +
> > +				startsect += mydata->clust_size;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* partial write at end */
> > +	if (size) {
> > +		wsize = size;
> > +		ret = disk_read(startsect, mydata->clust_size, tmpbuf_cluster);
> > +		if (ret != mydata->clust_size) {
> > +			debug("Error reading data (got %d)\n", ret);
> > +			return -1;
> > +		}
> > +		memcpy(tmpbuf_cluster, buffer, wsize);
> > +		ret = disk_write(startsect, mydata->clust_size, tmpbuf_cluster);
> > +		if (ret != mydata->clust_size) {
> > +			debug("Error writing data (got %d)\n", ret);
> > +			return -1;
> > +		}
> > +
> > +		size -= wsize;
> > +		buffer += wsize;
> > +		*gotsize += wsize;
> > +	}
> > +
> > +	assert(!size);
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Find the first empty cluster
> >   */
> > @@ -579,26 +693,158 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  	unsigned int bytesperclust = mydata->clust_size * mydata->sect_size;
> >  	__u32 curclust = START(dentptr);
> >  	__u32 endclust = 0, newclust = 0;
> > -	loff_t actsize;
> > +	loff_t cur_pos, offset, actsize, wsize;
> >  
> >  	*gotsize = 0;
> > -	filesize = maxsize;
> > +	filesize = pos + maxsize;
> >  
> >  	debug("%llu bytes\n", filesize);
> >  
> > -	if (curclust) {
> > -		/*
> > -		 * release already-allocated clusters anyway
> > -		 */
> > -		if (clear_fatent(mydata, curclust)) {
> > -			printf("Error: clearing FAT entries\n");
> > +	if (!filesize) {
> > +		if (!curclust)
> > +			return 0;
> > +		if (!CHECK_CLUST(curclust, mydata->fatsize) ||
> > +		    IS_LAST_CLUST(curclust, mydata->fatsize)) {
> > +			clear_fatent(mydata, curclust);
> > +			set_start_cluster(mydata, dentptr, 0);
> > +			return 0;
> > +		}
> > +		debug("curclust: 0x%x\n", curclust);
> > +		debug("Invalid FAT entry\n");
> > +		return -1;
> > +	}
> > +
> > +	if (!curclust) {
> > +		assert(pos == 0);
> > +		goto set_clusters;
> > +	}
> > +
> > +	/* go to cluster at pos */
> > +	cur_pos = bytesperclust;
> > +	while (1) {
> > +		if (pos <= cur_pos)
> > +			break;
> > +		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> > +			break;
> > +
> > +		newclust = get_fatent(mydata, curclust);
> > +		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> > +		    CHECK_CLUST(newclust, mydata->fatsize)) {
> > +			debug("curclust: 0x%x\n", curclust);
> > +			debug("Invalid FAT entry\n");
> >  			return -1;
> >  		}
> > +
> > +		cur_pos += bytesperclust;
> > +		curclust = newclust;
> > +	}
> > +	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> > +		assert(pos == cur_pos);
> > +		goto set_clusters;
> >  	}
> >  
> > -	curclust = find_empty_cluster(mydata);
> > -	set_start_cluster(mydata, dentptr, curclust);
> > +	assert(pos < cur_pos);
> > +	cur_pos -= bytesperclust;
> >  
> > +	/* overwrite */
> > +	assert(IS_LAST_CLUST(curclust, mydata->fatsize) ||
> > +	       !CHECK_CLUST(curclust, mydata->fatsize));
> > +
> > +	while (1) {
> > +		/* search for allocated consecutive clusters */
> > +		actsize = bytesperclust;
> > +		endclust = curclust;
> > +		while (1) {
> > +			if (filesize <= (cur_pos + actsize))
> > +				break;
> > +
> > +			newclust = get_fatent(mydata, endclust);
> > +
> > +			if (IS_LAST_CLUST(newclust, mydata->fatsize))
> > +				break;
> > +			if (CHECK_CLUST(newclust, mydata->fatsize)) {
> > +				debug("curclust: 0x%x\n", curclust);
> > +				debug("Invalid FAT entry\n");
> > +				return -1;
> > +			}
> > +
> > +			actsize += bytesperclust;
> > +			endclust = newclust;
> > +		}
> > +
> > +		/* overwrite to <curclust..endclust> */
> > +		if (pos < cur_pos)
> > +			offset = 0;
> > +		else
> > +			offset = pos - cur_pos;
> > +		wsize = min(cur_pos + actsize, filesize) - pos;
> > +		if (get_set_cluster(mydata, curclust, offset, buffer, wsize,
> > +								&actsize)) {
> > +			printf("Error get-and-setting cluster\n");
> > +			return -1;
> > +		}
> > +		buffer += wsize;
> > +		*gotsize += wsize;
> > +		cur_pos += offset + wsize;
> > +
> > +		if (filesize <= cur_pos)
> > +			break;
> > +
> > +		/* CHECK: newclust = get_fatent(mydata, endclust); */
> > +
> > +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
> > +			/* no more clusters */
> > +			break;
> > +
> > +		curclust = newclust;
> > +	}
> > +
> > +	if (filesize <= cur_pos) {
> > +		/* no more write */
> > +		newclust = get_fatent(mydata, endclust);
> > +		if (!IS_LAST_CLUST(newclust, mydata->fatsize)) {
> > +			/* truncate the rest */
> > +			clear_fatent(mydata, newclust);
> > +
> > +			/* Mark end of file in FAT */
> > +			if (mydata->fatsize == 12)
> > +				newclust = 0xfff;
> > +			else if (mydata->fatsize == 16)
> > +				newclust = 0xffff;
> > +			else if (mydata->fatsize == 32)
> > +				newclust = 0xfffffff;
> > +			set_fatent_value(mydata, endclust, newclust);
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	curclust = endclust;
> > +	filesize -= cur_pos;
> > +	assert(!(cur_pos % bytesperclust));
> > +
> > +set_clusters:
> > +	/* allocate and write */
> > +	assert(!pos);
> > +
> > +	/* Assure that curclust is valid */
> > +	if (!curclust) {
> > +		curclust = find_empty_cluster(mydata);
> > +		set_start_cluster(mydata, dentptr, curclust);
> > +	} else {
> > +		newclust = get_fatent(mydata, curclust);
> > +
> > +		if (IS_LAST_CLUST(newclust, mydata->fatsize)) {
> > +			newclust = determine_fatent(mydata, curclust);
> > +			set_fatent_value(mydata, curclust, newclust);
> > +			curclust = newclust;
> > +		} else {
> > +			debug("error: something wrong\n");
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	/* TODO: already partially written */
> >  	if (check_overflow(mydata, curclust, filesize)) {
> >  		printf("Error: no space left: %llu\n", filesize);
> >  		return -1;
> > @@ -852,6 +1098,16 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
> >  			goto exit;
> >  		}
> >  
> > +		/* A file exists */
> > +		if (pos == -1)
> > +			/* Append to the end */
> > +			pos = FAT2CPU32(retdent->size);
> > +		if (pos > retdent->size) {
> > +			/* No hole allowed */
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> > +
> >  		/* Update file size in a directory entry */
> >  		retdent->size = cpu_to_le32(pos + size);
> >  	} else {
> > @@ -872,6 +1128,12 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer,
> >  			goto exit;
> >  		}
> >  
> > +		if (pos) {
> > +			/* No hole allowed */
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> > +
> >  		memset(itr->dent, 0, sizeof(*itr->dent));
> >  
> >  		/* Set short name to set alias checksum field in dir_slot */
> > @@ -921,10 +1183,5 @@ exit:
> >  int file_fat_write(const char *filename, void *buffer, loff_t offset,
> >  		   loff_t maxsize, loff_t *actwrite)
> >  {
> > -	if (offset != 0) {
> > -		printf("Error: non zero offset is currently not supported.\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
> >  }
> > 
> 

[U-Boot] [PATCH 10/17] cmd: fat: add offset parameter to fatwrite

[AKASHI Takahiro]

In this patch, fatwrite command is extended so as to accept an additional
parameter of file offset.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/fat.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/cmd/fat.c b/cmd/fat.c
index 03de5d11af..2a5f7bfc26 100644
--- a/cmd/fat.c
+++ b/cmd/fat.c
@@ -104,6 +104,7 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 	int ret;
 	unsigned long addr;
 	unsigned long count;
+	long offset;
 	struct blk_desc *dev_desc = NULL;
 	disk_partition_t info;
 	int dev = 0;
@@ -126,9 +127,11 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 	}
 	addr = simple_strtoul(argv[3], NULL, 16);
 	count = (argc <= 5) ? 0 : simple_strtoul(argv[5], NULL, 16);
+	/* offset should be a hex, but "-1" is allowed */
+	offset = (argc <= 6) ? 0 : simple_strtol(argv[6], NULL, 16);
 
 	buf = map_sysmem(addr, count);
-	ret = file_fat_write(argv[4], buf, 0, count, &size);
+	ret = file_fat_write(argv[4], buf, offset, count, &size);
 	unmap_sysmem(buf);
 	if (ret < 0) {
 		printf("\n** Unable to write \"%s\" from %s %d:%d **\n",
@@ -142,9 +145,9 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag,
 }
 
 U_BOOT_CMD(
-	fatwrite,	6,	0,	do_fat_fswrite,
+	fatwrite,	7,	0,	do_fat_fswrite,
 	"write file into a dos filesystem",
-	"<interface> <dev[:part]> <addr> <filename> [<bytes>]\n"
+	"<interface> <dev[:part]> <addr> <filename> [<bytes> [<offset>]]\n"
 	"    - write file 'filename' from the address 'addr' in RAM\n"
 	"      to 'dev' on 'interface'"
 );
-- 
2.17.0

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[AKASHI Takahiro]

"mkdir" interface is added to file operations.
This is a preparatory change as mkdir support for FAT file system
will be added in next patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/fs.h | 10 ++++++++++
 2 files changed, 55 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index 33808d549e..3cb6b21fe9 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename,
 	return -EACCES;
 }
 
+static inline int fs_mkdir_unsupported(const char *dirname)
+{
+	return -1;
+}
+
 struct fstype_info {
 	int fstype;
 	char *name;
@@ -142,6 +147,7 @@ struct fstype_info {
 	int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
 	/* see fs_closedir() */
 	void (*closedir)(struct fs_dir_stream *dirs);
+	int (*mkdir)(const char *dirname);
 };
 
 static struct fstype_info fstypes[] = {
@@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fat_opendir,
 		.readdir = fat_readdir,
 		.closedir = fat_closedir,
+		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
@@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = {
 #endif
 		.uuid = ext4fs_uuid,
 		.opendir = fs_opendir_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 #ifdef CONFIG_SANDBOX
@@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = {
 		.write = fs_write_sandbox,
 		.uuid = fs_uuid_unsupported,
 		.opendir = fs_opendir_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 #ifdef CONFIG_CMD_UBIFS
@@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = {
 		.write = fs_write_unsupported,
 		.uuid = fs_uuid_unsupported,
 		.opendir = fs_opendir_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_BTRFS
@@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = {
 		.write = fs_write_unsupported,
 		.uuid = btrfs_uuid,
 		.opendir = fs_opendir_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 	{
@@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = {
 		.write = fs_write_unsupported,
 		.uuid = fs_uuid_unsupported,
 		.opendir = fs_opendir_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 	},
 };
 
@@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs)
 }
 
 
+int fs_mkdir(const char *dirname)
+{
+	int ret;
+
+	struct fstype_info *info = fs_get_info(fs_type);
+
+	ret = info->mkdir(dirname);
+
+	fs_type = FS_TYPE_ANY;
+	fs_close();
+
+	return ret;
+}
+
 int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype)
 {
@@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	return CMD_RET_SUCCESS;
 }
 
+int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+								int fstype)
+{
+	int ret;
+
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	if (fs_set_blk_dev(argv[1], argv[2], fstype))
+		return 1;
+
+	ret = fs_mkdir(argv[3]);
+	if (ret) {
+		printf("** Unable to create a directory \"%s\" **\n", argv[3]);
+		return 1;
+	}
+
+	return 0;
+}
diff --git a/include/fs.h b/include/fs.h
index 163da103b4..fbaee154dd 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs);
  */
 void fs_closedir(struct fs_dir_stream *dirs);
 
+/*
+ * fs_mkdir - Create a directory
+ *
+ * @filename: Name of directory to create
+ * @return 0 on success, -1 on error conditions
+ */
+int fs_mkdir(const char *filename);
+
 /*
  * Common implementation for various filesystem commands, optionally limited
  * to a specific filesystem type via the fstype parameter.
@@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file,
 		int fstype);
 int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
+int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+		int fstype);
 
 /*
  * Determine the UUID of the specified filesystem and print it. Optionally it is
-- 
2.17.0

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> "mkdir" interface is added to file operations.
> This is a preparatory change as mkdir support for FAT file system
> will be added in next patch.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/fs.h | 10 ++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index 33808d549e..3cb6b21fe9 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename,
>  	return -EACCES;
>  }
>  
> +static inline int fs_mkdir_unsupported(const char *dirname)
> +{
> +	return -1;
> +}
> +
>  struct fstype_info {
>  	int fstype;
>  	char *name;
> @@ -142,6 +147,7 @@ struct fstype_info {
>  	int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
>  	/* see fs_closedir() */
>  	void (*closedir)(struct fs_dir_stream *dirs);
> +	int (*mkdir)(const char *dirname);
>  };
>  
>  static struct fstype_info fstypes[] = {
> @@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = {
>  		.opendir = fat_opendir,
>  		.readdir = fat_readdir,
>  		.closedir = fat_closedir,
> +		.mkdir = fs_mkdir_unsupported,

Why should we create a dummy function? Just use NULL as an indicator
that the interface method is not implemented. See the use of structures
for the driver model.

@Simon
The array fstypes[] should be replaced by a linker list to match the
driver model.

>  	},
>  #endif
>  #ifdef CONFIG_FS_EXT4
> @@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = {
>  #endif
>  		.uuid = ext4fs_uuid,
>  		.opendir = fs_opendir_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  	},
>  #endif
>  #ifdef CONFIG_SANDBOX
> @@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = {
>  		.write = fs_write_sandbox,
>  		.uuid = fs_uuid_unsupported,
>  		.opendir = fs_opendir_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  	},
>  #endif
>  #ifdef CONFIG_CMD_UBIFS
> @@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = {
>  		.write = fs_write_unsupported,
>  		.uuid = fs_uuid_unsupported,
>  		.opendir = fs_opendir_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  	},
>  #endif
>  #ifdef CONFIG_FS_BTRFS
> @@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = {
>  		.write = fs_write_unsupported,
>  		.uuid = btrfs_uuid,
>  		.opendir = fs_opendir_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  	},
>  #endif
>  	{
> @@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = {
>  		.write = fs_write_unsupported,
>  		.uuid = fs_uuid_unsupported,
>  		.opendir = fs_opendir_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  	},
>  };
>  
> @@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs)
>  }
>  
>  
> +int fs_mkdir(const char *dirname)
> +{
> +	int ret;
> +
> +	struct fstype_info *info = fs_get_info(fs_type);
> +
> +	ret = info->mkdir(dirname);

Please, check if mkdir is NULL before calling.

> +
> +	fs_type = FS_TYPE_ANY;
> +	fs_close();

What do you want to close here if mkdir() is not implemented by the file
system?

Best regards

Heinrich

> +
> +	return ret;
> +}
> +
>  int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>  		int fstype)
>  {
> @@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	return CMD_RET_SUCCESS;
>  }
>  
> +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> +								int fstype)
> +{
> +	int ret;
> +
> +	if (argc != 4)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], argv[2], fstype))
> +		return 1;
> +
> +	ret = fs_mkdir(argv[3]);
> +	if (ret) {
> +		printf("** Unable to create a directory \"%s\" **\n", argv[3]);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> diff --git a/include/fs.h b/include/fs.h
> index 163da103b4..fbaee154dd 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs);
>   */
>  void fs_closedir(struct fs_dir_stream *dirs);
>  
> +/*
> + * fs_mkdir - Create a directory
> + *
> + * @filename: Name of directory to create
> + * @return 0 on success, -1 on error conditions
> + */
> +int fs_mkdir(const char *filename);
> +
>  /*
>   * Common implementation for various filesystem commands, optionally limited
>   * to a specific filesystem type via the fstype parameter.
> @@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file,
>  		int fstype);
>  int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>  		int fstype);
> +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> +		int fstype);
>  
>  /*
>   * Determine the UUID of the specified filesystem and print it. Optionally it is
> 

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[Simon Glass]

Hi,

On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
>> "mkdir" interface is added to file operations.
>> This is a preparatory change as mkdir support for FAT file system
>> will be added in next patch.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/fs.h | 10 ++++++++++
>>  2 files changed, 55 insertions(+)
>>

We need to get a proper fs test in place before we add any more stuff.

Does someone waent to try converting fs-test.sh to pytest in test/py/tests ?

Regards,
Simon

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[Tom Rini]

On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote:
> Hi,
> 
> On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> >> "mkdir" interface is added to file operations.
> >> This is a preparatory change as mkdir support for FAT file system
> >> will be added in next patch.
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/fs.h | 10 ++++++++++
> >>  2 files changed, 55 insertions(+)
> >>
> 
> We need to get a proper fs test in place before we add any more stuff.

Agreed that we need more tests as part of this series.

> Does someone waent to try converting fs-test.sh to pytest in test/py/tests ?

I thought there was at least a first pass at that?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180723/4918f93a/attachment.sig>

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[AKASHI Takahiro]

On Mon, Jul 23, 2018 at 07:56:58PM -0400, Tom Rini wrote:
> On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote:
> > Hi,
> > 
> > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > >> "mkdir" interface is added to file operations.
> > >> This is a preparatory change as mkdir support for FAT file system
> > >> will be added in next patch.
> > >>
> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> ---
> > >>  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >>  include/fs.h | 10 ++++++++++
> > >>  2 files changed, 55 insertions(+)
> > >>
> > 
> > We need to get a proper fs test in place before we add any more stuff.
> 
> Agreed that we need more tests as part of this series.

Is this a new standard rule for new features?
Just kidding, but testing was definitely one of my concerns partly
because fs-test.sh is anyhow in an old fashion and partly because
I can't find any criteria for test coverage:
- white-box test or black-box test
- coverage for corner (or edge) cases
- some sort of stress test, etc.

> > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ?
> 
> I thought there was at least a first pass at that?

Well, I don't see any file system related scripts under test/py/tests.

Please advise me.

Thanks,
-Takahiro AKASHI

> -- 
> Tom

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[Simon Glass]

Hi,

On 23 July 2018 at 19:45, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> On Mon, Jul 23, 2018 at 07:56:58PM -0400, Tom Rini wrote:
>> On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote:
>> > Hi,
>> >
>> > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
>> > >> "mkdir" interface is added to file operations.
>> > >> This is a preparatory change as mkdir support for FAT file system
>> > >> will be added in next patch.
>> > >>
>> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> > >> ---
>> > >>  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> > >>  include/fs.h | 10 ++++++++++
>> > >>  2 files changed, 55 insertions(+)
>> > >>
>> >
>> > We need to get a proper fs test in place before we add any more stuff.
>>
>> Agreed that we need more tests as part of this series.
>
> Is this a new standard rule for new features?
> Just kidding, but testing was definitely one of my concerns partly
> because fs-test.sh is anyhow in an old fashion and partly because
> I can't find any criteria for test coverage:
> - white-box test or black-box test
> - coverage for corner (or edge) cases
> - some sort of stress test, etc.
>
>> > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ?
>>
>> I thought there was at least a first pass at that?
>
> Well, I don't see any file system related scripts under test/py/tests.
>
> Please advise me.

It should be possible to translate it from shell to Python without too
much trouble if you able able to give it a crack. See for example:

8729d58259 test: Convert the vboot test to test/py

Re testing in general, my view is:

- Unit tests / smaller tests are better than large ones
- We don't need stress tests for functionality (e.g. the current FS
test uses filesystems that are quite large, and I think they could
just be very very small and still get good test coverage and run
faster)
- Corner cases are an important part of tests (and easy to test with unit tests)
- Use the pytest framework

One day we'll enable code coverage for U-Boot sandbox and get a feel
for the missing test coverage.

Regards,
Simon

[U-Boot] [PATCH 11/17] fs: add mkdir interface

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 07:35:18PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > "mkdir" interface is added to file operations.
> > This is a preparatory change as mkdir support for FAT file system
> > will be added in next patch.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  fs/fs.c      | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/fs.h | 10 ++++++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 33808d549e..3cb6b21fe9 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename,
> >  	return -EACCES;
> >  }
> >  
> > +static inline int fs_mkdir_unsupported(const char *dirname)
> > +{
> > +	return -1;
> > +}
> > +
> >  struct fstype_info {
> >  	int fstype;
> >  	char *name;
> > @@ -142,6 +147,7 @@ struct fstype_info {
> >  	int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
> >  	/* see fs_closedir() */
> >  	void (*closedir)(struct fs_dir_stream *dirs);
> > +	int (*mkdir)(const char *dirname);
> >  };
> >  
> >  static struct fstype_info fstypes[] = {
> > @@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = {
> >  		.opendir = fat_opendir,
> >  		.readdir = fat_readdir,
> >  		.closedir = fat_closedir,
> > +		.mkdir = fs_mkdir_unsupported,
> 
> Why should we create a dummy function? Just use NULL as an indicator

because I followed coding style of the existing code here.

> that the interface method is not implemented. See the use of structures
> for the driver model.

Can any file system ever be based on driver model?

> @Simon
> The array fstypes[] should be replaced by a linker list to match the
> driver model.
> 
> >  	},
> >  #endif
> >  #ifdef CONFIG_FS_EXT4
> > @@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = {
> >  #endif
> >  		.uuid = ext4fs_uuid,
> >  		.opendir = fs_opendir_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  #endif
> >  #ifdef CONFIG_SANDBOX
> > @@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = {
> >  		.write = fs_write_sandbox,
> >  		.uuid = fs_uuid_unsupported,
> >  		.opendir = fs_opendir_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  #endif
> >  #ifdef CONFIG_CMD_UBIFS
> > @@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = {
> >  		.write = fs_write_unsupported,
> >  		.uuid = fs_uuid_unsupported,
> >  		.opendir = fs_opendir_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  #endif
> >  #ifdef CONFIG_FS_BTRFS
> > @@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = {
> >  		.write = fs_write_unsupported,
> >  		.uuid = btrfs_uuid,
> >  		.opendir = fs_opendir_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  #endif
> >  	{
> > @@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = {
> >  		.write = fs_write_unsupported,
> >  		.uuid = fs_uuid_unsupported,
> >  		.opendir = fs_opendir_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  };
> >  
> > @@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs)
> >  }
> >  
> >  
> > +int fs_mkdir(const char *dirname)
> > +{
> > +	int ret;
> > +
> > +	struct fstype_info *info = fs_get_info(fs_type);
> > +
> > +	ret = info->mkdir(dirname);
> 
> Please, check if mkdir is NULL before calling.

No other place checks for NULL. Is this really required?

> > +
> > +	fs_type = FS_TYPE_ANY;
> > +	fs_close();
> 
> What do you want to close here if mkdir() is not implemented by the file
> system?

I'm not quite confident here but ->close() be paired with ->probe(),
which is set to be called in fs_set_blk_dev().

Thanks,
-Takahiro AKASHI


> Best regards
> 
> Heinrich
> 
> > +
> > +	return ret;
> > +}
> > +
> >  int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> >  		int fstype)
> >  {
> > @@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> > +								int fstype)
> > +{
> > +	int ret;
> > +
> > +	if (argc != 4)
> > +		return CMD_RET_USAGE;
> > +
> > +	if (fs_set_blk_dev(argv[1], argv[2], fstype))
> > +		return 1;
> > +
> > +	ret = fs_mkdir(argv[3]);
> > +	if (ret) {
> > +		printf("** Unable to create a directory \"%s\" **\n", argv[3]);
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> > diff --git a/include/fs.h b/include/fs.h
> > index 163da103b4..fbaee154dd 100644
> > --- a/include/fs.h
> > +++ b/include/fs.h
> > @@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs);
> >   */
> >  void fs_closedir(struct fs_dir_stream *dirs);
> >  
> > +/*
> > + * fs_mkdir - Create a directory
> > + *
> > + * @filename: Name of directory to create
> > + * @return 0 on success, -1 on error conditions
> > + */
> > +int fs_mkdir(const char *filename);
> > +
> >  /*
> >   * Common implementation for various filesystem commands, optionally limited
> >   * to a specific filesystem type via the fstype parameter.
> > @@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file,
> >  		int fstype);
> >  int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> >  		int fstype);
> > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> > +		int fstype);
> >  
> >  /*
> >   * Determine the UUID of the specified filesystem and print it. Optionally it is
> > 
> 

[U-Boot] [PATCH 12/17] fs: fat: remember the starting cluster number of directory

[AKASHI Takahiro]

The starting cluster number of directory is needed to initialize ".."
(parent directory) entry when creating a new directory.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat.c  | 2 ++
 include/fat.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 9bafc3a40c..ade5264551 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -639,6 +639,7 @@ int fat_itr_root(fat_itr *itr, fsdata *fsdata)
 		return -ENXIO;
 
 	itr->fsdata = fsdata;
+	itr->start_clust = 0;
 	itr->clust = fsdata->root_cluster;
 	itr->next_clust = fsdata->root_cluster;
 	itr->dent = NULL;
@@ -674,6 +675,7 @@ void fat_itr_child(fat_itr *itr, fat_itr *parent)
 	assert(fat_itr_isdir(parent));
 
 	itr->fsdata = parent->fsdata;
+	itr->start_clust = clustnum;
 	if (clustnum > 0) {
 		itr->clust = clustnum;
 		itr->next_clust = clustnum;
diff --git a/include/fat.h b/include/fat.h
index bc0f77abb5..295da0f243 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -197,6 +197,7 @@ static inline u32 sect_to_clust(fsdata *fsdata, int sect)
 
 typedef struct {
 	fsdata    *fsdata;        /* filesystem parameters */
+	unsigned   start_clust;   /* first cluster */
 	unsigned   clust;         /* current cluster */
 	unsigned   next_clust;    /* next cluster if remaining == 0 */
 	int        last_cluster;  /* set once we've read last cluster */
-- 
2.17.0

[U-Boot] [PATCH 13/17] fs: fat: support mkdir

[AKASHI Takahiro]

In this patch, mkdir support is added to FAT file system.
A newly created directory contains only "." and ".." entries.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 fs/fat/fat_write.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
 fs/fs.c            |   3 +-
 include/fat.h      |   1 +
 3 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index cc45a33876..781883c9f4 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -1185,3 +1185,141 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
 {
 	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
 }
+
+int fat_mkdir(const char *new_dirname)
+{
+	dir_entry *retdent;
+	fsdata datablock = { .fatbuf = NULL, };
+	fsdata *mydata = &datablock;
+	fat_itr *itr = NULL;
+	char *dirname_copy, *parent, *dirname;
+	char l_dirname[VFAT_MAXLEN_BYTES];
+	int ret = -1;
+	loff_t actwrite;
+	unsigned int bytesperclust;
+	dir_entry *dotdent = NULL;
+
+	dirname_copy = strdup(new_dirname);
+	if (!dirname_copy)
+		goto exit;
+
+	split_filename(dirname_copy, &parent, &dirname);
+	if (!strlen(dirname)) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	if (normalize_longname(l_dirname, dirname)) {
+		printf("FAT: illegal filename (%s)\n", dirname);
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	itr = malloc_cache_aligned(sizeof(fat_itr));
+	if (!itr) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	ret = fat_itr_root(itr, &datablock);
+	if (ret)
+		goto exit;
+
+	total_sector = datablock.bs_total_sect;
+	if (total_sector == 0)
+		total_sector = (int)cur_part_info.size; /* cast of lbaint_t */
+
+	ret = fat_itr_resolve(itr, parent, TYPE_DIR);
+	if (ret) {
+		printf("%s: doesn't exist (%d)\n", parent, ret);
+		goto exit;
+	}
+
+	retdent = find_directory_entry(itr, l_dirname);
+
+	if (retdent) {
+		printf("%s: already exists\n", l_dirname);
+		ret = -EEXIST;
+		goto exit;
+	} else {
+		if (itr->is_root) {
+			/* root dir cannot have "." or ".." */
+			if (!strcmp(l_dirname, ".") ||
+			    !strcmp(l_dirname, "..")) {
+				ret = -EINVAL;
+				goto exit;
+			}
+		}
+
+		if (!itr->dent) {
+			printf("Error: allocating new dir entry\n");
+			ret = -EIO;
+			goto exit;
+		}
+
+		memset(itr->dent, 0, sizeof(*itr->dent));
+
+		/* Set short name to set alias checksum field in dir_slot */
+		set_name(itr->dent, dirname);
+		fill_dir_slot(itr, dirname);
+
+		/* Set attribute as archieve for regular file */
+		fill_dentry(itr->fsdata, itr->dent, dirname, 0, 0,
+							ATTR_DIR | ATTR_ARCH);
+
+		retdent = itr->dent;
+	}
+
+	/* Default entries */
+	bytesperclust = mydata->clust_size * mydata->sect_size;
+	dotdent = malloc_cache_aligned(bytesperclust);
+	if (!dotdent) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+	memset(dotdent, 0, bytesperclust);
+
+	memcpy(dotdent[0].name, ".       ", 8);
+	memcpy(dotdent[0].ext, "   ", 3);
+	dotdent[0].attr = ATTR_DIR | ATTR_ARCH;
+
+	memcpy(dotdent[1].name, "..      ", 8);
+	memcpy(dotdent[1].ext, "   ", 3);
+	dotdent[1].attr = ATTR_DIR | ATTR_ARCH;
+	set_start_cluster(mydata, &dotdent[1], itr->start_clust);
+
+	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
+								&actwrite);
+	if (ret < 0) {
+		printf("Error: writing contents\n");
+		goto exit;
+	}
+	/* Write twice for "." */
+	set_start_cluster(mydata, &dotdent[0], START(retdent));
+	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
+								&actwrite);
+	if (ret < 0) {
+		printf("Error: writing contents\n");
+		goto exit;
+	}
+
+	/* Flush fat buffer */
+	ret = flush_dirty_fat_buffer(mydata);
+	if (ret) {
+		printf("Error: flush fat buffer\n");
+		goto exit;
+	}
+
+	/* Write directory table to device */
+	ret = set_cluster(mydata, itr->clust, itr->block,
+			mydata->clust_size * mydata->sect_size);
+	if (ret)
+		printf("Error: writing directory entry\n");
+
+exit:
+	free(dirname_copy);
+	free(mydata->fatbuf);
+	free(itr);
+	free(dotdent);
+	return ret;
+}
diff --git a/fs/fs.c b/fs/fs.c
index 3cb6b21fe9..a92e060296 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -164,14 +164,15 @@ static struct fstype_info fstypes[] = {
 		.read = fat_read_file,
 #ifdef CONFIG_FAT_WRITE
 		.write = file_fat_write,
+		.mkdir = fat_mkdir,
 #else
 		.write = fs_write_unsupported,
+		.mkdir = fs_mkdir_unsupported,
 #endif
 		.uuid = fs_uuid_unsupported,
 		.opendir = fat_opendir,
 		.readdir = fat_readdir,
 		.closedir = fat_closedir,
-		.mkdir = fs_mkdir_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
diff --git a/include/fat.h b/include/fat.h
index 295da0f243..039b6e03de 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -237,5 +237,6 @@ int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
 int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
 int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
 void fat_closedir(struct fs_dir_stream *dirs);
+int fat_mkdir(const char *dirname);
 void fat_close(void);
 #endif /* _FAT_H_ */
-- 
2.17.0

[U-Boot] [PATCH 13/17] fs: fat: support mkdir

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> In this patch, mkdir support is added to FAT file system.
> A newly created directory contains only "." and ".." entries.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

The patch does set the creation date of the directory according to the
real time clock but to 1980-01-01T00:00:00Z.

$ ls /mnt/dir1/ -la --full-time
drwxr-xr-x 2 root root  2048 1980-01-01 01:00:00.000000000 +0100 dir3

Please, set the time correctly if the real time clock is available.

Best regards

Heinrich

> ---
>  fs/fat/fat_write.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/fs.c            |   3 +-
>  include/fat.h      |   1 +
>  3 files changed, 141 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index cc45a33876..781883c9f4 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -1185,3 +1185,141 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
>  {
>  	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
>  }
> +
> +int fat_mkdir(const char *new_dirname)
> +{
> +	dir_entry *retdent;
> +	fsdata datablock = { .fatbuf = NULL, };
> +	fsdata *mydata = &datablock;
> +	fat_itr *itr = NULL;
> +	char *dirname_copy, *parent, *dirname;
> +	char l_dirname[VFAT_MAXLEN_BYTES];
> +	int ret = -1;
> +	loff_t actwrite;
> +	unsigned int bytesperclust;
> +	dir_entry *dotdent = NULL;
> +
> +	dirname_copy = strdup(new_dirname);
> +	if (!dirname_copy)
> +		goto exit;
> +
> +	split_filename(dirname_copy, &parent, &dirname);
> +	if (!strlen(dirname)) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	if (normalize_longname(l_dirname, dirname)) {
> +		printf("FAT: illegal filename (%s)\n", dirname);
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	itr = malloc_cache_aligned(sizeof(fat_itr));
> +	if (!itr) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	ret = fat_itr_root(itr, &datablock);
> +	if (ret)
> +		goto exit;
> +
> +	total_sector = datablock.bs_total_sect;
> +	if (total_sector == 0)
> +		total_sector = (int)cur_part_info.size; /* cast of lbaint_t */
> +
> +	ret = fat_itr_resolve(itr, parent, TYPE_DIR);
> +	if (ret) {
> +		printf("%s: doesn't exist (%d)\n", parent, ret);
> +		goto exit;
> +	}
> +
> +	retdent = find_directory_entry(itr, l_dirname);
> +
> +	if (retdent) {
> +		printf("%s: already exists\n", l_dirname);
> +		ret = -EEXIST;
> +		goto exit;
> +	} else {
> +		if (itr->is_root) {
> +			/* root dir cannot have "." or ".." */
> +			if (!strcmp(l_dirname, ".") ||
> +			    !strcmp(l_dirname, "..")) {
> +				ret = -EINVAL;
> +				goto exit;
> +			}
> +		}
> +
> +		if (!itr->dent) {
> +			printf("Error: allocating new dir entry\n");
> +			ret = -EIO;
> +			goto exit;
> +		}
> +
> +		memset(itr->dent, 0, sizeof(*itr->dent));
> +
> +		/* Set short name to set alias checksum field in dir_slot */
> +		set_name(itr->dent, dirname);
> +		fill_dir_slot(itr, dirname);
> +
> +		/* Set attribute as archieve for regular file */
> +		fill_dentry(itr->fsdata, itr->dent, dirname, 0, 0,
> +							ATTR_DIR | ATTR_ARCH);
> +
> +		retdent = itr->dent;
> +	}
> +
> +	/* Default entries */
> +	bytesperclust = mydata->clust_size * mydata->sect_size;
> +	dotdent = malloc_cache_aligned(bytesperclust);
> +	if (!dotdent) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +	memset(dotdent, 0, bytesperclust);
> +
> +	memcpy(dotdent[0].name, ".       ", 8);
> +	memcpy(dotdent[0].ext, "   ", 3);
> +	dotdent[0].attr = ATTR_DIR | ATTR_ARCH;
> +
> +	memcpy(dotdent[1].name, "..      ", 8);
> +	memcpy(dotdent[1].ext, "   ", 3);
> +	dotdent[1].attr = ATTR_DIR | ATTR_ARCH;
> +	set_start_cluster(mydata, &dotdent[1], itr->start_clust);
> +
> +	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
> +								&actwrite);
> +	if (ret < 0) {
> +		printf("Error: writing contents\n");
> +		goto exit;
> +	}
> +	/* Write twice for "." */
> +	set_start_cluster(mydata, &dotdent[0], START(retdent));
> +	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
> +								&actwrite);
> +	if (ret < 0) {
> +		printf("Error: writing contents\n");
> +		goto exit;
> +	}
> +
> +	/* Flush fat buffer */
> +	ret = flush_dirty_fat_buffer(mydata);
> +	if (ret) {
> +		printf("Error: flush fat buffer\n");
> +		goto exit;
> +	}
> +
> +	/* Write directory table to device */
> +	ret = set_cluster(mydata, itr->clust, itr->block,
> +			mydata->clust_size * mydata->sect_size);
> +	if (ret)
> +		printf("Error: writing directory entry\n");
> +
> +exit:
> +	free(dirname_copy);
> +	free(mydata->fatbuf);
> +	free(itr);
> +	free(dotdent);
> +	return ret;
> +}
> diff --git a/fs/fs.c b/fs/fs.c
> index 3cb6b21fe9..a92e060296 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -164,14 +164,15 @@ static struct fstype_info fstypes[] = {
>  		.read = fat_read_file,
>  #ifdef CONFIG_FAT_WRITE
>  		.write = file_fat_write,
> +		.mkdir = fat_mkdir,
>  #else
>  		.write = fs_write_unsupported,
> +		.mkdir = fs_mkdir_unsupported,
>  #endif
>  		.uuid = fs_uuid_unsupported,
>  		.opendir = fat_opendir,
>  		.readdir = fat_readdir,
>  		.closedir = fat_closedir,
> -		.mkdir = fs_mkdir_unsupported,
>  	},
>  #endif
>  #ifdef CONFIG_FS_EXT4
> diff --git a/include/fat.h b/include/fat.h
> index 295da0f243..039b6e03de 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -237,5 +237,6 @@ int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
>  int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
>  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
>  void fat_closedir(struct fs_dir_stream *dirs);
> +int fat_mkdir(const char *dirname);
>  void fat_close(void);
>  #endif /* _FAT_H_ */
> 

[U-Boot] [PATCH 13/17] fs: fat: support mkdir

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 07:14:21PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > In this patch, mkdir support is added to FAT file system.
> > A newly created directory contains only "." and ".." entries.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> The patch does set the creation date of the directory according to the
> real time clock but to 1980-01-01T00:00:00Z.
> 
> $ ls /mnt/dir1/ -la --full-time
> drwxr-xr-x 2 root root  2048 1980-01-01 01:00:00.000000000 +0100 dir3
> 
> Please, set the time correctly if the real time clock is available.

Good point, but I'd like to put this issue into future TODO list
as it will end up introducing a new API, such as gettimeofday()?

(and this is not a FAT-specific issue.)

Thanks,
-Takahiro AKASHI

> Best regards
> 
> Heinrich
> 
> > ---
> >  fs/fat/fat_write.c | 138 +++++++++++++++++++++++++++++++++++++++++++++
> >  fs/fs.c            |   3 +-
> >  include/fat.h      |   1 +
> >  3 files changed, 141 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index cc45a33876..781883c9f4 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -1185,3 +1185,141 @@ int file_fat_write(const char *filename, void *buffer, loff_t offset,
> >  {
> >  	return file_fat_write_at(filename, offset, buffer, maxsize, actwrite);
> >  }
> > +
> > +int fat_mkdir(const char *new_dirname)
> > +{
> > +	dir_entry *retdent;
> > +	fsdata datablock = { .fatbuf = NULL, };
> > +	fsdata *mydata = &datablock;
> > +	fat_itr *itr = NULL;
> > +	char *dirname_copy, *parent, *dirname;
> > +	char l_dirname[VFAT_MAXLEN_BYTES];
> > +	int ret = -1;
> > +	loff_t actwrite;
> > +	unsigned int bytesperclust;
> > +	dir_entry *dotdent = NULL;
> > +
> > +	dirname_copy = strdup(new_dirname);
> > +	if (!dirname_copy)
> > +		goto exit;
> > +
> > +	split_filename(dirname_copy, &parent, &dirname);
> > +	if (!strlen(dirname)) {
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	if (normalize_longname(l_dirname, dirname)) {
> > +		printf("FAT: illegal filename (%s)\n", dirname);
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	itr = malloc_cache_aligned(sizeof(fat_itr));
> > +	if (!itr) {
> > +		ret = -ENOMEM;
> > +		goto exit;
> > +	}
> > +
> > +	ret = fat_itr_root(itr, &datablock);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	total_sector = datablock.bs_total_sect;
> > +	if (total_sector == 0)
> > +		total_sector = (int)cur_part_info.size; /* cast of lbaint_t */
> > +
> > +	ret = fat_itr_resolve(itr, parent, TYPE_DIR);
> > +	if (ret) {
> > +		printf("%s: doesn't exist (%d)\n", parent, ret);
> > +		goto exit;
> > +	}
> > +
> > +	retdent = find_directory_entry(itr, l_dirname);
> > +
> > +	if (retdent) {
> > +		printf("%s: already exists\n", l_dirname);
> > +		ret = -EEXIST;
> > +		goto exit;
> > +	} else {
> > +		if (itr->is_root) {
> > +			/* root dir cannot have "." or ".." */
> > +			if (!strcmp(l_dirname, ".") ||
> > +			    !strcmp(l_dirname, "..")) {
> > +				ret = -EINVAL;
> > +				goto exit;
> > +			}
> > +		}
> > +
> > +		if (!itr->dent) {
> > +			printf("Error: allocating new dir entry\n");
> > +			ret = -EIO;
> > +			goto exit;
> > +		}
> > +
> > +		memset(itr->dent, 0, sizeof(*itr->dent));
> > +
> > +		/* Set short name to set alias checksum field in dir_slot */
> > +		set_name(itr->dent, dirname);
> > +		fill_dir_slot(itr, dirname);
> > +
> > +		/* Set attribute as archieve for regular file */
> > +		fill_dentry(itr->fsdata, itr->dent, dirname, 0, 0,
> > +							ATTR_DIR | ATTR_ARCH);
> > +
> > +		retdent = itr->dent;
> > +	}
> > +
> > +	/* Default entries */
> > +	bytesperclust = mydata->clust_size * mydata->sect_size;
> > +	dotdent = malloc_cache_aligned(bytesperclust);
> > +	if (!dotdent) {
> > +		ret = -ENOMEM;
> > +		goto exit;
> > +	}
> > +	memset(dotdent, 0, bytesperclust);
> > +
> > +	memcpy(dotdent[0].name, ".       ", 8);
> > +	memcpy(dotdent[0].ext, "   ", 3);
> > +	dotdent[0].attr = ATTR_DIR | ATTR_ARCH;
> > +
> > +	memcpy(dotdent[1].name, "..      ", 8);
> > +	memcpy(dotdent[1].ext, "   ", 3);
> > +	dotdent[1].attr = ATTR_DIR | ATTR_ARCH;
> > +	set_start_cluster(mydata, &dotdent[1], itr->start_clust);
> > +
> > +	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
> > +								&actwrite);
> > +	if (ret < 0) {
> > +		printf("Error: writing contents\n");
> > +		goto exit;
> > +	}
> > +	/* Write twice for "." */
> > +	set_start_cluster(mydata, &dotdent[0], START(retdent));
> > +	ret = set_contents(mydata, retdent, 0, (__u8 *)dotdent, bytesperclust,
> > +								&actwrite);
> > +	if (ret < 0) {
> > +		printf("Error: writing contents\n");
> > +		goto exit;
> > +	}
> > +
> > +	/* Flush fat buffer */
> > +	ret = flush_dirty_fat_buffer(mydata);
> > +	if (ret) {
> > +		printf("Error: flush fat buffer\n");
> > +		goto exit;
> > +	}
> > +
> > +	/* Write directory table to device */
> > +	ret = set_cluster(mydata, itr->clust, itr->block,
> > +			mydata->clust_size * mydata->sect_size);
> > +	if (ret)
> > +		printf("Error: writing directory entry\n");
> > +
> > +exit:
> > +	free(dirname_copy);
> > +	free(mydata->fatbuf);
> > +	free(itr);
> > +	free(dotdent);
> > +	return ret;
> > +}
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 3cb6b21fe9..a92e060296 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -164,14 +164,15 @@ static struct fstype_info fstypes[] = {
> >  		.read = fat_read_file,
> >  #ifdef CONFIG_FAT_WRITE
> >  		.write = file_fat_write,
> > +		.mkdir = fat_mkdir,
> >  #else
> >  		.write = fs_write_unsupported,
> > +		.mkdir = fs_mkdir_unsupported,
> >  #endif
> >  		.uuid = fs_uuid_unsupported,
> >  		.opendir = fat_opendir,
> >  		.readdir = fat_readdir,
> >  		.closedir = fat_closedir,
> > -		.mkdir = fs_mkdir_unsupported,
> >  	},
> >  #endif
> >  #ifdef CONFIG_FS_EXT4
> > diff --git a/include/fat.h b/include/fat.h
> > index 295da0f243..039b6e03de 100644
> > --- a/include/fat.h
> > +++ b/include/fat.h
> > @@ -237,5 +237,6 @@ int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
> >  int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
> >  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
> >  void fat_closedir(struct fs_dir_stream *dirs);
> > +int fat_mkdir(const char *dirname);
> >  void fat_close(void);
> >  #endif /* _FAT_H_ */
> > 
> 

[U-Boot] [PATCH 14/17] cmd: fat: add fatmkdir command

[AKASHI Takahiro]

In this patch, a new command, fatmkdir, is added.

Please note that, as there is no notion of "current directory" on u-boot,
a directory name specified must contains an absolute directory path as
a parent directory. Otherwise, "/" (root directory) is assumed.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/fat.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/cmd/fat.c b/cmd/fat.c
index 2a5f7bfc26..136a5114c6 100644
--- a/cmd/fat.c
+++ b/cmd/fat.c
@@ -151,4 +151,17 @@ U_BOOT_CMD(
 	"    - write file 'filename' from the address 'addr' in RAM\n"
 	"      to 'dev' on 'interface'"
 );
+
+static int do_fat_mkdir(cmd_tbl_t *cmdtp, int flag, int argc,
+						char * const argv[])
+{
+	return do_mkdir(cmdtp, flag, argc, argv, FS_TYPE_FAT);
+}
+
+U_BOOT_CMD(
+	fatmkdir,	4,	1,	do_fat_mkdir,
+	"create a directory",
+	"<interface> [<dev[:part]>] <directory>\n"
+	"    - create a directory in 'dev' on 'interface'"
+);
 #endif
-- 
2.17.0

[U-Boot] [PATCH 14/17] cmd: fat: add fatmkdir command

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> In this patch, a new command, fatmkdir, is added.
> 
> Please note that, as there is no notion of "current directory" on u-boot,
> a directory name specified must contains an absolute directory path as
> a parent directory. Otherwise, "/" (root directory) is assumed.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---

Please, provide a Python test with an additional patch.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [PATCH 14/17] cmd: fat: add fatmkdir command

[AKASHI Takahiro]

On Fri, Jul 20, 2018 at 07:09:17PM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > In this patch, a new command, fatmkdir, is added.
> > 
> > Please note that, as there is no notion of "current directory" on u-boot,
> > a directory name specified must contains an absolute directory path as
> > a parent directory. Otherwise, "/" (root directory) is assumed.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> 
> Please, provide a Python test with an additional patch.

Please see my reply to Simon's/Tom's comments.

> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Thanks a lot for your review.
-Takahiro AKASHI

[U-Boot] [PATCH 15/17] efi_loader: file: support creating a directory

[AKASHI Takahiro]

In efi world, there is no obvious "mkdir" interface, instead, Open()
with EFI_FILE_MODE_CREATE in mode parameter and EFI_FILE_DIRECTORY
in attributes parameter creates a directory.

In this patch, efi_file_open() is extended so as to accept such
a combination of parameters and call u-boot's mkdir interface for
expected action.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_file.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index e6a15bcb52..6ec98c8022 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -130,7 +130,8 @@ static int sanitize_path(char *path)
  * With windoze style backlashes, ofc.
  */
 static struct efi_file_handle *file_open(struct file_system *fs,
-		struct file_handle *parent, s16 *file_name, u64 mode)
+		struct file_handle *parent, s16 *file_name, u64 mode,
+		u64 attributes)
 {
 	struct file_handle *fh;
 	char f0[MAX_UTF8_PER_UTF16] = {0};
@@ -173,7 +174,12 @@ static struct efi_file_handle *file_open(struct file_system *fs,
 		if (set_blk_dev(fh))
 			goto error;
 
-		if (!((mode & EFI_FILE_MODE_CREATE) || fs_exists(fh->path)))
+		if ((mode & EFI_FILE_MODE_CREATE) &&
+		    (attributes & EFI_FILE_DIRECTORY)) {
+			if (fs_mkdir(fh->path))
+				goto error;
+		} else if (!((mode & EFI_FILE_MODE_CREATE) ||
+			     fs_exists(fh->path)))
 			goto error;
 
 		/* figure out if file is a directory: */
@@ -199,7 +205,7 @@ static efi_status_t EFIAPI efi_file_open(struct efi_file_handle *file,
 	EFI_ENTRY("%p, %p, \"%ls\", %llx, %llu", file, new_handle, file_name,
 		  open_mode, attributes);
 
-	*new_handle = file_open(fh->fs, fh, file_name, open_mode);
+	*new_handle = file_open(fh->fs, fh, file_name, open_mode, attributes);
 	if (!*new_handle)
 		return EFI_EXIT(EFI_NOT_FOUND);
 
@@ -598,7 +604,7 @@ efi_open_volume(struct efi_simple_file_system_protocol *this,
 
 	EFI_ENTRY("%p, %p", this, root);
 
-	*root = file_open(fs, NULL, NULL, 0);
+	*root = file_open(fs, NULL, NULL, 0, 0);
 
 	return EFI_EXIT(EFI_SUCCESS);
 }
-- 
2.17.0

[U-Boot] [PATCH 16/17] efi_loader: implement a pseudo "file delete"

[AKASHI Takahiro]

This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
Returning EFI_SUCCESS can cheat SCT execution.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_file.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 6ec98c8022..12044a0c71 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
 	return EFI_EXIT(file_close(fh));
 }
 
+static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file,
+					  efi_uintn_t *buffer_size,
+					  void *buffer);
+
 static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file)
 {
 	struct file_handle *fh = to_fh(file);
+	efi_uintn_t size = 0;
 	EFI_ENTRY("%p", file);
+
+	/* TODO: implement real 'delete' */
+	efi_file_write(file, &size, NULL);
 	file_close(fh);
-	return EFI_EXIT(EFI_WARN_DELETE_FAILURE);
+	return EFI_SUCCESS;
 }
 
 static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
-- 
2.17.0

[U-Boot] [PATCH 17/17] fs-test: fix false positive error at Test Case 12

[AKASHI Takahiro]

The error message to be matched is wrong. Fix it.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 test/fs/fs-test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
index 2e8d5ee4df..7b0c5ea56f 100755
--- a/test/fs/fs-test.sh
+++ b/test/fs/fs-test.sh
@@ -522,7 +522,7 @@ function check_results() {
 		"TC11: 1MB write to $3.w - content verified"
 
 	# Check lookup of 'dot' directory
-	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write file'
+	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write'
 	pass_fail "TC12: 1MB write to . - write denied"
 
 	# Check directory traversal
-- 
2.17.0

[U-Boot] [PATCH 17/17] fs-test: fix false positive error at Test Case 12

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> The error message to be matched is wrong. Fix it.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  test/fs/fs-test.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> index 2e8d5ee4df..7b0c5ea56f 100755
> --- a/test/fs/fs-test.sh
> +++ b/test/fs/fs-test.sh
> @@ -522,7 +522,7 @@ function check_results() {
>  		"TC11: 1MB write to $3.w - content verified"
>  
>  	# Check lookup of 'dot' directory
> -	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write file'
> +	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write'
>  	pass_fail "TC12: 1MB write to . - write denied"
>  
>  	# Check directory traversal
> 

Tom suggested in
https://lists.denx.de/pipermail/u-boot/2018-July/336378.html that the
comment headers in test/fs/fs-test.sh should be updated to reflect the
total number of passes and fails.

Best regards

Heinrich

[U-Boot] [PATCH 17/17] fs-test: fix false positive error at Test Case 12

[AKASHI Takahiro]

On Sun, Jul 29, 2018 at 09:02:00AM +0200, Heinrich Schuchardt wrote:
> On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> > The error message to be matched is wrong. Fix it.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  test/fs/fs-test.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> > index 2e8d5ee4df..7b0c5ea56f 100755
> > --- a/test/fs/fs-test.sh
> > +++ b/test/fs/fs-test.sh
> > @@ -522,7 +522,7 @@ function check_results() {
> >  		"TC11: 1MB write to $3.w - content verified"
> >  
> >  	# Check lookup of 'dot' directory
> > -	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write file'
> > +	grep -A4 "Test Case 12 " "$1" | grep -q 'Unable to write'
> >  	pass_fail "TC12: 1MB write to . - write denied"
> >  
> >  	# Check directory traversal
> > 
> 
> Tom suggested in
> https://lists.denx.de/pipermail/u-boot/2018-July/336378.html that the
> comment headers in test/fs/fs-test.sh should be updated to reflect the
> total number of passes and fails.

Thank you for this heads-up.
I'm now trying to rewrite fs-test.sh (at least part of it) on top of
py.test framework.
It will take some time as I'm still learning how to write tests for py.test.

-Takahiro AKASHI


> Best regards
> 
> Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[Heinrich Schuchardt]

On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> This patch series is an attempt to address FAT write related issues
> in an effort of running UEFI SCT (Self-Certification Test) to verify
> UEFI support on u-boot.
> 
> SCT is a test platform as well as an extentisive collection of test
> cases for UEFI specification. It can run all the tests automatically
> and save test results to dedicated log files.
> 
> AFAIK, what's missing in the current fat file system to safely run
> SCT without errors (I don't mean test case failures) are:
> * write a file located under sub-directories
> * write a file with non-zero offset
> * delete a file
> * create a directory
> 
> Patch#1 to patch#6 are some sort of preparatory ones.
> Patch#7 implements write with sub-directories path.
> Patch#8 to patch#10 implement write with non-zero offset.
> Patch#11 to patch#15 are related to creating a directory.
> Patch#16 provides delete, but it doesn't actually delete a file
> but instead return 0 with purging a file content, which is good
> enough to run SCT for now.
> Finally, patch#17 fixes a minor bug in fs-test.sh.
> 
> I applied this patch set on top of v2018.07 along with a couple of
> yet-to--be-upstreamed UEFI-related patches, and could successfully
> run unmodified SCT[1] on qemu-arm.
> 
> [1] http://uefi.org/testtools

Thanks Takahiro for getting this all implemented.

I have some questions concerning code pages:

The FAT file system contains a short name and a long name. The short 
name is in the 8-bit code page of the system. The long name is 16bit 
Unicode.

Which local code page do you assume? I would suggest to use 437 as we do 
in some other drivers.

With your patch fat_mkdir() and fat_file_write() do not yet support 
UTF-8 as file names and convert this to the 437 local page for the short 
name and to UTF-16 for the long name.

Without this conversoin we will not be able to implement the EFI 
specification.

normalize_longname() simply ignores codes 0x80-0xFF. Please, have a look at

https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Microsoft Extensible Firmware Initiative FAT32 File System Specification
FAT: General Overview of On-Disk Format
Version 1.03, December 6, 2000

Shouldn't we implement the base-name algorithm as described in the paper?

Best regards

Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[Heinrich Schuchardt]

Hello Tom, hello Alex,

I have been testing the patches. They are working fine for ASCII file
names. To support Unicode file names extra work will be needed. But
probably we should postpone this to a later patch series.

There are some dependencies with my work for correcting errors in
Unicode handling for the EFI branch. Should the patches be passed via
efi-next?

Best regards

Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[AKASHI Takahiro]

On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> Hello Tom, hello Alex,
> 
> I have been testing the patches. They are working fine for ASCII file
> names. To support Unicode file names extra work will be needed. But
> probably we should postpone this to a later patch series.

I absolutely agree.
To be clear, the aim of my FAT write patchset is to successfully run
SCT to evaluate and verify UEFI-related features rather than to make
the implementation comply with UEFI specification.

For the latter purpose, separate patches would be compiled as it might
require time-consuming efforts.
(I'm sure that SCT's file (media access) protocol test causes lots of
test case failures.)

Thanks,
-Takahiro AKASHI

> There are some dependencies with my work for correcting errors in
> Unicode handling for the EFI branch. Should the patches be passed via
> efi-next?
> 
> Best regards
> 
> Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[Tom Rini]

On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> Hello Tom, hello Alex,
> 
> I have been testing the patches. They are working fine for ASCII file
> names. To support Unicode file names extra work will be needed. But
> probably we should postpone this to a later patch series.
> 
> There are some dependencies with my work for correcting errors in
> Unicode handling for the EFI branch. Should the patches be passed via
> efi-next?

Yes, a follow-up series makes sense, and yes, efi-next for the patches
themselves sounds fine, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180723/cd0cef29/attachment.sig>

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[Heinrich Schuchardt]

On 07/23/2018 04:46 PM, Tom Rini wrote:
> On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
>> Hello Tom, hello Alex,
>>
>> I have been testing the patches. They are working fine for ASCII file
>> names. To support Unicode file names extra work will be needed. But
>> probably we should postpone this to a later patch series.
>>
>> There are some dependencies with my work for correcting errors in
>> Unicode handling for the EFI branch. Should the patches be passed via
>> efi-next?
> 
> Yes, a follow-up series makes sense, and yes, efi-next for the patches
> themselves sounds fine, thanks!
> 

Hello Takahiro,

could you, please, resubmit the series with the few changes needed so
that Alex can consider them for efi-next.

Cheers

Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[AKASHI Takahiro]

On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
> On 07/23/2018 04:46 PM, Tom Rini wrote:
> > On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> >> Hello Tom, hello Alex,
> >>
> >> I have been testing the patches. They are working fine for ASCII file
> >> names. To support Unicode file names extra work will be needed. But
> >> probably we should postpone this to a later patch series.
> >>
> >> There are some dependencies with my work for correcting errors in
> >> Unicode handling for the EFI branch. Should the patches be passed via
> >> efi-next?
> > 
> > Yes, a follow-up series makes sense, and yes, efi-next for the patches
> > themselves sounds fine, thanks!
> > 
> 
> Hello Takahiro,
> 
> could you, please, resubmit the series with the few changes needed so
> that Alex can consider them for efi-next.

Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
its function.

What's more, I ran fs-test.sh again, and all tests passed, either against
fat16 or fat32. Do we still want to update a test result in fs-test.sh?
(removing it would be the easiest.)

Thanks,
-Takahiro AKASHI


> Cheers
> 
> Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[AKASHI Takahiro]

On Tue, Aug 07, 2018 at 02:38:34PM +0900, AKASHI Takahiro wrote:
> On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
> > On 07/23/2018 04:46 PM, Tom Rini wrote:
> > > On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> > >> Hello Tom, hello Alex,
> > >>
> > >> I have been testing the patches. They are working fine for ASCII file
> > >> names. To support Unicode file names extra work will be needed. But
> > >> probably we should postpone this to a later patch series.
> > >>
> > >> There are some dependencies with my work for correcting errors in
> > >> Unicode handling for the EFI branch. Should the patches be passed via
> > >> efi-next?
> > > 
> > > Yes, a follow-up series makes sense, and yes, efi-next for the patches
> > > themselves sounds fine, thanks!
> > > 
> > 
> > Hello Takahiro,
> > 
> > could you, please, resubmit the series with the few changes needed so
> > that Alex can consider them for efi-next.
> 
> Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
> changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
> is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
> its function.

See:
https://git.linaro.org/people/takahiro.akashi/u-boot.git fat_write

> What's more, I ran fs-test.sh again, and all tests passed, either against
> fat16 or fat32. Do we still want to update a test result in fs-test.sh?
> (removing it would be the easiest.)

I've almost finished to convert fs-test.sh to a Python script, but
it will take some time to add more tests against my patch set as
my priority right now is on examining a test result from SCT.

-Takahiro AKASHI


> Thanks,
> -Takahiro AKASHI
> 
> 
> > Cheers
> > 
> > Heinrich

[U-Boot] [PATCH 00/17] fs: fat: extend FAT write operations

[Heinrich Schuchardt]

On 08/07/2018 07:38 AM, AKASHI Takahiro wrote:
> On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
>> On 07/23/2018 04:46 PM, Tom Rini wrote:
>>> On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
>>>> Hello Tom, hello Alex,
>>>>
>>>> I have been testing the patches. They are working fine for ASCII file
>>>> names. To support Unicode file names extra work will be needed. But
>>>> probably we should postpone this to a later patch series.
>>>>
>>>> There are some dependencies with my work for correcting errors in
>>>> Unicode handling for the EFI branch. Should the patches be passed via
>>>> efi-next?
>>>
>>> Yes, a follow-up series makes sense, and yes, efi-next for the patches
>>> themselves sounds fine, thanks!
>>>
>>
>> Hello Takahiro,
>>
>> could you, please, resubmit the series with the few changes needed so
>> that Alex can consider them for efi-next.
> 
> Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
> changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
> is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
> its function.

Please, add a patch doing the reverting or rebase patch 5.

> 
> What's more, I ran fs-test.sh again, and all tests passed, either against
> fat16 or fat32. Do we still want to update a test result in fs-test.sh?
> (removing it would be the easiest.)

As long as we have no better tests in place updating those comments is
appropriate.

In my review of patch 01/17 I indicated an error in evaluating the
sector number.

Best regards

Heinrich