[U-Boot] [PATCH 1/3] ext4: cleanup unlink_filename function

[U-Boot] [PATCH 1/3] ext4: cleanup unlink_filename function

[Stefan Brüns]

Use the same variable names as in search_dir, to make purpose of variables
more obvious.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index e78185b..699a640 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -854,34 +854,35 @@ fail:
 
 static int unlink_filename(char *filename, unsigned int blknr)
 {
-	int totalbytes = 0;
 	int templength = 0;
 	int status, inodeno;
 	int found = 0;
-	char *root_first_block_buffer = NULL;
+	int offset;
+	char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
 	struct ext2_dirent *previous_dir = NULL;
 	char *ptr = NULL;
 	struct ext_filesystem *fs = get_fs();
 	int ret = -1;
 
-	/* get the first block of root */
-	root_first_block_buffer = zalloc(fs->blksz);
-	if (!root_first_block_buffer)
+	block_buffer = zalloc(fs->blksz);
+	if (!block_buffer)
 		return -ENOMEM;
+
+	/* read the directory block */
 	status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0,
-				fs->blksz, root_first_block_buffer);
+				fs->blksz, block_buffer);
 	if (status == 0)
 		goto fail;
 
-	if (ext4fs_log_journal(root_first_block_buffer, blknr))
+	if (ext4fs_log_journal(block_buffer, blknr))
 		goto fail;
-	dir = (struct ext2_dirent *)root_first_block_buffer;
+	dir = (struct ext2_dirent *)block_buffer;
 	ptr = (char *)dir;
-	totalbytes = 0;
+	offset = 0;
 	while (le16_to_cpu(dir->direntlen) >= 0) {
 		/*
-		 * blocksize-totalbytes because last
+		 * blocksize-offset because last
 		 * directory length i.e., *dir->direntlen
 		 * is free availble space in the block that
 		 * means it is a last entry of directory entry
@@ -903,12 +904,12 @@ static int unlink_filename(char *filename, unsigned int blknr)
 			break;
 		}
 
-		if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
+		if (fs->blksz - offset == le16_to_cpu(dir->direntlen))
 			break;
 
 		/* traversing the each directory entry */
 		templength = le16_to_cpu(dir->direntlen);
-		totalbytes = totalbytes + templength;
+		offset = offset + templength;
 		previous_dir = dir;
 		dir = (struct ext2_dirent *)((char *)dir + templength);
 		ptr = (char *)dir;
@@ -916,12 +917,12 @@ static int unlink_filename(char *filename, unsigned int blknr)
 
 
 	if (found == 1) {
-		if (ext4fs_put_metadata(root_first_block_buffer, blknr))
+		if (ext4fs_put_metadata(block_buffer, blknr))
 			goto fail;
 		ret = inodeno;
 	}
 fail:
-	free(root_first_block_buffer);
+	free(block_buffer);
 
 	return ret;
 }
-- 
2.10.0

[U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Stefan Brüns]

The direntlen checks were quite bogus, i.e. the loop termination used
"len + offset == blocksize" (exact match only), and checked for a
direntlen less than 0. The latter can never happen as the len is
unsigned, this has been reported by Coverity, CID 153384.

