[U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node()

[U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node()

[Tom Rini]

The function ext4fs_free_node() exists for dealing with "dirnode"
structures that we allocate.  However, we do not allocate these
dynamically as needed but rather as a single instance in ext4fs_mount()
that we zalloc().  Coverity scan notes that in two places we're doing
what it calls a "Free of address-of expression" as we were free()'ing
oldnode.  However, oldnode was never directly allocated, nor any other
instance which we were calling ext4fs_free_node() on.  Removing this
structure allows us to also restructure ext4fs_close() slightlu too.

Tested on OMAP4 Pandaboard with Fedora 23 (/boot is ext4) as well as
reading and writing files from / to /boot and vice-versa and confirming
they read back again correctly.

Cc: Stephen Warren <swarren@nvidia.com>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Cc: Simon Glass <sjg@chromium.org>
Cc: Przemyslaw Marczak <p.marczak@samsung.com>
Reported-by: Coverity (CID 131278, 131091)
Signed-off-by: Tom Rini <trini@konsulko.com>

---
I would really appreciate a few more sets of eyes on these changes.  I
was surprised with the conclusion I came to, but after reading the code
paths a few times, I kept coming back to this same conclusion.
---
 fs/ext4/ext4_common.c |   38 +++++++++-----------------------------
 fs/ext4/ext4fs.c      |    7 -------
 include/ext4fs.h      |    1 -
 3 files changed, 9 insertions(+), 37 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index e73223a..14096e0 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -1879,15 +1879,13 @@ void ext4fs_reinit_global(void)
 		ext4fs_indir3_blkno = -1;
 	}
 }
+
 void ext4fs_close(void)
 {
-	if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
-		ext4fs_free_node(ext4fs_file, &ext4fs_root->diropen);
-		ext4fs_file = NULL;
-	}
 	if (ext4fs_root != NULL) {
 		free(ext4fs_root);
 		ext4fs_root = NULL;
+		ext4fs_file = NULL;
 	}
 
 	ext4fs_reinit_global();
@@ -2094,10 +2092,8 @@ static int ext4fs_find_file1(const char *currpath,
 				*(next++) = '\0';
 		}
 
-		if (type != FILETYPE_DIRECTORY) {
-			ext4fs_free_node(currnode, currroot);
+		if (type != FILETYPE_DIRECTORY)
 			return 0;
-		}
 
 		oldnode = currnode;
 
