[U-Boot] [PATCH v2 0/7] Add support for symlink creation in EXT4

[U-Boot] [PATCH v2 0/7] Add support for symlink creation in EXT4

[Jean-Jacques Hiblot]

This series adds support for the creation of symbolic links on ext4
file-systems.
The motivation behind this work is to have the ability to "do" the job
of update-alternatives in u-boot.
Firmware on TI's platform are usually managed with update-alternatives and
are thus targeted by a symbolic link. In some situations we need the
ability to select an alternate firmware before the linux kernel is started
so that when a early driver needing the firmware comes up, it can be fed
the firmware of our choice.

Tested on a am57xx_evm, using a EXT4 partition on external SDcard.
The filesystem can be checked later with: fsck.ext4 -f <dev>

usage example:
=> ln mmc 0:2 zImage /boot/the_linux_kernel

Changes in v2:
- Prevent write access if metadata checksum is enabled
- Add a FS integrity check at the end of the FS tests
- Fix issue in ext4fs_delete_file() when target in not stored in an
  allocated block
- Added python tests for symlinks under sandbox

Jean-Jacques Hiblot (7):
  fs: ext4: do not allow writes if metadata checksum is active
  test: fs: disable the metadata checksums on ext4 filesystems
  test: fs: run fsck after on fs image after the tests are run
  fs: ext4: constify the buffer passed to write functions
  fs: ext4: Add support for the creation of symbolic links
  fs: Add a new command to create symbolic links
  test: fs: Added tests for symlinks

 cmd/fs.c                              |  14 +++
 fs/ext4/ext4_common.c                 |   4 +-
 fs/ext4/ext4_common.h                 |   2 +-
 fs/ext4/ext4_write.c                  |  76 ++++++++++++----
 fs/fs.c                               |  44 +++++++++
 include/ext4fs.h                      |   6 +-
 include/fs.h                          |   2 +
 test/py/tests/test_fs/conftest.py     |  91 +++++++++++++++++++
 test/py/tests/test_fs/fstest_defs.py  |   3 +
 test/py/tests/test_fs/test_symlink.py | 125 ++++++++++++++++++++++++++
 10 files changed, 345 insertions(+), 22 deletions(-)
 create mode 100644 test/py/tests/test_fs/test_symlink.py

-- 
2.17.1

