Re: [PATCH][BZ 15089] Fixes malloc_trim always trims for large padding

[Fernando J V da Silva <fernandojvdasilva at gmail dot com>]

Hi Will and OndÅej!

Thanks for your reviews!

Just to make sure I understood... The concern regarding copyright is
because of the fix was based on suggestion by Thiago Ize on Bugzilla,
as I mentioned on the commit message, right? If that is the case, I
should put only an "Ideas by" entry for him at the copyright part of
the .c file, is that correct?

Cheers,

Fernando J V da Silva


2013/11/20 OndÅej BÃlka <neleai@seznam.cz>:
> On Wed, Nov 20, 2013 at 10:28:16AM +0000, Will Newton wrote:
>> On 20 November 2013 04:38, Fernando J V da Silva
>> <fernandojvdasilva@gmail.com> wrote:
>> > Hi all!
>> >
>> > I'm starting learning about glibc code and I've began trying to fix
>> > the bug 15089. I've put some comments on Bugzilla and I wrote a patch
>> > (attached) for fixing it, based on the solution proposed by Thiago
>> > Ize' comment in Bugzilla as well
>> > (https://sourceware.org/bugzilla/show_bug.cgi?id=15089).
>> >
>> > That solution really fixes the problem, but the systrim() code still
>> > mixes some signed and unsigned operations to calculate the size used
>> > to call sbrk(), which may be dangerous (and in fact caused this bug).
>> > I think the most correct solution would be to refactor this function
>> > for only using unsigned variables but, to be honest, I haven't tried
>> > it as I'm new on that (may be someone could give me some advices about
>> > it?).
>> >
>> > I would be glad if someone could have some time to comment it! :-)
>>
>> Hi Fernando,
>>
>> Thanks for the patch!
>>
>> In general the ternary operator tends to be quite hard to read so in
>> my opinion is best avoided outside of macros. The overflow case should
>> be quite simple to handle with a return 0, e.g.:
>>
>> if (top_area <= pad)
>>   return 0;
>>
>> Then we can remove the whole "extra > 0" conditional below as well.
>>
>> Also you need a GNU changelog entry to accompany your change (include
>> it in the commit message rather than as a patch to ChangeLog as it
>> makes it easier to merge) which mentions the chnages made and the BZ
>> number that is being closed (see ChangeLog for examples of this).
>>
>> Finally, and perhaps the most difficult, I think this change may need
>> a copyright assignment on file to be accepted (see
>> https://sourceware.org/glibc/wiki/Contribution%20checklist).
>>
> Copyrigth assignment is not needed for this patch but it is better to get it in
> advance.
>
> You need copyrigth assignment once total lines that you contributed
> exceed 15 lines, see
>
> http://www.gnu.org/prep/maintain/maintain.html#Legally-Significant
>
>
>