Skip to contentSkip to conversation

fs: ext4: Remove ext4fs_free_node()

PATCH New Tom Rini 2 participants 3 messages Dec 10, 2015 – Dec 14, 2015 ext4 file system

lore
Share & tools
lore

Tom Rini

author
#0 lore raw

PATCH 1/1 fs: ext4: Remove ext4fs_free_node()

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 <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Sjoerd Simons <[email protected]>
Cc Simon Glass <[email protected]>
Cc Przemyslaw Marczak <[email protected]>
Reported-by Coverity (CID 131278, 131091)

---
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(-)
fs/ext4/ext4_common.c +9 -29
index e73223a..14096e0 100644
@@ -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)
fs/ext4/ext4fs.c +0 -7
index 258b9379..36f8023 100644
@@ -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;
 }
include/ext4fs.h +0 -1
index 6888adc..9cb7882 100644
@@ -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);
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 fs: ext4: Remove ext4fs_free_node()

Quoted 9 lines · 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.

Quoted 3 lines

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;

author
#2 lore raw

PATCH 1/1 fs: ext4: Remove ext4fs_free_node()

Quoted 45 lines · Stephen Warren
Quoted 9 lines · 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.

Quoted 3 lines

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;

Quoted 8 lines

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!

Signature · 8 lines
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>