[U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active

[Jean-Jacques Hiblot]

u-boot does not supports updating the metadata chacksums

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

Changes in v2:
- Prevent write access if metadata checksum is enabled

 fs/ext4/ext4_write.c | 12 ++++++++++--
 include/ext4fs.h     |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index a7f543f7df..1de29236f0 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 
 	g_parent_inode = zalloc(fs->inodesz);
 	if (!g_parent_inode)
-		goto fail;
+		goto fail_ext4fs_init;
 
 	if (ext4fs_init() != 0) {
 		printf("error in File System init\n");
-		return -1;
+		goto fail_ext4fs_init;
+	}
+
+	if (le32_to_cpu(fs->sb->feature_ro_compat) &
+		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
+		printf("u-boot doesn't support updating the metadata checksums yet\n");
+		goto fail;
 	}
+
 	inodes_per_block = fs->blksz / fs->inodesz;
 	parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
 	if (parent_inodeno == -1)
@@ -990,6 +997,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	return 0;
 fail:
 	ext4fs_deinit();
+fail_ext4fs_init:
 	free(inode_buffer);
 	free(g_parent_inode);
 	free(temp_ptr);
diff --git a/include/ext4fs.h b/include/ext4fs.h
index bb55639107..bcf440364e 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -32,6 +32,7 @@
 #define EXT4_EXTENTS_FL		0x00080000 /* Inode uses extents */
 #define EXT4_EXT_MAGIC			0xf30a
 #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM	0x0010
+#define EXT4_FEATURE_RO_COMPAT_METADATA_CSUM	0x0400
 #define EXT4_FEATURE_INCOMPAT_EXTENTS	0x0040
 #define EXT4_FEATURE_INCOMPAT_64BIT	0x0080
 #define EXT4_INDIRECT_BLOCKS		12
-- 
2.17.1

[U-Boot] [PATCH v2 2/7] test: fs: disable the metadata checksums on ext4 filesystems

[Jean-Jacques Hiblot]

If the metadata checksums are enabled, all write operations will fail.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

Changes in v2: None

 test/py/tests/test_fs/conftest.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
index 43eeb4be0b..745ed0ed38 100644
--- a/test/py/tests/test_fs/conftest.py
+++ b/test/py/tests/test_fs/conftest.py
@@ -143,6 +143,8 @@ def mk_fs(config, fs_type, size, id):
         mkfs_opt = '-F 16'
     elif fs_type == 'fat32':
         mkfs_opt = '-F 32'
+    elif fs_type == 'ext4':
+        mkfs_opt = '-O ^metadata_csum'
     else:
         mkfs_opt = ''
 
-- 
2.17.1

[U-Boot] [PATCH v2 3/7] test: fs: run fsck after on fs image after the tests are run

[Jean-Jacques Hiblot]

This is to check the integrity of the FS after the test operations.
This is useful to make sure that the operations are implemented properly,
and are not going to create silent corruptions.
Currently only the integrity of EXT4 filesystems is checked.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

Changes in v2:
- Add a FS integrity check at the end of the FS tests

 test/py/tests/test_fs/conftest.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
index 745ed0ed38..e742cda662 100644
--- a/test/py/tests/test_fs/conftest.py
+++ b/test/py/tests/test_fs/conftest.py
@@ -216,6 +216,15 @@ def mount_fs(fs_type, device, mount_point):
     except CalledProcessError:
         raise
 
+
+def fsck(img, fs_type):
+    try:
+        if fs_type == 'ext4':
+            check_call('fsck.ext4 -n -f %s' % img, shell=True)
+    except CalledProcessError:
+        raise
+
+
 def umount_fs(mount_point):
     """Unmount a volume.
 
@@ -336,6 +345,7 @@ def fs_obj_basic(request, u_boot_config):
         yield [fs_ubtype, fs_img, md5val]
     finally:
         umount_fs(mount_dir)
+        fsck(fs_img, fs_type)
         call('rmdir %s' % mount_dir, shell=True)
         if fs_img:
             call('rm -f %s' % fs_img, shell=True)
@@ -423,6 +433,7 @@ def fs_obj_ext(request, u_boot_config):
         yield [fs_ubtype, fs_img, md5val]
     finally:
         umount_fs(mount_dir)
+        fsck(fs_img, fs_type)
         call('rmdir %s' % mount_dir, shell=True)
         if fs_img:
             call('rm -f %s' % fs_img, shell=True)
@@ -457,6 +468,7 @@ def fs_obj_mkdir(request, u_boot_config):
     else:
         yield [fs_ubtype, fs_img]
     finally:
+        fsck(fs_img, fs_type)
         if fs_img:
             call('rm -f %s' % fs_img, shell=True)
 
@@ -522,6 +534,7 @@ def fs_obj_unlink(request, u_boot_config):
         yield [fs_ubtype, fs_img]
     finally:
         umount_fs(mount_dir)
+        fsck(fs_img, fs_type)
         call('rmdir %s' % mount_dir, shell=True)
         if fs_img:
             call('rm -f %s' % fs_img, shell=True)
-- 
2.17.1

[U-Boot] [PATCH v2 4/7] fs: ext4: constify the buffer passed to write functions

[Jean-Jacques Hiblot]

There is no need to modify the buffer passed to ext4fs_write_file().
The memset() has no utility there, and comes probably from the equivalent
ext4fs_read_file() function.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

Changes in v2: None

 fs/ext4/ext4_common.c |  2 +-
 fs/ext4/ext4_common.h |  2 +-
 fs/ext4/ext4_write.c  | 11 +++++------
 include/ext4fs.h      |  2 +-
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 67e2471bd3..b881482c39 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -190,7 +190,7 @@ uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n)
 	return res;
 }
 
-void put_ext4(uint64_t off, void *buf, uint32_t size)
+void put_ext4(u64 off, const void *buf, u32 size)
 {
 	uint64_t startblock;
 	uint64_t remainder;
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
index 1ee81ab7ce..50155eccfc 100644
--- a/fs/ext4/ext4_common.h
+++ b/fs/ext4/ext4_common.h
@@ -72,7 +72,7 @@ int ext4fs_iget(int inode_no, struct ext2_inode *inode);
 void ext4fs_allocate_blocks(struct ext2_inode *file_inode,
 				unsigned int total_remaining_blocks,
 				unsigned int *total_no_of_block);
-void put_ext4(uint64_t off, void *buf, uint32_t size);
+void put_ext4(u64 off, const void *buf, u32 size);
 struct ext2_block_group *ext4fs_get_group_descriptor
 	(const struct ext_filesystem *fs, uint32_t bg_idx);
 uint64_t ext4fs_bg_get_block_id(const struct ext2_block_group *bg,
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 1de29236f0..51ca1c0916 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -752,7 +752,7 @@ void ext4fs_deinit(void)
  * contigous sectors as ext4fs_read_file
  */
 static int ext4fs_write_file(struct ext2_inode *file_inode,
-			     int pos, unsigned int len, char *buf)
+			     int pos, unsigned int len, const char *buf)
 {
 	int i;
 	int blockcnt;
@@ -764,7 +764,7 @@ static int ext4fs_write_file(struct ext2_inode *file_inode,
 	int delayed_start = 0;
 	int delayed_extent = 0;
 	int delayed_next = 0;
-	char *delayed_buf = NULL;
+	const char *delayed_buf = NULL;
 
 	/* Adjust len so it we can't read past the end of the file. */
 	if (len > filesize)
@@ -816,7 +816,6 @@ static int ext4fs_write_file(struct ext2_inode *file_inode,
 					 (uint32_t) delayed_extent);
 				previous_block_number = -1;
 			}
-			memset(buf, 0, fs->blksz - skipfirst);
 		}
 		buf += fs->blksz - skipfirst;
 	}
@@ -830,8 +829,8 @@ static int ext4fs_write_file(struct ext2_inode *file_inode,
 	return len;
 }
 
-int ext4fs_write(const char *fname, unsigned char *buffer,
-					unsigned long sizebytes)
+int ext4fs_write(const char *fname, const char *buffer,
+		 unsigned long sizebytes)
 {
 	int ret = 0;
 	struct ext2_inode *file_inode = NULL;
@@ -950,7 +949,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 	if (ext4fs_put_metadata(temp_ptr, itable_blkno))
 		goto fail;
 	/* copy the file content into data blocks */
-	if (ext4fs_write_file(file_inode, 0, sizebytes, (char *)buffer) == -1) {
+	if (ext4fs_write_file(file_inode, 0, sizebytes, buffer) == -1) {
 		printf("Error in copying content\n");
 		/* FIXME: Deallocate data blocks */
 		goto fail;
diff --git a/include/ext4fs.h b/include/ext4fs.h
index bcf440364e..d4dcefaa96 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -128,7 +128,7 @@ extern int gindex;
 int ext4fs_init(void);
 void ext4fs_deinit(void);
 int ext4fs_filename_unlink(char *filename);
-int ext4fs_write(const char *fname, unsigned char *buffer,
+int ext4fs_write(const char *fname, const char *buffer,
 		 unsigned long sizebytes);
 int ext4_write_file(const char *filename, void *buf, loff_t offset, loff_t len,
 		    loff_t *actwrite);
-- 
2.17.1

[U-Boot] [PATCH v2 5/7] fs: ext4: Add support for the creation of symbolic links

[Jean-Jacques Hiblot]

Re-use the functions used to write/create a file, to support creation of a
symbolic link.
The difference with a regular file are small:
- The inode mode is flagged with S_IFLNK instead of S_IFREG
- The ext2_dirent's filetype is FILETYPE_SYMLINK instead of FILETYPE_REG
- Instead of storing the content of a file in allocated blocks, the path
to the target is stored. And if the target's path is short enough, no block
is allocated and the target's path is stored in ext2_inode.b.symlink

As with regulars files, if a file/symlink with the same name exits, it is
unlinked first and then re-created.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

Changes in v2:
- Fix issue in ext4fs_delete_file() when target in not stored in an
  allocated block

 fs/ext4/ext4_common.c |  2 +-
 fs/ext4/ext4_write.c  | 55 +++++++++++++++++++++++++++++++++++--------
 include/ext4fs.h      |  3 ++-
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index b881482c39..ac04a27728 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -607,7 +607,7 @@ restart_read:
 		dir->direntlen = cpu_to_le16(fs->blksz - totalbytes);
 
 	dir->namelen = strlen(filename);
-	dir->filetype = FILETYPE_REG;	/* regular file */
+	dir->filetype = file_type;
 	temp_dir = (char *)dir;
 	temp_dir = temp_dir + sizeof(struct ext2_dirent);
 	memcpy(temp_dir, filename, strlen(filename));
diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 51ca1c0916..52720a80f0 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -431,7 +431,7 @@ fail:
 	free(journal_buffer);
 }
 
-static int ext4fs_delete_file(int inodeno)
+static int ext4fs_delete_file(int inodeno, int type)
 {
 	struct ext2_inode inode;
 	short status;
@@ -465,6 +465,15 @@ static int ext4fs_delete_file(int inodeno)
 	if (le32_to_cpu(inode.size) % fs->blksz)
 		no_blocks++;
 
+	/*
+	 * special case for symlinks whose target are small enough that
+	 *it fits in struct ext2_inode.b.symlink: no block had been allocated
+	 */
+	if (type == FILETYPE_SYMLINK &&
+	    le32_to_cpu(inode.size) <= sizeof(inode.b.symlink)) {
+		no_blocks = 0;
+	}
+
 	if (le32_to_cpu(inode.flags) & EXT4_EXTENTS_FL) {
 		/* FIXME delete extent index blocks, i.e. eh_depth >= 1 */
 		struct ext4_extent_header *eh =
@@ -830,7 +839,7 @@ static int ext4fs_write_file(struct ext2_inode *file_inode,
 }
 
 int ext4fs_write(const char *fname, const char *buffer,
-		 unsigned long sizebytes)
+		 unsigned long sizebytes, int type)
 {
 	int ret = 0;
 	struct ext2_inode *file_inode = NULL;
@@ -853,8 +862,12 @@ int ext4fs_write(const char *fname, const char *buffer,
 	struct ext2_block_group *bgd = NULL;
 	struct ext_filesystem *fs = get_fs();
 	ALLOC_CACHE_ALIGN_BUFFER(char, filename, 256);
+	bool store_link_in_inode = false;
 	memset(filename, 0x00, 256);
 
+	if (type != FILETYPE_REG && type != FILETYPE_SYMLINK)
+		return -1;
+
 	g_parent_inode = zalloc(fs->inodesz);
 	if (!g_parent_inode)
 		goto fail_ext4fs_init;
@@ -884,7 +897,7 @@ int ext4fs_write(const char *fname, const char *buffer,
 	/* check if the filename is already present in root */
 	existing_file_inodeno = ext4fs_filename_unlink(filename);
 	if (existing_file_inodeno != -1) {
-		ret = ext4fs_delete_file(existing_file_inodeno);
+		ret = ext4fs_delete_file(existing_file_inodeno, type);
 		fs->first_pass_bbmap = 0;
 		fs->curr_blkno = 0;
 
@@ -893,8 +906,16 @@ int ext4fs_write(const char *fname, const char *buffer,
 		if (ret)
 			goto fail;
 	}
-	/* calucalate how many blocks required */
-	bytes_reqd_for_file = sizebytes;
+
+	/* calculate how many blocks required */
+	if (type == FILETYPE_SYMLINK &&
+	    sizebytes <= sizeof(file_inode->b.symlink)) {
+		store_link_in_inode = true;
+		bytes_reqd_for_file = 0;
+	} else {
+		bytes_reqd_for_file = sizebytes;
+	}
+
 	blks_reqd_for_file = lldiv(bytes_reqd_for_file, fs->blksz);
 	if (do_div(bytes_reqd_for_file, fs->blksz) != 0) {
 		blks_reqd_for_file++;
@@ -907,7 +928,7 @@ int ext4fs_write(const char *fname, const char *buffer,
 		goto fail;
 	}
 
-	inodeno = ext4fs_update_parent_dentry(filename, FILETYPE_REG);
+	inodeno = ext4fs_update_parent_dentry(filename, type);
 	if (inodeno == -1)
 		goto fail;
 	/* prepare file inode */
@@ -915,14 +936,23 @@ int ext4fs_write(const char *fname, const char *buffer,
 	if (!inode_buffer)
 		goto fail;
 	file_inode = (struct ext2_inode *)inode_buffer;
-	file_inode->mode = cpu_to_le16(S_IFREG | S_IRWXU |
-	    S_IRGRP | S_IROTH | S_IXGRP | S_IXOTH);
+	file_inode->size = cpu_to_le32(sizebytes);
+	if (type == FILETYPE_SYMLINK) {
+		file_inode->mode = cpu_to_le16(S_IFLNK | S_IRWXU | S_IRWXG |
+					       S_IRWXO);
+		if (store_link_in_inode) {
+			strncpy(file_inode->b.symlink, buffer, sizebytes);
+			sizebytes = 0;
+		}
+	} else {
+		file_inode->mode = cpu_to_le16(S_IFREG | S_IRWXU | S_IRGRP |
+					       S_IROTH | S_IXGRP | S_IXOTH);
+	}
 	/* ToDo: Update correct time */
 	file_inode->mtime = cpu_to_le32(timestamp);
 	file_inode->atime = cpu_to_le32(timestamp);
 	file_inode->ctime = cpu_to_le32(timestamp);
 	file_inode->nlinks = cpu_to_le16(1);
-	file_inode->size = cpu_to_le32(sizebytes);
 
 	/* Allocate data blocks */
 	ext4fs_allocate_blocks(file_inode, blocks_remaining,
@@ -1015,7 +1045,7 @@ int ext4_write_file(const char *filename, void *buf, loff_t offset,
 		return -1;
 	}
 
-	ret = ext4fs_write(filename, buf, len);
+	ret = ext4fs_write(filename, buf, len, FILETYPE_REG);
 	if (ret) {
 		printf("** Error ext4fs_write() **\n");
 		goto fail;
@@ -1030,3 +1060,8 @@ fail:
 
 	return -1;
 }
+
+int ext4fs_create_link(const char *target, const char *fname)
+{
+	return ext4fs_write(fname, target, strlen(target), FILETYPE_SYMLINK);
+}
diff --git a/include/ext4fs.h b/include/ext4fs.h
index d4dcefaa96..946dc6a302 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -129,9 +129,10 @@ int ext4fs_init(void);
 void ext4fs_deinit(void);
 int ext4fs_filename_unlink(char *filename);
 int ext4fs_write(const char *fname, const char *buffer,
-		 unsigned long sizebytes);
+				 unsigned long sizebytes, int type);
 int ext4_write_file(const char *filename, void *buf, loff_t offset, loff_t len,
 		    loff_t *actwrite);
+int ext4fs_create_link(const char *target, const char *fname);
 #endif
 
 struct ext_filesystem *get_fs(void);
-- 
2.17.1

[U-Boot] [PATCH v2 6/7] fs: Add a new command to create symbolic links

[Jean-Jacques Hiblot]

The command line is:
ln <interface> <dev[:part]> target linkname

Currently symbolic links are supported only in ext4 and only if the option
CMD_EXT4_WRITE is enabled.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

Changes in v2: None

 cmd/fs.c     | 14 ++++++++++++++
 fs/fs.c      | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/fs.h |  2 ++
 3 files changed, 60 insertions(+)

diff --git a/cmd/fs.c b/cmd/fs.c
index 8064a1c84d..ba0e08cd12 100644
--- a/cmd/fs.c
+++ b/cmd/fs.c
@@ -74,6 +74,20 @@ U_BOOT_CMD(
 	"      device type 'interface' instance 'dev'."
 )
 
+static int do_ln_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[])
+{
+	return do_ln(cmdtp, flag, argc, argv, FS_TYPE_ANY);
+}
+
+U_BOOT_CMD(
+	ln,	5,	1,	do_ln_wrapper,
+	"Create a symbolic link",
+	"<interface> <dev[:part]> target linkname\n"
+	"    - create a symbolic link to 'target' with the name 'linkname' on\n"
+	"      device type 'interface' instance 'dev'."
+)
+
 static int do_fstype_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
 				char * const argv[])
 {
diff --git a/fs/fs.c b/fs/fs.c
index 7fd22101ef..953a38dadc 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -90,6 +90,11 @@ static inline int fs_write_unsupported(const char *filename, void *buf,
 	return -1;
 }
 
+static inline int fs_ln_unsupported(const char *filename, const char *target)
+{
+	return -1;
+}
+
 static inline void fs_close_unsupported(void)
 {
 }
@@ -154,6 +159,7 @@ struct fstype_info {
 	void (*closedir)(struct fs_dir_stream *dirs);
 	int (*unlink)(const char *filename);
 	int (*mkdir)(const char *dirname);
+	int (*ln)(const char *filename, const char *target);
 };
 
 static struct fstype_info fstypes[] = {
@@ -181,6 +187,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fat_opendir,
 		.readdir = fat_readdir,
 		.closedir = fat_closedir,
+		.ln = fs_ln_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
@@ -196,8 +203,10 @@ static struct fstype_info fstypes[] = {
 		.read = ext4_read_file,
 #ifdef CONFIG_CMD_EXT4_WRITE
 		.write = ext4_write_file,
+		.ln = ext4fs_create_link,
 #else
 		.write = fs_write_unsupported,
+		.ln = fs_ln_unsupported,
 #endif
 		.uuid = ext4fs_uuid,
 		.opendir = fs_opendir_unsupported,
@@ -221,6 +230,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fs_opendir_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.ln = fs_ln_unsupported,
 	},
 #endif
 #ifdef CONFIG_CMD_UBIFS
@@ -239,6 +249,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fs_opendir_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.ln = fs_ln_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_BTRFS
@@ -257,6 +268,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fs_opendir_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.ln = fs_ln_unsupported,
 	},
 #endif
 	{
@@ -274,6 +286,7 @@ static struct fstype_info fstypes[] = {
 		.opendir = fs_opendir_unsupported,
 		.unlink = fs_unlink_unsupported,
 		.mkdir = fs_mkdir_unsupported,
+		.ln = fs_ln_unsupported,
 	},
 };
 
@@ -602,6 +615,22 @@ int fs_mkdir(const char *dirname)
 	return ret;
 }
 
+int fs_ln(const char *fname, const char *target)
+{
+	struct fstype_info *info = fs_get_info(fs_type);
+	int ret;
+
+	ret = info->ln(fname, target);
+
+	if (ret < 0) {
+		printf("** Unable to create link %s -> %s **\n", fname, target);
+		ret = -1;
+	}
+	fs_close();
+
+	return ret;
+}
+
 int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype)
 {
@@ -838,3 +867,18 @@ int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 
 	return 0;
 }
+
+int do_ln(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+	  int fstype)
+{
+	if (argc != 5)
+		return CMD_RET_USAGE;
+
+	if (fs_set_blk_dev(argv[1], argv[2], fstype))
+		return 1;
+
+	if (fs_ln(argv[3], argv[4]))
+		return 1;
+
+	return 0;
+}
diff --git a/include/fs.h b/include/fs.h
index aa3604db8d..6854597700 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -191,6 +191,8 @@ int do_rm(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);
+int do_ln(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.1

[U-Boot] [PATCH v2 7/7] test: fs: Added tests for symlinks

[Jean-Jacques Hiblot]

Test cases are:
1) basic link creation, verify it can be followed
2) chained links, verify it can be followed
3) replace exiting file a with a link, and a link with a link. verify it
   can be followed
4) create a broken link, verify it can't be followed

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

---

Changes in v2:
- Added python tests for symlinks under sandbox

 test/py/tests/test_fs/conftest.py     |  76 ++++++++++++++++
 test/py/tests/test_fs/fstest_defs.py  |   3 +
 test/py/tests/test_fs/test_symlink.py | 125 ++++++++++++++++++++++++++
 3 files changed, 204 insertions(+)
 create mode 100644 test/py/tests/test_fs/test_symlink.py

diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
index e742cda662..99bdc337f6 100644
--- a/test/py/tests/test_fs/conftest.py
+++ b/test/py/tests/test_fs/conftest.py
@@ -13,6 +13,7 @@ supported_fs_basic = ['fat16', 'fat32', 'ext4']
 supported_fs_ext = ['fat16', 'fat32']
 supported_fs_mkdir = ['fat16', 'fat32']
 supported_fs_unlink = ['fat16', 'fat32']
+supported_fs_symlink = ['ext4']
 
 #
 # Filesystem test specific setup
@@ -48,6 +49,7 @@ def pytest_configure(config):
     global supported_fs_ext
     global supported_fs_mkdir
     global supported_fs_unlink
+    global supported_fs_symlink
 
     def intersect(listA, listB):
         return  [x for x in listA if x in listB]
@@ -59,6 +61,7 @@ def pytest_configure(config):
         supported_fs_ext =  intersect(supported_fs, supported_fs_ext)
         supported_fs_mkdir =  intersect(supported_fs, supported_fs_mkdir)
         supported_fs_unlink =  intersect(supported_fs, supported_fs_unlink)
+        supported_fs_symlink =  intersect(supported_fs, supported_fs_symlink)
 
 def pytest_generate_tests(metafunc):
     """Parametrize fixtures, fs_obj_xxx
@@ -84,6 +87,9 @@ def pytest_generate_tests(metafunc):
     if 'fs_obj_unlink' in metafunc.fixturenames:
         metafunc.parametrize('fs_obj_unlink', supported_fs_unlink,
             indirect=True, scope='module')
+    if 'fs_obj_symlink' in metafunc.fixturenames:
+        metafunc.parametrize('fs_obj_symlink', supported_fs_symlink,
+            indirect=True, scope='module')
 
 #
 # Helper functions
@@ -538,3 +544,73 @@ def fs_obj_unlink(request, u_boot_config):
         call('rmdir %s' % mount_dir, shell=True)
         if fs_img:
             call('rm -f %s' % fs_img, shell=True)
+
+#
+# Fixture for symlink fs test
+#
+# NOTE: yield_fixture was deprecated since pytest-3.0
+ at pytest.yield_fixture()
+def fs_obj_symlink(request, u_boot_config):
+    """Set up a file system to be used in symlink fs test.
+
+    Args:
+        request: Pytest request object.
+        u_boot_config: U-boot configuration.
+
+    Return:
+        A fixture for basic fs test, i.e. a triplet of file system type,
+        volume file name and  a list of MD5 hashes.
+    """
+    fs_type = request.param
+    fs_img = ''
+
+    fs_ubtype = fstype_to_ubname(fs_type)
+    check_ubconfig(u_boot_config, fs_ubtype)
+
+    mount_dir = u_boot_config.persistent_data_dir + '/mnt'
+
+    small_file = mount_dir + '/' + SMALL_FILE
+    medium_file = mount_dir + '/' + MEDIUM_FILE
+
+    try:
+
+        # 3GiB volume
+        fs_img = mk_fs(u_boot_config, fs_type, 0x40000000, '1GB')
+
+        # Mount the image so we can populate it.
+        check_call('mkdir -p %s' % mount_dir, shell=True)
+        mount_fs(fs_type, fs_img, mount_dir)
+
+        # Create a subdirectory.
+        check_call('mkdir %s/SUBDIR' % mount_dir, shell=True)
+
+        # Create a small file in this image.
+        check_call('dd if=/dev/urandom of=%s bs=1M count=1'
+                   % small_file, shell=True)
+
+        # Create a medium file in this image.
+        check_call('dd if=/dev/urandom of=%s bs=10M count=1'
+                   % medium_file, shell=True)
+
+        # Generate the md5sums of reads that we will test against small file
+        out = check_output(
+            'dd if=%s bs=1M skip=0 count=1 2> /dev/null | md5sum'
+            % small_file, shell=True)
+        md5val = [out.split()[0]]
+        out = check_output(
+            'dd if=%s bs=10M skip=0 count=1 2> /dev/null | md5sum'
+            % medium_file, shell=True)
+        md5val.extend([out.split()[0]])
+
+        umount_fs(mount_dir)
+    except CalledProcessError:
+        pytest.skip('Setup failed for filesystem: ' + fs_type)
+        return
+    else:
+        yield [fs_ubtype, fs_img, md5val]
+    finally:
+        umount_fs(mount_dir)
+        fsck(fs_img, fs_type)
+        call('rmdir %s' % mount_dir, shell=True)
+        if fs_img:
+            call('rm -f %s' % fs_img, shell=True)
diff --git a/test/py/tests/test_fs/fstest_defs.py b/test/py/tests/test_fs/fstest_defs.py
index 5f107562d9..35b2bb6518 100644
--- a/test/py/tests/test_fs/fstest_defs.py
+++ b/test/py/tests/test_fs/fstest_defs.py
@@ -6,6 +6,9 @@ MIN_FILE='testfile'
 # $SMALL_FILE is the name of the 1MB file in the file system image
 SMALL_FILE='1MB.file'
 
+# $MEDIUM_FILE is the name of the 10MB file in the file system image
+MEDIUM_FILE='10MB.file'
+
 # $BIG_FILE is the name of the 2.5GB file in the file system image
 BIG_FILE='2.5GB.file'
 
diff --git a/test/py/tests/test_fs/test_symlink.py b/test/py/tests/test_fs/test_symlink.py
new file mode 100644
index 0000000000..bc4e679437
--- /dev/null
+++ b/test/py/tests/test_fs/test_symlink.py
@@ -0,0 +1,125 @@
+# SPDX-License-Identifier:      GPL-2.0+
+# Copyright (c) 2018, Texas Instrument
+# Author: Jean-Jacques Hiblot <jjhiblot@ti.com>
+#
+# U-Boot File System:symlink Test
+
+"""
+This test verifies unlink operation (deleting a file or a directory)
+on file system.
+"""
+
+import pytest
+import re
+from fstest_defs import *
+
+
+ at pytest.mark.boardspec('sandbox')
+ at pytest.mark.slow
+class TestSymlink(object):
+    def test_symlink1(self, u_boot_console, fs_obj_symlink):
+        """
+        Test Case 1 - create a link. and follow it when reading
+        """
+        fs_type, fs_img, md5val = fs_obj_symlink
+        with u_boot_console.log.section('Test Case 1 - create link and read'):
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'ln host 0:0 %s /%s.link ' % (SMALL_FILE, SMALL_FILE),
+            ])
+            assert('' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                '%sload host 0:0 %x /%s.link' % (fs_type, ADDR, SMALL_FILE),
+                'printenv filesize'])
+            assert('filesize=100000' in ''.join(output))
+
+            # Test Case 4b - Read full 1MB of small file
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val[0] in ''.join(output))
+
+    def test_symlink2(self, u_boot_console, fs_obj_symlink):
+        """
+        Test Case 2 - create chained links
+        """
+        fs_type, fs_img, md5val = fs_obj_symlink
+        with u_boot_console.log.section('Test Case 2 - create chained links'):
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'ln host 0:0 %s /%s.link1 ' % (SMALL_FILE, SMALL_FILE),
+                'ln host 0:0 /%s.link1 /SUBDIR/%s.link2' % (
+                    SMALL_FILE, SMALL_FILE),
+                'ln host 0:0 SUBDIR/%s.link2 /%s.link3' % (
+                    SMALL_FILE, SMALL_FILE),
+            ])
+            assert('' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                '%sload host 0:0 %x /%s.link3' % (fs_type, ADDR, SMALL_FILE),
+                'printenv filesize'])
+            assert('filesize=100000' in ''.join(output))
+
+            # Test Case 4b - Read full 1MB of small file
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val[0] in ''.join(output))
+
+    def test_symlink3(self, u_boot_console, fs_obj_symlink):
+        """
+        Test Case 3 - replace file/link with link
+        """
+        fs_type, fs_img, md5val = fs_obj_symlink
+        with u_boot_console.log.section('Test Case 1 - create link and read'):
+            output = u_boot_console.run_command_list([
+                'host bind 0 %s' % fs_img,
+                'setenv filesize',
+                'ln host 0:0 %s /%s ' % (MEDIUM_FILE, SMALL_FILE),
+                'ln host 0:0 %s /%s.link ' % (MEDIUM_FILE, MEDIUM_FILE),
+            ])
+            assert('' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                '%sload host 0:0 %x /%s' % (fs_type, ADDR, SMALL_FILE),
+                'printenv filesize'])
+            assert('filesize=a00000' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val[1] in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'ln host 0:0 %s.link /%s ' % (MEDIUM_FILE, SMALL_FILE),
+                '%sload host 0:0 %x /%s' % (fs_type, ADDR, SMALL_FILE),
+                'printenv filesize'])
+            assert('filesize=a00000' in ''.join(output))
+
+            output = u_boot_console.run_command_list([
+                'md5sum %x $filesize' % ADDR,
+                'setenv filesize'])
+            assert(md5val[1] in ''.join(output))
+
+    def test_symlink4(self, u_boot_console, fs_obj_symlink):
+        """
+        Test Case 4 - create a broken link
+        """
+        fs_type, fs_img, md5val = fs_obj_symlink
+        with u_boot_console.log.section('Test Case 1 - create link and read'):
+
+            output = u_boot_console.run_command_list([
+                'setenv filesize',
+                'ln host 0:0 nowhere /link ',
+            ])
+            assert('' in ''.join(output))
+
+            output = u_boot_console.run_command(
+                '%sload host 0:0 %x /link' %
+                (fs_type, ADDR))
+            with u_boot_console.disable_check('error_notification'):
+                output = u_boot_console.run_command('printenv filesize')
+            assert('"filesize" not defined' in ''.join(output))
-- 
2.17.1

