On Thu, Nov 25, 2010 at 11:24 AM, Cyril Hrubis <[email protected]> wrote:
> Hi!
>> >> if (ret) {
>> >> - printf(TNAME " Error at lio_listio() %d: %s\n", errno,
>> >> strerror(errno));
>> >> + printf (TNAME " Error at lio_listio() %d: %s\n",
>> >> + errno, strerror(errno));
>> >
>> > According to lkml coding style (that we are trying to use here), space
>> > should be only between C keywords and (). Not betweeen library calls
>> > like printf and so.
>>
>> This is true for LTP code, but this is third party code. The
>> documentation claims to conform to the LKML coding style though (even
>> though I know it isn't true).
>
> Wasn't the policy "fix everything that you touch" ?
Agreed -- within reason. Sometimes patches are committed with a bunch
of style changes to them even though the functional changes are
minimal. I usually go in after the fact to clean up style, because it
then makes the actual functional commit easy to parse out for other
people.
>> >> for (i=0; i<NUM_AIOCBS; i++)
>> >> free (aiocbs[i]);
>> >> free (bufs);
>> >> @@ -165,9 +147,10 @@ main ()
>> >> }
>> >>
>> >> /* Check selected request has not completed yet */
>> >> - if (received_selected) {
>> >> - printf (TNAME " Error : AIOCB %d already completed before
>> >> suspend\n",
>> >> - WAIT_FOR_AIOCB);
>> >> + err = aio_error(aiocbs[WAIT_FOR_AIOCB]);
>> >> + if (err != EINPROGRESS) {
>> >> + printf (TNAME " Error : AIOCB %d should not have completed "
>> >> + "before suspend\n", WAIT_FOR_AIOCB);
>> >> for (i=0; i<NUM_AIOCBS; i++)
>> >> free (aiocbs[i]);
>> >> free (bufs);
>> > ....
>> >> exit(PTS_FAIL);
>> >>}
>> >
>> > Another problem lies here. The test relies that aio operation in not
>> > finished when we got here. And as some linux kernels fallbacks aio as
>> > synchronous IO, when we got there, it's allready finished. So I think we
>> > should change this PTS_FAIL to PTS_UNRESOLVED.
>> >
>> > Garrett what you think?
>>
>> If an operating system doesn't fully implement a set of requirements,
>> it shouldn't claim to do so. If it does then that's a bug in the
>> implementation.
>
> I usually agree, but here, it could fail just because testing machine is
> too fast or because whatever else happen when scheduling processes/AIO
> writes. This is IMHO too fragile to be marked as FAIL.
Understood. Some of this junk has been working by accident for a long
time and it's time to fix things -- I just don't always agree with the
proposed solution because of the implications one could be introducing
in the said proposed solution :(...
>> >> @@ -178,11 +161,9 @@ main ()
>> >>
>> >> /* Suspend on selected request */
>> >> ret = aio_suspend((const struct aiocb **)plist, 2, NULL);
>> >
>> > Please remove useless cast to (const struct aiocb**)
>>
>> Depends on whether or not this is correct. According to GCC it matters:
>>
>> ../../../conformance/interfaces/aio_suspend/1-1.c:180: warning:
>> passing argument 1 of 'aio_suspend' from incompatible pointer type
>>
>> This should probably have the restrict c99 keyword attached to it though.
>
> Out of curiosity, what OS/gcc version does this? I have no warnings with
> all the casts removed from 1-1.c.
>
>> >> -
>> >> - /* Check selected request has completed */
>> >> - if (!received_selected) {
>> >> - printf (TNAME " Error : AIOCB %d should have completed
>> >> after suspend\n",
>> >> - WAIT_FOR_AIOCB);
>> >> + if (ret) {
>> >> + printf (TNAME " Error at aio_suspend() %d: %s\n",
>> >> + errno, strerror (errno));
>> >> for (i=0; i<NUM_AIOCBS; i++)
>> >> free (aiocbs[i]);
>> >> free (bufs);
>> >> @@ -191,9 +172,11 @@ main ()
>> >> exit (PTS_FAIL);
>> >> }
>> >
>> > Here again with the printf.
>> >
>> >> -
>> >> - if (ret) {
>> >> - printf (TNAME " Error at aio_suspend() %d: %s\n", errno,
>> >> strerror (errno));
>> >> + /* Check selected request has completed */
>> >> + err = aio_error(aiocbs[WAIT_FOR_AIOCB]);
>> >> + if (err) {
>> >> + printf (TNAME " Error : AIOCB %d should have completed
>> >> after "
>> >> + "suspend\n", WAIT_FOR_AIOCB);
>> >> for (i=0; i<NUM_AIOCBS; i++)
>> >> free (aiocbs[i]);
>> >> free (bufs);
>> >
>> > And here.
>>
>> I've cleaned up the style in this test to be more logical.
>> Unfortunately the bull.net contributed tests weren't cleaned up on
>> import to posix_testsuite, just like some of the syscall testcases in
>> LTP proper.
>
> That's why we are here, to fix the mess ;).
:).
------------------------------------------------------------------------------
Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
Tap into the largest installed PC base & get more eyes on your game by
optimizing for Intel(R) Graphics Technology. Get started today with the
Intel(R) Software Partner Program. Five $500 cash prizes are up for grabs.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