@@ -2114,26 +2110,18 @@ static int ext4fs_find_file1(const char *currpath,
 			char *symlink;
 
 			/* Test if the symlink does not loop. */
-			if (++symlinknest == 8) {
-				ext4fs_free_node(currnode, currroot);
-				ext4fs_free_node(oldnode, currroot);
+			if (++symlinknest == 8)
 				return 0;
-			}
 
 			symlink = ext4fs_read_symlink(currnode);
-			ext4fs_free_node(currnode, currroot);
 
-			if (!symlink) {
-				ext4fs_free_node(oldnode, currroot);
+			if (!symlink)
 				return 0;
-			}
 
 			debug("Got symlink >%s<\n", symlink);
 
-			if (symlink[0] == '/') {
-				ext4fs_free_node(oldnode, currroot);
+			if (symlink[0] == '/')
 				oldnode = &ext4fs_root->diropen;
-			}
 
 			/* Lookup the node the symlink points to. */
 			status = ext4fs_find_file1(symlink, oldnode,
@@ -2141,14 +2129,10 @@ static int ext4fs_find_file1(const char *currpath,
 
 			free(symlink);
 
-			if (status == 0) {
-				ext4fs_free_node(oldnode, currroot);
+			if (status == 0)
 				return 0;
-			}
 		}
 
-		ext4fs_free_node(oldnode, currroot);
-
 		/* Found the node! */
 		if (!next || *next == '\0') {
 			*currfound = currnode;
@@ -2196,22 +2180,18 @@ int ext4fs_open(const char *filename, loff_t *len)
 	status = ext4fs_find_file(filename, &ext4fs_root->diropen, &fdiro,
 				  FILETYPE_REG);
 	if (status == 0)
-		goto fail;
+		return -1;
 
 	if (!fdiro->inode_read) {
 		status = ext4fs_read_inode(fdiro->data, fdiro->ino,
 				&fdiro->inode);
 		if (status == 0)
-			goto fail;
+			return -1;
 	}
 	*len = __le32_to_cpu(fdiro->inode.size);
 	ext4fs_file = fdiro;
 
 	return 0;
-fail:
-	ext4fs_free_node(fdiro, &ext4fs_root->diropen);
-
-	return -1;
 }
 
 int ext4fs_mount(unsigned part_length)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
index 258b9379..36f8023 100644
--- a/fs/ext4/ext4fs.c
+++ b/fs/ext4/ext4fs.c
@@ -35,12 +35,6 @@ struct ext_filesystem *get_fs(void)
 	return &ext_fs;
 }
 
-void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot)
-{
-	if ((node != &ext4fs_root->diropen) && (node != currroot))
-		free(node);
-}
-
 /*
  * Taken from openmoko-kernel mailing list: By Andy green
  * Optimized read file API : collects and defers contiguous sector
@@ -171,7 +165,6 @@ int ext4fs_ls(const char *dirname)
 	}
 
 	ext4fs_iterate_dir(dirnode, NULL, NULL, NULL);
-	ext4fs_free_node(dirnode, &ext4fs_root->diropen);
 
 	return 0;
 }
diff --git a/include/ext4fs.h b/include/ext4fs.h
index 6888adc..9cb7882 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -139,7 +139,6 @@ void ext4fs_reinit_global(void);
 int ext4fs_ls(const char *dirname);
 int ext4fs_exists(const char *filename);
 int ext4fs_size(const char *filename, loff_t *size);
-void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
 int ext4fs_devread(lbaint_t sector, int byte_offset, int byte_len, char *buf);
 void ext4fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info);
 long int read_allocated_block(struct ext2_inode *inode, int fileblock);
-- 
1.7.9.5

[U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node()

[Stephen Warren]

On 12/09/2015 05:54 PM, Tom Rini wrote:
> The function ext4fs_free_node() exists for dealing with "dirnode"
> structures that we allocate.  However, we do not allocate these
> dynamically as needed but rather as a single instance in ext4fs_mount()
> that we zalloc().  Coverity scan notes that in two places we're doing
> what it calls a "Free of address-of expression" as we were free()'ing
> oldnode.  However, oldnode was never directly allocated, nor any other
> instance which we were calling ext4fs_free_node() on.  Removing this
> structure allows us to also restructure ext4fs_close() slightlu too.
>
> Tested on OMAP4 Pandaboard with Fedora 23 (/boot is ext4) as well as
> reading and writing files from / to /boot and vice-versa and confirming
> they read back again correctly.

I think if this change was valid, then we could delete the global 
variable ext4fs_file, and replace all references to it with a reference 
to the dirnode member in struct ext2_data?

Anyway, I believe that the-value-pointed-at-by-ext4fs_file is 
dynamically allocated outside struct ext2_data sometimes:

(All line number references relative to 5076c64a08d2 "Merge branch 
'master' of git://git.denx.de/u-boot-spi")

2187 int ext4fs_open(const char *filename, loff_t *len)
...
2196	status = ext4fs_find_file(filename, &ext4fs_root->diropen, ...
...
2208	ext4fs_file = fdiro;

calls

2163 int ext4fs_find_file(...
2164	struct ext2fs_node **foundnode, int expecttype)
...
2173	status = ext4fs_find_file1(path, rootnode, foundnode, ...

calls

2063 static int ext4fs_find_file1(const char *currpath,
...
2105		found = ext4fs_iterate_dir(currnode, name, &currnode,...
...
2154 			*currfound = currnode;

calls:

fs/ext4/ext4_common.c:
1896 int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
...
1941			fdiro = zalloc(sizeof(struct ext2fs_node));
...
1991					*fnode = fdiro;

[U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node()

[Tom Rini]

On Mon, Dec 14, 2015 at 12:32:13PM -0700, Stephen Warren wrote:
> On 12/09/2015 05:54 PM, Tom Rini wrote:
> >The function ext4fs_free_node() exists for dealing with "dirnode"
> >structures that we allocate.  However, we do not allocate these
> >dynamically as needed but rather as a single instance in ext4fs_mount()
> >that we zalloc().  Coverity scan notes that in two places we're doing
> >what it calls a "Free of address-of expression" as we were free()'ing
> >oldnode.  However, oldnode was never directly allocated, nor any other
> >instance which we were calling ext4fs_free_node() on.  Removing this
> >structure allows us to also restructure ext4fs_close() slightlu too.
> >
> >Tested on OMAP4 Pandaboard with Fedora 23 (/boot is ext4) as well as
> >reading and writing files from / to /boot and vice-versa and confirming
> >they read back again correctly.
> 
> I think if this change was valid, then we could delete the global
> variable ext4fs_file, and replace all references to it with a
> reference to the dirnode member in struct ext2_data?
> 
> Anyway, I believe that the-value-pointed-at-by-ext4fs_file is
> dynamically allocated outside struct ext2_data sometimes:
> 
> (All line number references relative to 5076c64a08d2 "Merge branch
> 'master' of git://git.denx.de/u-boot-spi")
> 
> 2187 int ext4fs_open(const char *filename, loff_t *len)
> ...
> 2196	status = ext4fs_find_file(filename, &ext4fs_root->diropen, ...
> ...
> 2208	ext4fs_file = fdiro;
> 
> calls
> 
> 2163 int ext4fs_find_file(...
> 2164	struct ext2fs_node **foundnode, int expecttype)
> ...
> 2173	status = ext4fs_find_file1(path, rootnode, foundnode, ...
> 
> calls
> 
> 2063 static int ext4fs_find_file1(const char *currpath,
> ...
> 2105		found = ext4fs_iterate_dir(currnode, name, &currnode,...
> ...
> 2154 			*currfound = currnode;
> 
> calls:
> 
> fs/ext4/ext4_common.c:
> 1896 int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
> ...
> 1941			fdiro = zalloc(sizeof(struct ext2fs_node));
> ...
> 1991					*fnode = fdiro;

Ah, yes, I missed that one, good catch, thanks!

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