[U-Boot] [PATCH v2 4/7] fs: ext4: constify the buffer passed to write functions

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:37PM +0100, Jean-Jacques Hiblot wrote:

> There is no need to modify the buffer passed to ext4fs_write_file().
> The memset() has no utility there, and comes probably from the equivalent
> ext4fs_read_file() function.

Can you please clarify the commit message a little?  I assume if we
re-worded the second sentence to this, it would still be correct:
The memset() call is not required here and was likely copied from the
equivalent part of the ext4fs_read_file() function where we do need it.

And if not, can you please explain why we don't need memset?  Thanks!

> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
> 
> Changes in v2: None
> 
>  fs/ext4/ext4_common.c |  2 +-
>  fs/ext4/ext4_common.h |  2 +-
>  fs/ext4/ext4_write.c  | 11 +++++------
>  include/ext4fs.h      |  2 +-
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 67e2471bd3..b881482c39 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -190,7 +190,7 @@ uint32_t ext4fs_div_roundup(uint32_t size, uint32_t n)
>  	return res;
>  }
>  
> -void put_ext4(uint64_t off, void *buf, uint32_t size)
> +void put_ext4(u64 off, const void *buf, u32 size)
>  {
>  	uint64_t startblock;
>  	uint64_t remainder;
> diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
> index 1ee81ab7ce..50155eccfc 100644
> --- a/fs/ext4/ext4_common.h
> +++ b/fs/ext4/ext4_common.h
> @@ -72,7 +72,7 @@ int ext4fs_iget(int inode_no, struct ext2_inode *inode);
>  void ext4fs_allocate_blocks(struct ext2_inode *file_inode,
>  				unsigned int total_remaining_blocks,
>  				unsigned int *total_no_of_block);
> -void put_ext4(uint64_t off, void *buf, uint32_t size);
> +void put_ext4(u64 off, const void *buf, u32 size);
>  struct ext2_block_group *ext4fs_get_group_descriptor
>  	(const struct ext_filesystem *fs, uint32_t bg_idx);
>  uint64_t ext4fs_bg_get_block_id(const struct ext2_block_group *bg,

Since the rest of the function is using uintXX_t not uXX can you please
just change to const?

-- 
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/20190201/1efef7b4/attachment.sig>

[U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote:

> u-boot does not supports updating the metadata chacksums
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/7496ccb1/attachment.sig>

[U-Boot] [PATCH v2 2/7] test: fs: disable the metadata checksums on ext4 filesystems

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:35PM +0100, Jean-Jacques Hiblot wrote:

> If the metadata checksums are enabled, all write operations will fail.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/d2d225c3/attachment.sig>

[U-Boot] [PATCH v2 3/7] test: fs: run fsck after on fs image after the tests are run

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:36PM +0100, Jean-Jacques Hiblot wrote:

> This is to check the integrity of the FS after the test operations.
> This is useful to make sure that the operations are implemented properly,
> and are not going to create silent corruptions.
> Currently only the integrity of EXT4 filesystems is checked.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 
> ---
> 
> Changes in v2:
> - Add a FS integrity check at the end of the FS tests
> 
>  test/py/tests/test_fs/conftest.py | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
> index 745ed0ed38..e742cda662 100644
> --- a/test/py/tests/test_fs/conftest.py
> +++ b/test/py/tests/test_fs/conftest.py
> @@ -216,6 +216,15 @@ def mount_fs(fs_type, device, mount_point):
>      except CalledProcessError:
>          raise
>  
> +
> +def fsck(img, fs_type):
> +    try:
> +        if fs_type == 'ext4':
> +            check_call('fsck.ext4 -n -f %s' % img, shell=True)
> +    except CalledProcessError:
> +        raise
> +
> +

Why don't we just call 'fsck -f -n ...' and check all filesystems?
Force and "make no changes" are both general fsck options and not
filesystem specific.  And if we're messing up FAT as part of the tests
we should know that now :)

-- 
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/20190201/f02b6f9d/attachment.sig>

[U-Boot] [PATCH v2 5/7] fs: ext4: Add support for the creation of symbolic links

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:38PM +0100, Jean-Jacques Hiblot wrote:

> Re-use the functions used to write/create a file, to support creation of a
> symbolic link.
> The difference with a regular file are small:
> - The inode mode is flagged with S_IFLNK instead of S_IFREG
> - The ext2_dirent's filetype is FILETYPE_SYMLINK instead of FILETYPE_REG
> - Instead of storing the content of a file in allocated blocks, the path
> to the target is stored. And if the target's path is short enough, no block
> is allocated and the target's path is stored in ext2_inode.b.symlink
> 
> As with regulars files, if a file/symlink with the same name exits, it is
> unlinked first and then re-created.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/4859ce47/attachment.sig>

[U-Boot] [PATCH v2 6/7] fs: Add a new command to create symbolic links

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:39PM +0100, Jean-Jacques Hiblot wrote:

> The command line is:
> ln <interface> <dev[:part]> target linkname
> 
> Currently symbolic links are supported only in ext4 and only if the option
> CMD_EXT4_WRITE is enabled.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/e3ad0de1/attachment.sig>

[U-Boot] [PATCH v2 7/7] test: fs: Added tests for symlinks

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:40PM +0100, Jean-Jacques Hiblot wrote:

> Test cases are:
> 1) basic link creation, verify it can be followed
> 2) chained links, verify it can be followed
> 3) replace exiting file a with a link, and a link with a link. verify it
>    can be followed
> 4) create a broken link, verify it can't be followed
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/46734e92/attachment.sig>

