Garrett Cooper wrote: > 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 ;). > > :). >
Thanks very much Garrett and Cyril, your answer/question does make this thread alive . :) Now, we have spent so much time to discuss the code style, I know it's important because it can help us for later working. But could we do it later on? My purpose is fixing this test, because is not according to POSIX, i think we should do more discussion about this. :) Thanks Bian ------------------------------------------------------------------------------ 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
