All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node()
@ 2015-12-10  0:54 Tom Rini
  2015-12-14 19:32 ` Stephen Warren
  0 siblings, 1 reply; 3+ messages in thread
From: Tom Rini @ 2015-12-10  0:54 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-12-14 19:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-10  0:54 [U-Boot] [PATCH] fs: ext4: Remove ext4fs_free_node() Tom Rini
2015-12-14 19:32 ` Stephen Warren
2015-12-14 19:56   ` Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.