[U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active

[Tom Rini]

On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote:

> u-boot does not supports updating the metadata chacksums
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> 
> ---
> 
> Changes in v2:
> - Prevent write access if metadata checksum is enabled
> 
>  fs/ext4/ext4_write.c | 12 ++++++++++--
>  include/ext4fs.h     |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index a7f543f7df..1de29236f0 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
>  
>  	g_parent_inode = zalloc(fs->inodesz);
>  	if (!g_parent_inode)
> -		goto fail;
> +		goto fail_ext4fs_init;
>  
>  	if (ext4fs_init() != 0) {
>  		printf("error in File System init\n");
> -		return -1;
> +		goto fail_ext4fs_init;
> +	}
> +
> +	if (le32_to_cpu(fs->sb->feature_ro_compat) &
> +		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
> +		printf("u-boot doesn't support updating the metadata checksums yet\n");
> +		goto fail;
>  	}
> +
>  	inodes_per_block = fs->blksz / fs->inodesz;
>  	parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
>  	if (parent_inodeno == -1)

I'm following up to myself on this one as, Ugh.  To repeat myself from a
while back:
commit 6f94ab6656ceffb3f2a972c8de4c554502b6f2b7
Author: Tom Rini <trini@konsulko.com>
Date:   Fri Jul 22 17:59:11 2016 -0400

    ext4: Refuse to mount filesystems with 64bit feature set
    
    With e2fsprogs after 1.43 the 64bit and metadata_csum features are
    enabled by default.  The metadata_csum feature changes how
    ext4_group_desc->bg_checksum is calculated, which would break write
    support.  The 64bit feature however introduces changes such that it
    cannot be read by implementations that do not support it.  Since we do
    not support this, we must not mount it.
    
    Cc: Stephen Warren <swarren@nvidia.com>
    Cc: Simon Glass <sjg@chromium.org>
    Cc: Lukasz Majewski <l.majewski@samsung.com>
    Cc: Stefan Roese <sr@denx.de>
    Reported-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
    Signed-off-by: Tom Rini <trini@konsulko.com>

Which means that starting way back then I should have also done
something to say we cannot write to these new images either.  It's good
and important to finally catch this failure now.  I suspect it's however
going to start to be an unexpected problem.  Have you any idea how much
work would go in to supporting the metadata_csum feature?  Thanks again!

-- 
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/20190201/a86c6b0a/attachment.sig>

[U-Boot] [PATCH v2 3/7] test: fs: run fsck after on fs image after the tests are run

[Jean-Jacques Hiblot]

On 01/02/2019 16:18, Tom Rini wrote:
> On Fri, Feb 01, 2019 at 03:33:36PM +0100, Jean-Jacques Hiblot wrote:
>
>> This is to check the integrity of the FS after the test operations.
>> This is useful to make sure that the operations are implemented properly,
>> and are not going to create silent corruptions.
>> Currently only the integrity of EXT4 filesystems is checked.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>
>> ---
>>
>> Changes in v2:
>> - Add a FS integrity check at the end of the FS tests
>>
>>   test/py/tests/test_fs/conftest.py | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
>> index 745ed0ed38..e742cda662 100644
>> --- a/test/py/tests/test_fs/conftest.py
>> +++ b/test/py/tests/test_fs/conftest.py
>> @@ -216,6 +216,15 @@ def mount_fs(fs_type, device, mount_point):
>>       except CalledProcessError:
>>           raise
>>   
>> +
>> +def fsck(img, fs_type):
>> +    try:
>> +        if fs_type == 'ext4':
>> +            check_call('fsck.ext4 -n -f %s' % img, shell=True)
>> +    except CalledProcessError:
>> +        raise
>> +
>> +
> Why don't we just call 'fsck -f -n ...' and check all filesystems?

> Force and "make no changes" are both general fsck options and not

Strange. those options do not appear in the help or manpage.

I'll switch to fsck in the next version

> filesystem specific.  And if we're messing up FAT as part of the tests
> we should know that now :)

