Re: [PATCH v5] btrfs: Fix memory leakage in the tree-log.c

[Geyslan GregÃrio Bem]

2013/10/11 Stefan Behrens <sbehrens@xxxxxxxxxxxxxxxx>:
> 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 <geyslan@xxxxxxxxx>
>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/