This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] malloc: Add posix_memalign test.
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: "Carlos O'Donell" <carlos at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 02 Oct 2013 14:18:53 -0300
- Subject: Re: [PATCH v2] malloc: Add posix_memalign test.
- Authentication-results: sourceware.org; auth=none
- References: <523042B4 dot 90602 at linaro dot org> <524C4D9E dot 3070701 at redhat dot com> <524C5073 dot 3010402 at linux dot vnet dot ibm dot com> <524C5280 dot 9000403 at redhat dot com>
On 02-10-2013 14:06, Carlos O'Donell wrote:
> On 10/02/2013 12:57 PM, Adhemerval Zanella wrote:
>> On 02-10-2013 13:45, Carlos O'Donell wrote:
>>> Will,
>>>
>>> Thanks for the additional test case.
>>>
>>> Some comments below, there are some things that
>>> should get fixed up.
>>>
>>>> + if (ret == ENOMEM && p != NULL)
>>>> + merror ("returned an error but pointer was modified");
>>>> +
>>> OK.
>>>
>>> Needs free (p).
>> From my point of view, there is no need in calling free on a NULL ponter. In fact
>> free(NULL) will only call the malloc hook, if any, and still in this case I don't
>> see it needed. It also aplies for other cases where you require the free().
> If p is non-null then posix_memalign allocated
> something.
>
> The standard then says:
> ~~~
> The free() function shall deallocate memory that
> has previously been allocated by posix_memalign()
> ~~~
>
> So we might as well test `free (p)' also works
> correctly for each test?
>
> It's beneficial IMO to go through the whole sequence
> of calls as the user would also and that includes
> calling free even if p is NULL to exercise the hook
> code.
Well, I'm not opposing on the 'free' call itself. I just think it is not really
required for testing purpose since an error will be reported previously and
freeing memory in this case might be just undefined behavior.
>>>> + ret = posix_memalign (&p, sizeof (void *), 0);
>>>> +
>>>> + if (ret != 0 || p == NULL)
>>>> + merror ("posix_memalign (&p, sizeof (void *), 0) failed.");
>>>> +
>>>> + free (p);
>>> OK.
>>>
>>>> +
>>> Test case needs comment e.g.
>>> /* Test that a multiple of sizeof(void *) but not a power of two, fails. */
>>>
>>> Should set p to NULL (for later free).
>> Also, posix_memalign disconsider the value of '*memptr', so I also think it is not needed
>> to really sets it to NULL.
> Could you please clarify what you mean by `disconsider?'
>
> Do you mean to say *memptr is set to NULL?
Sorry, "disconsider" is not even a word, my bad. I meant the pointer value won't be considered
on the posix_memalign code, it will just sets the *memptr value with the allocated value
internally.
>
> I was worried it might be left unmodified and that's problematic.
I thought so, but also any non-default case will trigger an error.
>
> Cheers,
> Carlos.
>