>

[U-Boot] [PATCH v2 3/7] test: fs: run fsck after on fs image after the tests are run

[Jean-Jacques Hiblot]

On 01/02/2019 16:18, Tom Rini wrote:
> On Fri, Feb 01, 2019 at 03:33:36PM +0100, Jean-Jacques Hiblot wrote:
>
>> This is to check the integrity of the FS after the test operations.
>> This is useful to make sure that the operations are implemented properly,
>> and are not going to create silent corruptions.
>> Currently only the integrity of EXT4 filesystems is checked.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>
>> ---
>>
>> Changes in v2:
>> - Add a FS integrity check at the end of the FS tests
>>
>>   test/py/tests/test_fs/conftest.py | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
>> index 745ed0ed38..e742cda662 100644
>> --- a/test/py/tests/test_fs/conftest.py
>> +++ b/test/py/tests/test_fs/conftest.py
>> @@ -216,6 +216,15 @@ def mount_fs(fs_type, device, mount_point):
>>       except CalledProcessError:
>>           raise
>>   
>> +
>> +def fsck(img, fs_type):
>> +    try:
>> +        if fs_type == 'ext4':
>> +            check_call('fsck.ext4 -n -f %s' % img, shell=True)
>> +    except CalledProcessError:
>> +        raise
>> +
>> +
> Why don't we just call 'fsck -f -n ...' and check all filesystems?
> Force and "make no changes" are both general fsck options and not
> filesystem specific.  And if we're messing up FAT as part of the tests
> we should know that now :)

