Re: [LTP] [POSIX][PATCH 1/2][v2]Use aio_error instead signal hander at aio_suspend/1-1

Cyril Hrubis
2010-11-25T19:12:02Z

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" ?

> >>               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.

> >> @@ -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 ;).

--
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
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.