2013/10/11 Stefan Behrens <sbehr...@giantdisaster.de>: > On Thu, 10 Oct 2013 19:11:22 -0300, Geyslan G. Bem wrote: >> In add_inode_ref() function: >> >> Initializes local pointers. >> >> Reduces the logical condition with the __add_inode_ref() return >> value by using only one 'goto out'. >> >> Centralizes the exiting, ensuring the freeing of all used memory. >> >> Signed-off-by: Geyslan G. Bem <geys...@gmail.com> > > The patch looks correct to me, but there are some nitpicking style issues. > > >> --- >> fs/btrfs/tree-log.c | 36 ++++++++++++++++++++++-------------- >> 1 file changed, 22 insertions(+), 14 deletions(-) >> >> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c >> index 79f057c..beb41b0 100644 >> --- a/fs/btrfs/tree-log.c >> +++ b/fs/btrfs/tree-log.c >> @@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> struct extent_buffer *eb, int slot, >> struct btrfs_key *key) >> { >> - struct inode *dir; >> - struct inode *inode; >> + struct inode *dir = NULL; >> + struct inode *inode = NULL; >> unsigned long ref_ptr; >> unsigned long ref_end; >> - char *name; >> + char *name = NULL; >> int namelen; >> int ret; >> int search_done = 0; >> @@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> * care of the rest >> */ >> dir = read_one_inode(root, parent_objectid); >> - if (!dir) >> - return -ENOENT; >> + if (!dir) { >> + ret = -ENOENT; >> + goto out; >> + } >> >> inode = read_one_inode(root, inode_objectid); >> if (!inode) { >> - iput(dir); >> - return -EIO; >> + ret = -EIO; >> + goto out; >> } >> >> while (ref_ptr < ref_end) { >> @@ -1169,14 +1171,17 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> */ >> if (!dir) >> dir = read_one_inode(root, parent_objectid); >> - if (!dir) >> - return -ENOENT; >> + if (!dir) { >> + ret = -ENOENT; >> + goto out; >> + } >> } else { >> ret = ref_get_fields(eb, ref_ptr, &namelen, &name, >> &ref_index); >> } >> if (ret) >> - return ret; >> + goto out; >> + > > This additional empty line is not required, we would have two empty > lines in a row.
Ok, I'll remove it. > > >> >> /* if we already have a perfect match, we're done */ >> if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode), >> @@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> parent_objectid, >> ref_index, name, namelen, >> &search_done); >> - if (ret == 1) { >> - ret = 0; >> + > > This empty line is also not required. You know, this hurts on regular > 80x24 terminals. Do you get more than 24 lines on your display? > I thought there was a rule for this in Documentation/CodingStyle, but > there isn't. Therefore it's up to you. But the two empty lines above are > definitely not wanted. > Yes, I get more than 24 lines. But no problem, I know about the Coding Style. This issue will be fixed. > >> + if (ret) { >> + if (ret == 1) >> + ret--; > > I don't see a reason to change the "ret = 0" to a "ret--", this is not > an optimization and makes the code less readable. > Really. Using -O2 the code is equal. I'll redo to "ret = 0;" with the new conditional scope. > >> goto out; >> } >> - if (ret) >> - goto out; >> } >> >> /* insert our name */ >> @@ -1215,6 +1220,8 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> >> ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen; >> kfree(name); >> + name = NULL; >> + > > Another empty line which is not required for the purpose of this patch. Ok, it'll be removed. > > >> if (log_ref_ver) { >> iput(dir); >> dir = NULL; >> @@ -1225,6 +1232,7 @@ static noinline int add_inode_ref(struct >> btrfs_trans_handle *trans, >> ret = overwrite_item(trans, root, path, eb, slot, key); >> out: >> btrfs_release_path(path); >> + kfree(name); >> iput(dir); >> iput(inode); >> return ret; >> > > Thank you again. Geyslan G. Bem -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/