When I tried to enable fsck on all FS, I discovered that calling fsck() 
at this point won't work.

This is because of the "life-cycle" of pytest fixtures works. How it is 
done today makes failure report unreliable (the error is reported for 
the test after the failed one).

I'll try to come up with something but it'll take some time to get used 
to pytest. Meanwhile I'll drop this from the  series.

JJ



>

[U-Boot] [PATCH v2 3/7] test: fs: run fsck after on fs image after the tests are run

[Tom Rini]

On Fri, Feb 01, 2019 at 06:00:15PM +0100, Jean-Jacques Hiblot wrote:
> 
> On 01/02/2019 16:18, Tom Rini wrote:
> >On Fri, Feb 01, 2019 at 03:33:36PM +0100, Jean-Jacques Hiblot wrote:
> >
> >>This is to check the integrity of the FS after the test operations.
> >>This is useful to make sure that the operations are implemented properly,
> >>and are not going to create silent corruptions.
> >>Currently only the integrity of EXT4 filesystems is checked.
> >>
> >>Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >>
> >>---
> >>
> >>Changes in v2:
> >>- Add a FS integrity check at the end of the FS tests
> >>
> >>  test/py/tests/test_fs/conftest.py | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >>diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
> >>index 745ed0ed38..e742cda662 100644
> >>--- a/test/py/tests/test_fs/conftest.py
> >>+++ b/test/py/tests/test_fs/conftest.py
> >>@@ -216,6 +216,15 @@ def mount_fs(fs_type, device, mount_point):
> >>      except CalledProcessError:
> >>          raise
> >>+
> >>+def fsck(img, fs_type):
> >>+    try:
> >>+        if fs_type == 'ext4':
> >>+            check_call('fsck.ext4 -n -f %s' % img, shell=True)
> >>+    except CalledProcessError:
> >>+        raise
> >>+
> >>+
> >Why don't we just call 'fsck -f -n ...' and check all filesystems?
> 
> >Force and "make no changes" are both general fsck options and not
> 
> Strange. those options do not appear in the help or manpage.
> 
> I'll switch to fsck in the next version

