Re: [PATCH v2] malloc: Add posix_memalign test.

[Adhemerval Zanella <azanella at linux dot vnet dot ibm 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.
>