fs: ext4: Remove ext4fs_free_node()
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.
---
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
@@ -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
@@ -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
@@ -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
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 inext4fs_mount()that wezalloc(). 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 callingext4fs_free_node()on. Removing this structure allows us to also restructureext4fs_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;
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 inext4fs_mount()that wezalloc(). 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 callingext4fs_free_node()on. Removing this structure allows us to also restructureext4fs_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 structext2_data?Anyway, I believe that the-value-pointed-at-by-ext4fs_file is dynamically allocated outside struct
ext2_datasometimes:(All line number references relative to
5076c64a08d2"Merge branch 'master' ofgit://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, ...
...
2208ext4fs_file= fdiro;calls
2163 int ext4fs_find_file(...
2164 structext2fs_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(structext2fs_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>