Ugh, I take it back and blame myself.  You can't just do /sbin/fsck
--help and get something like I expected but didn't read closely enough
to see it was plumbing down to fsck.ext4.  So what you have above is
right and the it falls on me to add a patch to fsck.vfat or so later.

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190201/20675ca6/attachment.sig>

[U-Boot] [PATCH v2 1/7] fs: ext4: do not allow writes if metadata checksum is active

[Jean-Jacques Hiblot]

On 01/02/2019 16:23, Tom Rini wrote:
> On Fri, Feb 01, 2019 at 03:33:34PM +0100, Jean-Jacques Hiblot wrote:
>
>> u-boot does not supports updating the metadata chacksums
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>
>> ---
>>
>> Changes in v2:
>> - Prevent write access if metadata checksum is enabled
>>
>>   fs/ext4/ext4_write.c | 12 ++++++++++--
>>   include/ext4fs.h     |  1 +
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
>> index a7f543f7df..1de29236f0 100644
>> --- a/fs/ext4/ext4_write.c
>> +++ b/fs/ext4/ext4_write.c
>> @@ -858,12 +858,19 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
>>   
>>   	g_parent_inode = zalloc(fs->inodesz);
>>   	if (!g_parent_inode)
>> -		goto fail;
>> +		goto fail_ext4fs_init;
>>   
>>   	if (ext4fs_init() != 0) {
>>   		printf("error in File System init\n");
>> -		return -1;
>> +		goto fail_ext4fs_init;
>> +	}
>> +
>> +	if (le32_to_cpu(fs->sb->feature_ro_compat) &
>> +		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) {
>> +		printf("u-boot doesn't support updating the metadata checksums yet\n");
>> +		goto fail;
>>   	}
>> +
>>   	inodes_per_block = fs->blksz / fs->inodesz;
>>   	parent_inodeno = ext4fs_get_parent_inode_num(fname, filename, F_FILE);
>>   	if (parent_inodeno == -1)
> I'm following up to myself on this one as, Ugh.  To repeat myself from a
> while back:
> commit 6f94ab6656ceffb3f2a972c8de4c554502b6f2b7
> Author: Tom Rini <trini@konsulko.com>
> Date:   Fri Jul 22 17:59:11 2016 -0400
>
>      ext4: Refuse to mount filesystems with 64bit feature set
>      
>      With e2fsprogs after 1.43 the 64bit and metadata_csum features are
>      enabled by default.  The metadata_csum feature changes how
>      ext4_group_desc->bg_checksum is calculated, which would break write
>      support.  The 64bit feature however introduces changes such that it
>      cannot be read by implementations that do not support it.  Since we do
>      not support this, we must not mount it.
>      
>      Cc: Stephen Warren <swarren@nvidia.com>
>      Cc: Simon Glass <sjg@chromium.org>
>      Cc: Lukasz Majewski <l.majewski@samsung.com>
>      Cc: Stefan Roese <sr@denx.de>
>      Reported-by: Andrew Bradford <andrew.bradford@kodakalaris.com>
>      Signed-off-by: Tom Rini <trini@konsulko.com>
>
> Which means that starting way back then I should have also done
> something to say we cannot write to these new images either.  It's good
> and important to finally catch this failure now.  I suspect it's however
> going to start to be an unexpected problem.  Have you any idea how much
> work would go in to supporting the metadata_csum feature?  Thanks again!

I had a look and this is going to be a non-trivial job. The ext4 code is 
showing its age.  And then there would be another couple of flags to 
handle too like journal checksuming, and then also hashtree dirs.

There are a couple of ext4 libs out there (I'm thinking of lwext4 or 
e2fsprogs) that do a much better job at handling the 
compatible/incompatible/read-only flags. I wonder if we wouldn't be 
better off adapting one of them than fixing the current implementation.

JJ

>