Am 04.02.2015 07:48, schrieb NeilBrown:
> On Tue, 03 Feb 2015 11:19:58 +0100 walter harms <wharms@xxxxxx> wrote:
>
>>
>>
>> Am 03.02.2015 10:07, schrieb NeilBrown:
>>> On Tue, 03 Feb 2015 09:48:01 +0100 walter harms <wharms@xxxxxx> wrote:
>>>
>>>>
>>>>
>>>> Am 02.02.2015 20:46, schrieb NeilBrown:
>>>>> On Mon, 02 Feb 2015 16:20:42 +0100 SF Markus Elfring
>>>>> <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>>> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
>>>>>> Date: Mon, 2 Feb 2015 15:10:57 +0100
>>>>>>
>>>>>> The kfree() function tests whether its argument is NULL and then
>>>>>> returns immediately. Thus the test around the call is not needed.
>>>>>>
>>>>>> * This issue was detected by using the Coccinelle software.
>>>>>>
>>>>>> * Let us also move an assignment for the variable "pages" to the place
>>>>>> directly before it is really needed for a loop.
>>>>>>
>>>>>> * Let us also move another kfree() call into a block which should belong
>>>>>> to a previous check for the variable "bp".
>>>>>>
>>>>>> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/md/bitmap.c | 10 +++++-----
>>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>>>>>> index da3604e..47d72df 100644
>>>>>> --- a/drivers/md/bitmap.c
>>>>>> +++ b/drivers/md/bitmap.c
>>>>>> @@ -1586,15 +1586,15 @@ static void bitmap_free(struct bitmap *bitmap)
>>>>>> bitmap_file_unmap(&bitmap->storage);
>>>>>>
>>>>>> bp = bitmap->counts.bp;
>>>>>> - pages = bitmap->counts.pages;
>>>>>>
>>>>>> /* free all allocated memory */
>>>>>> -
>>>>>> - if (bp) /* deallocate the page memory */
>>>>>> + if (bp) { /* deallocate the page memory */
>>>>>> + pages = bitmap->counts.pages;
>>>>>> for (k = 0; k < pages; k++)
>>>>>> - if (bp[k].map && !bp[k].hijacked)
>>>>>> + if (!bp[k].hijacked)
>>>>>> kfree(bp[k].map);
>>>>>> - kfree(bp);
>>>>>> + kfree(bp);
>>>>>> + }
>>>>>> kfree(bitmap);
>>>>>> }
>>>>>>
>>>>>
>>>>> Hi,
>>>>> I'm somewhat amused that you removed a test for one kfree, but imposed a
>>>>> test on another. I realised the second test was already there, but why not
>>>>> just:
>>>>>
>>>>> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
>>>>> index da3604e73e8a..ad13b2e1bf1f 100644
>>>>> --- a/drivers/md/bitmap.c
>>>>> +++ b/drivers/md/bitmap.c
>>>>> @@ -1592,7 +1592,7 @@ static void bitmap_free(struct bitmap *bitmap)
>>>>>
>>>>> if (bp) /* deallocate the page memory */
>>>>> for (k = 0; k < pages; k++)
>>>>> - if (bp[k].map && !bp[k].hijacked)
>>>>> + if (!bp[k].hijacked)
>>>>> kfree(bp[k].map);
>>>>> kfree(bp);
>>>>> kfree(bitmap);
>>>>>
>>>>>
>>>>> It makes the intention of the patch much clearer.
>>>>>
>>>>> I'd probably prefer to leave the code as it is. I don't think either patch
>>>>> is really an improvement in readability, and readability trumps performance
>>>>> in places like this.
>>>>>
>>>>
>>>>
>>>> hello Neil,
>>>> just for my curiosity.
>>>> is it possible that a bp[k] exists but bp[k].hijacked is 0 ?
>>>> if that case: kfree(bp) would free an object in use.
>>>> otherwise hijacked seems useless here as it is possible to free
>>>> everything.
>>>>
>>>> re,
>>>> wh
>>>
>>> If .hijacked is 0, the .map is a pointer (that would need to be freed) or is
>>> NULL.
>>> If .hijacked is 1, then .map has been re-tasked as 2 16-bit counters. They
>>> are probably bother zero at this point, but I think it is safer to completely
>>> skip the 'free' if 'hijacked' is set as in that case 'map' isn't a pointer.
>>>
>>
>> It seems i was not clear enough, sorry about that.
>>
>> we have a loop to kfree(bp[k].map), thats fine how it is done.
>>
>> I am wondering about the kfree(bp) after the loop.
>> if bp[k].map is not freed for what ever reason how can it be freed later
>> after a kfree(bp) ?
>>
>> what is what i am missing ? Is there a hidden copy ?
>
> If bp[k].map is a pointer, it will be freed.
> If bp[k].map is not a pointer (because it has been hijacked), then there is
> nothing to free.
>
> So after the loop, all bp[k].maps that were allocated will have been freed.
>
> Does that help?
>
> NeilBrown
>
thx for explanation,
i am happy now, no further questions.
re,
wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