Use the same code as in search_dir for directory traversal. This code
has the correct checks for direntlen >= sizeof(struct dirent), and
offset < blocksize.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 699a640..11be0b7 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -854,16 +854,15 @@ fail:
 
 static int unlink_filename(char *filename, unsigned int blknr)
 {
-	int templength = 0;
-	int status, inodeno;
-	int found = 0;
+	int status;
+	int inodeno = 0;
 	int offset;
 	char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
-	struct ext2_dirent *previous_dir = NULL;
-	char *ptr = NULL;
+	struct ext2_dirent *previous_dir;
 	struct ext_filesystem *fs = get_fs();
 	int ret = -1;
+	char *direntname;
 
 	block_buffer = zalloc(fs->blksz);
 	if (!block_buffer)
@@ -877,20 +876,18 @@ static int unlink_filename(char *filename, unsigned int blknr)
 
 	if (ext4fs_log_journal(block_buffer, blknr))
 		goto fail;
-	dir = (struct ext2_dirent *)block_buffer;
-	ptr = (char *)dir;
 	offset = 0;
-	while (le16_to_cpu(dir->direntlen) >= 0) {
-		/*
-		 * blocksize-offset because last
-		 * directory length i.e., *dir->direntlen
-		 * is free availble space in the block that
-		 * means it is a last entry of directory entry
-		 */
+	do {
+		previous_dir = dir;
+		dir = (struct ext2_dirent *)(block_buffer + offset);
+		direntname = (char *)(dir) + sizeof(struct ext2_dirent);
+
+		int direntlen = le16_to_cpu(dir->direntlen);
+		if (direntlen < sizeof(struct ext2_dirent))
+			break;
+
 		if (dir->inode && (strlen(filename) == dir->namelen) &&
-		    (strncmp(ptr + sizeof(struct ext2_dirent),
-			     filename, dir->namelen) == 0)) {
-			printf("file found, deleting\n");
+		    (strncmp(direntname, filename, dir->namelen) == 0)) {
 			inodeno = le32_to_cpu(dir->inode);
 			if (previous_dir) {
 				uint16_t new_len;
@@ -900,23 +897,15 @@ static int unlink_filename(char *filename, unsigned int blknr)
 			} else {
 				dir->inode = 0;
 			}
-			found = 1;
 			break;
 		}
 
-		if (fs->blksz - offset == le16_to_cpu(dir->direntlen))
-			break;
+		offset += direntlen;
 
-		/* traversing the each directory entry */
-		templength = le16_to_cpu(dir->direntlen);
-		offset = offset + templength;
-		previous_dir = dir;
-		dir = (struct ext2_dirent *)((char *)dir + templength);
-		ptr = (char *)dir;
-	}
+	} while (offset < fs->blksz);
 
+	if (inodeno > 0) {
 
-	if (found == 1) {
 		if (ext4fs_put_metadata(block_buffer, blknr))
 			goto fail;
 		ret = inodeno;
-- 
2.10.0

[U-Boot] [PATCH 3/3] ext4: Only write journal entries for modified blocks in unlink_filename

[Stefan Brüns]

Instead of creating a journal entry for each directory block, even
if the block is unmodified, only log the modified block.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 11be0b7..ec02eec 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -874,8 +874,6 @@ static int unlink_filename(char *filename, unsigned int blknr)
 	if (status == 0)
 		goto fail;
 
-	if (ext4fs_log_journal(block_buffer, blknr))
-		goto fail;
 	offset = 0;
 	do {
 		previous_dir = dir;
@@ -889,14 +887,6 @@ static int unlink_filename(char *filename, unsigned int blknr)
 		if (dir->inode && (strlen(filename) == dir->namelen) &&
 		    (strncmp(direntname, filename, dir->namelen) == 0)) {
 			inodeno = le32_to_cpu(dir->inode);
-			if (previous_dir) {
-				uint16_t new_len;
-				new_len = le16_to_cpu(previous_dir->direntlen);
-				new_len += le16_to_cpu(dir->direntlen);
-				previous_dir->direntlen = cpu_to_le16(new_len);
-			} else {
-				dir->inode = 0;
-			}
 			break;
 		}
 
@@ -905,7 +895,20 @@ static int unlink_filename(char *filename, unsigned int blknr)
 	} while (offset < fs->blksz);
 
 	if (inodeno > 0) {
+		printf("file found, deleting\n");
+		if (ext4fs_log_journal(block_buffer, blknr))
+			goto fail;
 
+		if (previous_dir) {
+			/* merge dir entry with predecessor */
+			uint16_t new_len;
+			new_len = le16_to_cpu(previous_dir->direntlen);
+			new_len += le16_to_cpu(dir->direntlen);
+			previous_dir->direntlen = cpu_to_le16(new_len);
+		} else {
+			/* invalidate dir entry */
+			dir->inode = 0;
+		}
 		if (ext4fs_put_metadata(block_buffer, blknr))
 			goto fail;
 		ret = inodeno;
-- 
2.10.0

[U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:27PM +0200, Stefan Br?ns wrote:

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Reported-by: Coverity (CID: 153383, 153384)
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: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/b1767802/attachment.sig>

[U-Boot] [PATCH 1/3] ext4: cleanup unlink_filename function

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:26PM +0200, Stefan Br?ns wrote:

> Use the same variable names as in search_dir, to make purpose of variables
> more obvious.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

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: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/94371e1c/attachment.sig>

[U-Boot] [PATCH 3/3] ext4: Only write journal entries for modified blocks in unlink_filename

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:28PM +0200, Stefan Br?ns wrote:

> Instead of creating a journal entry for each directory block, even
> if the block is unmodified, only log the modified block.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

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: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161013/1a5912fb/attachment.sig>

[U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Lukasz Majewski]

Hi Stefan,

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 45
> +++++++++++++++++---------------------------- 1 file changed, 17
> insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 699a640..11be0b7 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -854,16 +854,15 @@ fail:
>  
>  static int unlink_filename(char *filename, unsigned int blknr)
>  {
> -	int templength = 0;
> -	int status, inodeno;
> -	int found = 0;
> +	int status;
> +	int inodeno = 0;
>  	int offset;
>  	char *block_buffer = NULL;
>  	struct ext2_dirent *dir = NULL;
> -	struct ext2_dirent *previous_dir = NULL;
> -	char *ptr = NULL;
> +	struct ext2_dirent *previous_dir;
>  	struct ext_filesystem *fs = get_fs();
>  	int ret = -1;
> +	char *direntname;
>  
>  	block_buffer = zalloc(fs->blksz);
>  	if (!block_buffer)
> @@ -877,20 +876,18 @@ static int unlink_filename(char *filename,
> unsigned int blknr) 
>  	if (ext4fs_log_journal(block_buffer, blknr))
>  		goto fail;
> -	dir = (struct ext2_dirent *)block_buffer;
> -	ptr = (char *)dir;
>  	offset = 0;
> -	while (le16_to_cpu(dir->direntlen) >= 0) {
> -		/*
> -		 * blocksize-offset because last
> -		 * directory length i.e., *dir->direntlen
> -		 * is free availble space in the block that
> -		 * means it is a last entry of directory entry
> -		 */
> +	do {
> +		previous_dir = dir;
> +		dir = (struct ext2_dirent *)(block_buffer + offset);
> +		direntname = (char *)(dir) + sizeof(struct
> ext2_dirent); +
> +		int direntlen = le16_to_cpu(dir->direntlen);
> +		if (direntlen < sizeof(struct ext2_dirent))
> +			break;
> +
>  		if (dir->inode && (strlen(filename) == dir->namelen)
> &&
> -		    (strncmp(ptr + sizeof(struct ext2_dirent),
> -			     filename, dir->namelen) == 0)) {
> -			printf("file found, deleting\n");
> +		    (strncmp(direntname, filename, dir->namelen) ==
> 0)) { inodeno = le32_to_cpu(dir->inode);
>  			if (previous_dir) {
>  				uint16_t new_len;
> @@ -900,23 +897,15 @@ static int unlink_filename(char *filename,
> unsigned int blknr) } else {
>  				dir->inode = 0;
>  			}
> -			found = 1;
>  			break;
>  		}
>  
> -		if (fs->blksz - offset ==
> le16_to_cpu(dir->direntlen))
> -			break;
> +		offset += direntlen;
>  
> -		/* traversing the each directory entry */
> -		templength = le16_to_cpu(dir->direntlen);
> -		offset = offset + templength;
> -		previous_dir = dir;
> -		dir = (struct ext2_dirent *)((char *)dir +
> templength);
> -		ptr = (char *)dir;
> -	}
> +	} while (offset < fs->blksz);
>  
> +	if (inodeno > 0) {
>  
> -	if (found == 1) {
>  		if (ext4fs_put_metadata(block_buffer, blknr))
>  			goto fail;
>  		ret = inodeno;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 3/3] ext4: Only write journal entries for modified blocks in unlink_filename

[Lukasz Majewski]

Hi Stefan,

> Instead of creating a journal entry for each directory block, even
> if the block is unmodified, only log the modified block.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 11be0b7..ec02eec 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -874,8 +874,6 @@ static int unlink_filename(char *filename,
> unsigned int blknr) if (status == 0)
>  		goto fail;
>  
> -	if (ext4fs_log_journal(block_buffer, blknr))
> -		goto fail;
>  	offset = 0;
>  	do {
>  		previous_dir = dir;
> @@ -889,14 +887,6 @@ static int unlink_filename(char *filename,
> unsigned int blknr) if (dir->inode && (strlen(filename) ==
> dir->namelen) && (strncmp(direntname, filename, dir->namelen) == 0)) {
>  			inodeno = le32_to_cpu(dir->inode);
> -			if (previous_dir) {
> -				uint16_t new_len;
> -				new_len =
> le16_to_cpu(previous_dir->direntlen);
> -				new_len +=
> le16_to_cpu(dir->direntlen);
> -				previous_dir->direntlen =
> cpu_to_le16(new_len);
> -			} else {
> -				dir->inode = 0;
> -			}
>  			break;
>  		}
>  
> @@ -905,7 +895,20 @@ static int unlink_filename(char *filename,
> unsigned int blknr) } while (offset < fs->blksz);
>  
>  	if (inodeno > 0) {
> +		printf("file found, deleting\n");
> +		if (ext4fs_log_journal(block_buffer, blknr))
> +			goto fail;
>  
> +		if (previous_dir) {
> +			/* merge dir entry with predecessor */
> +			uint16_t new_len;
> +			new_len =
> le16_to_cpu(previous_dir->direntlen);
> +			new_len += le16_to_cpu(dir->direntlen);
> +			previous_dir->direntlen =
> cpu_to_le16(new_len);
> +		} else {
> +			/* invalidate dir entry */
> +			dir->inode = 0;
> +		}
>  		if (ext4fs_put_metadata(block_buffer, blknr))
>  			goto fail;
>  		ret = inodeno;

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [U-Boot,1/3] ext4: cleanup unlink_filename function

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:26PM +0200, Stefan Br?ns wrote:

> Use the same variable names as in search_dir, to make purpose of variables
> more obvious.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 2/3] ext4: Fix handling of direntlen in unlink_filename

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:27PM +0200, Stefan Br?ns wrote:

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> Reported-by: Coverity (CID: 153383, 153384)
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 3/3] ext4: Only write journal entries for modified blocks in unlink_filename

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:28PM +0200, Stefan Br?ns wrote:

> Instead of creating a journal entry for each directory block, even
> if the block is unmodified, only log the modified block.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

Applied to u-boot/master, thanks!

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