From: Bian N. <bi...@cn...> - 2010-11-11 03:08:57
|
aio_suspend may be interrupted by IO request's completion signal, so we should use aio_error instead signal hander to check whether this request has complete. Signed-off-by: Bian Naimeng <bi...@cn...> ----- .../conformance/interfaces/aio_suspend/1-1.c | 49 ++++++--------- .../conformance/interfaces/aio_suspend/4-1.c | 65 +++++++------------- 2 files changed, 41 insertions(+), 73 deletions(-) -- Regards Bian Naimeng |
From: Bian N. <bi...@cn...> - 2010-11-11 03:17:02
|
aio_suspend may be interrupted by IO request's completion signal, so we should use aio_error instead signal hander to check whether this request has complete. Signed-off-by: Bian Naimeng <bi...@cn...> --- .../conformance/interfaces/aio_suspend/1-1.c | 49 +++++++------------ 1 files changed, 18 insertions(+), 31 deletions(-) diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c index 7d33e62..63d986e 100644 --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c @@ -44,18 +44,23 @@ #define BUF_SIZE 1024*1024 #define WAIT_FOR_AIOCB 6 -int received_selected = 0; int received_all = 0; -void -sigrt1_handler(int signum, siginfo_t *info, void *context) +int selected_request_status(struct aiocb *paiocb, int expect) { - if (info->si_value.sival_int == WAIT_FOR_AIOCB) - received_selected = 1; + int err = aio_error(paiocb); + + if (err != expect) { + printf (TNAME " Selected request expect %d[%s], not %d[%s]\n", + expect, strerror(expect), err, strerror(err)); + return 0; + } + + return 1; } void -sigrt2_handler(int signum, siginfo_t *info, void *context) +sigrt1_handler(int signum, siginfo_t *info, void *context) { received_all = 1; } @@ -123,37 +128,25 @@ main () aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; aiocbs[i]->aio_nbytes = BUF_SIZE; aiocbs[i]->aio_lio_opcode = LIO_READ; - - /* Use SIRTMIN+1 for individual completions */ - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; } /* Use SIGRTMIN+2 for list completion */ event.sigev_notify = SIGEV_SIGNAL; - event.sigev_signo = SIGRTMIN+2; + event.sigev_signo = SIGRTMIN+1; event.sigev_value.sival_ptr = NULL; - /* Setup handler for individual operation completion */ + /* Setup handler for list completion */ action.sa_sigaction = sigrt1_handler; sigemptyset(&action.sa_mask); action.sa_flags = SA_SIGINFO|SA_RESTART; sigaction(SIGRTMIN+1, &action, NULL); - /* Setup handler for list completion */ - action.sa_sigaction = sigrt2_handler; - sigemptyset(&action.sa_mask); - action.sa_flags = SA_SIGINFO|SA_RESTART; - sigaction(SIGRTMIN+2, &action, NULL); - /* Setup suspend list */ plist[0] = NULL; plist[1] = aiocbs[WAIT_FOR_AIOCB]; /* Submit request list */ ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); - if (ret) { printf(TNAME " Error at lio_listio() %d: %s\n", errno, strerror(errno)); for (i=0; i<NUM_AIOCBS; i++) @@ -165,9 +158,7 @@ main () } /* Check selected request has not completed yet */ - if (received_selected) { - printf (TNAME " Error : AIOCB %d already completed before suspend\n", - WAIT_FOR_AIOCB); + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -178,11 +169,8 @@ main () /* Suspend on selected request */ ret = aio_suspend((const struct aiocb **)plist, 2, NULL); - - /* 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 +179,8 @@ main () exit (PTS_FAIL); } - - if (ret) { - printf (TNAME " Error at aio_suspend() %d: %s\n", errno, strerror (errno)); + /* Check selected request has completed */ + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], 0)) { for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); -- 1.7.0.4 |
From: Cyril H. <ch...@su...> - 2010-11-11 14:40:42
|
Hi! > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. > > Signed-off-by: Bian Naimeng <bi...@cn...> > > --- > .../conformance/interfaces/aio_suspend/1-1.c | 49 +++++++------------ > 1 files changed, 18 insertions(+), 31 deletions(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > index 7d33e62..63d986e 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > @@ -44,18 +44,23 @@ > #define BUF_SIZE 1024*1024 > #define WAIT_FOR_AIOCB 6 > > -int received_selected = 0; > int received_all = 0; > > -void > -sigrt1_handler(int signum, siginfo_t *info, void *context) > +int selected_request_status(struct aiocb *paiocb, int expect) > { > - if (info->si_value.sival_int == WAIT_FOR_AIOCB) > - received_selected = 1; > + int err = aio_error(paiocb); > + > + if (err != expect) { > + printf (TNAME " Selected request expect %d[%s], not %d[%s]\n", > + expect, strerror(expect), err, strerror(err)); > + return 0; > + } > + > + return 1; > } > > void > -sigrt2_handler(int signum, siginfo_t *info, void *context) > +sigrt1_handler(int signum, siginfo_t *info, void *context) > { > received_all = 1; > } > @@ -123,37 +128,25 @@ main () > aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; > aiocbs[i]->aio_nbytes = BUF_SIZE; > aiocbs[i]->aio_lio_opcode = LIO_READ; > - > - /* Use SIRTMIN+1 for individual completions */ > - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; > - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; > - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; > } > > /* Use SIGRTMIN+2 for list completion */ > event.sigev_notify = SIGEV_SIGNAL; > - event.sigev_signo = SIGRTMIN+2; > + event.sigev_signo = SIGRTMIN+1; > event.sigev_value.sival_ptr = NULL; Fix comment here. > - /* Setup handler for individual operation completion */ > + /* Setup handler for list completion */ > action.sa_sigaction = sigrt1_handler; > sigemptyset(&action.sa_mask); > action.sa_flags = SA_SIGINFO|SA_RESTART; > sigaction(SIGRTMIN+1, &action, NULL); > > - /* Setup handler for list completion */ > - action.sa_sigaction = sigrt2_handler; > - sigemptyset(&action.sa_mask); > - action.sa_flags = SA_SIGINFO|SA_RESTART; > - sigaction(SIGRTMIN+2, &action, NULL); > - > /* Setup suspend list */ > plist[0] = NULL; > plist[1] = aiocbs[WAIT_FOR_AIOCB]; > > /* Submit request list */ > ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); > - No need to remove newline here. > if (ret) { > printf(TNAME " Error at lio_listio() %d: %s\n", errno, strerror(errno)); > for (i=0; i<NUM_AIOCBS; i++) > @@ -165,9 +158,7 @@ main () > } > > /* Check selected request has not completed yet */ > - if (received_selected) { > - printf (TNAME " Error : AIOCB %d already completed before suspend\n", > - WAIT_FOR_AIOCB); > + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); Any particular need to removing the printf() here? > @@ -178,11 +169,8 @@ main () > > /* Suspend on selected request */ > ret = aio_suspend((const struct aiocb **)plist, 2, NULL); > - > - /* 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 +179,8 @@ main () > exit (PTS_FAIL); > } > > - > - if (ret) { > - printf (TNAME " Error at aio_suspend() %d: %s\n", errno, strerror (errno)); > + /* Check selected request has completed */ > + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], 0)) { > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); And here. -- Cyril Hrubis ch...@su... |
From: Bian N. <bi...@cn...> - 2010-11-12 01:16:55
|
Cyril Hrubis wrote: > Hi! >> aio_suspend may be interrupted by IO request's completion signal, so we >> should use aio_error instead signal hander to check whether this request >> has complete. ... snip ... >> /* Use SIGRTMIN+2 for list completion */ >> event.sigev_notify = SIGEV_SIGNAL; >> - event.sigev_signo = SIGRTMIN+2; >> + event.sigev_signo = SIGRTMIN+1; >> event.sigev_value.sival_ptr = NULL; > > Fix comment here. Thanks. > >> - /* Setup handler for individual operation completion */ >> + /* Setup handler for list completion */ >> action.sa_sigaction = sigrt1_handler; >> sigemptyset(&action.sa_mask); >> action.sa_flags = SA_SIGINFO|SA_RESTART; >> sigaction(SIGRTMIN+1, &action, NULL); >> >> - /* Setup handler for list completion */ >> - action.sa_sigaction = sigrt2_handler; >> - sigemptyset(&action.sa_mask); >> - action.sa_flags = SA_SIGINFO|SA_RESTART; >> - sigaction(SIGRTMIN+2, &action, NULL); >> - >> /* Setup suspend list */ >> plist[0] = NULL; >> plist[1] = aiocbs[WAIT_FOR_AIOCB]; >> >> /* Submit request list */ >> ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); >> - > > No need to remove newline here. > >> if (ret) { >> printf(TNAME " Error at lio_listio() %d: %s\n", errno, strerror(errno)); >> for (i=0; i<NUM_AIOCBS; i++) >> @@ -165,9 +158,7 @@ main () >> } >> >> /* Check selected request has not completed yet */ >> - if (received_selected) { >> - printf (TNAME " Error : AIOCB %d already completed before suspend\n", >> - WAIT_FOR_AIOCB); >> + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { >> for (i=0; i<NUM_AIOCBS; i++) >> free (aiocbs[i]); >> free (bufs); > > Any particular need to removing the printf() here? > en..., i will send a new path without removing this printf. >> @@ -178,11 +169,8 @@ main () >> >> /* Suspend on selected request */ >> ret = aio_suspend((const struct aiocb **)plist, 2, NULL); >> - >> - /* 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 +179,8 @@ main () >> exit (PTS_FAIL); >> } >> >> - >> - if (ret) { >> - printf (TNAME " Error at aio_suspend() %d: %s\n", errno, strerror (errno)); >> + /* Check selected request has completed */ >> + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], 0)) { >> for (i=0; i<NUM_AIOCBS; i++) >> free (aiocbs[i]); >> free (bufs); > > And here. ditto, thanks. > -- Regards Bian Naimeng |
From: Bian N. <bi...@cn...> - 2010-11-11 03:18:40
|
aio_suspend may be interrupted by IO request's completion signal, so we should use aio_error instead signal hander to check whether this request has complete. Signed-off-by: Bian Naimeng <bi...@cn...> --- .../conformance/interfaces/aio_suspend/4-1.c | 65 +++++++------------- 1 files changed, 23 insertions(+), 42 deletions(-) diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c index 260c26a..6699ab4 100644 --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c @@ -41,18 +41,24 @@ #define BUF_SIZE 1024*1024 #define WAIT_FOR_AIOCB 6 -int received_selected = 0; int received_all = 0; -void -sigrt1_handler(int signum, siginfo_t *info, void *context) +int +selected_request_status(struct aiocb *paiocb, int expect) { - if (info->si_value.sival_int == WAIT_FOR_AIOCB) - received_selected = 1; + int err = aio_error(paiocb); + + if (err != expect) { + printf (TNAME " Selected request expect %d[%s], not %d[%s]\n", + expect, strerror(expect), err, strerror(err)); + return 0; + } + + return 1; } void -sigrt2_handler(int signum, siginfo_t *info, void *context) +sigrt1_handler(int signum, siginfo_t *info, void *context) { received_all = 1; } @@ -68,7 +74,7 @@ main () char *bufs; struct sigaction action; struct sigevent event; - struct timespec ts = {0, 10000000}; /* 10 ms */ + struct timespec ts = {0, 1000}; /* 1 us */ int errors = 0; int ret; int err; @@ -121,39 +127,28 @@ main () aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; aiocbs[i]->aio_nbytes = BUF_SIZE; aiocbs[i]->aio_lio_opcode = LIO_READ; - - /* Use SIRTMIN+1 for individual completions */ - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; } /* Use SIGRTMIN+2 for list completion */ event.sigev_notify = SIGEV_SIGNAL; - event.sigev_signo = SIGRTMIN+2; + event.sigev_signo = SIGRTMIN+1; event.sigev_value.sival_ptr = NULL; - /* Setup handler for individual operation completion */ + /* Setup handler for list completion */ action.sa_sigaction = sigrt1_handler; sigemptyset(&action.sa_mask); action.sa_flags = SA_SIGINFO|SA_RESTART; sigaction(SIGRTMIN+1, &action, NULL); - /* Setup handler for list completion */ - action.sa_sigaction = sigrt2_handler; - sigemptyset(&action.sa_mask); - action.sa_flags = SA_SIGINFO|SA_RESTART; - sigaction(SIGRTMIN+2, &action, NULL); - /* Setup suspend list */ plist[0] = NULL; plist[1] = aiocbs[WAIT_FOR_AIOCB]; /* Submit request list */ ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); - 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)); for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -163,9 +158,7 @@ main () } /* Check selected request has not completed yet */ - if (received_selected) { - printf (TNAME " Error : AIOCB %d already completed before suspend\n", - WAIT_FOR_AIOCB); + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -177,21 +170,10 @@ main () /* Suspend on selected request */ ret = aio_suspend((const struct aiocb **)plist, 2, &ts); - /* Check selected request has not completed */ - if (received_selected) { - printf (TNAME " Error : AIOCB %d should not have completed after timed out suspend\n", - WAIT_FOR_AIOCB); - for (i=0; i<NUM_AIOCBS; i++) - free (aiocbs[i]); - free (bufs); - free (aiocbs); - close (fd); - exit (PTS_FAIL); - } - /* timed out aio_suspend should return -1 and set errno to EAGAIN */ - if (ret != -1) { - printf (TNAME " aio_suspend() should return -1\n"); + if (ret != -1 || errno != EAGAIN) { + printf (TNAME " aio_suspend() should return -1 and set errno " + "to EAGAIN: %d, %s\n", ret, strerror(errno)); for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -200,9 +182,8 @@ main () exit (PTS_FAIL); } - if (errno != EAGAIN) { - printf (TNAME " aio_suspend() should set errno to EAGAIN: %d (%s)\n", - errno, strerror (errno)); + /* Check selected request has not completed */ + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); -- 1.7.0.4 |
From: Cyril H. <ch...@su...> - 2010-11-11 14:49:00
|
Hi! > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. > > Signed-off-by: Bian Naimeng <bi...@cn...> > > --- > .../conformance/interfaces/aio_suspend/4-1.c | 65 +++++++------------- > 1 files changed, 23 insertions(+), 42 deletions(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > index 260c26a..6699ab4 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > @@ -41,18 +41,24 @@ > #define BUF_SIZE 1024*1024 > #define WAIT_FOR_AIOCB 6 > > -int received_selected = 0; > int received_all = 0; > > -void > -sigrt1_handler(int signum, siginfo_t *info, void *context) > +int > +selected_request_status(struct aiocb *paiocb, int expect) > { > - if (info->si_value.sival_int == WAIT_FOR_AIOCB) > - received_selected = 1; > + int err = aio_error(paiocb); > + > + if (err != expect) { > + printf (TNAME " Selected request expect %d[%s], not %d[%s]\n", > + expect, strerror(expect), err, strerror(err)); > + return 0; > + } > + > + return 1; > } > > void > -sigrt2_handler(int signum, siginfo_t *info, void *context) > +sigrt1_handler(int signum, siginfo_t *info, void *context) > { > received_all = 1; > } > @@ -68,7 +74,7 @@ main () > char *bufs; > struct sigaction action; > struct sigevent event; > - struct timespec ts = {0, 10000000}; /* 10 ms */ > + struct timespec ts = {0, 1000}; /* 1 us */ > int errors = 0; > int ret; > int err; > @@ -121,39 +127,28 @@ main () > aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; > aiocbs[i]->aio_nbytes = BUF_SIZE; > aiocbs[i]->aio_lio_opcode = LIO_READ; > - > - /* Use SIRTMIN+1 for individual completions */ > - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; > - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; > - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; > } > > /* Use SIGRTMIN+2 for list completion */ > event.sigev_notify = SIGEV_SIGNAL; > - event.sigev_signo = SIGRTMIN+2; > + event.sigev_signo = SIGRTMIN+1; > event.sigev_value.sival_ptr = NULL; Same here. > - /* Setup handler for individual operation completion */ > + /* Setup handler for list completion */ > action.sa_sigaction = sigrt1_handler; > sigemptyset(&action.sa_mask); > action.sa_flags = SA_SIGINFO|SA_RESTART; > sigaction(SIGRTMIN+1, &action, NULL); > > - /* Setup handler for list completion */ > - action.sa_sigaction = sigrt2_handler; > - sigemptyset(&action.sa_mask); > - action.sa_flags = SA_SIGINFO|SA_RESTART; > - sigaction(SIGRTMIN+2, &action, NULL); > - > /* Setup suspend list */ > plist[0] = NULL; > plist[1] = aiocbs[WAIT_FOR_AIOCB]; > > /* Submit request list */ > ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); > - Here. > 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)); > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); > @@ -163,9 +158,7 @@ main () > } > > /* Check selected request has not completed yet */ > - if (received_selected) { > - printf (TNAME " Error : AIOCB %d already completed before suspend\n", > - WAIT_FOR_AIOCB); > + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); And here. > @@ -177,21 +170,10 @@ main () > /* Suspend on selected request */ > ret = aio_suspend((const struct aiocb **)plist, 2, &ts); Useless cast (and I've missed it in comments for previous patch). > - /* Check selected request has not completed */ > - if (received_selected) { > - printf (TNAME " Error : AIOCB %d should not have completed after timed out suspend\n", > - WAIT_FOR_AIOCB); > - for (i=0; i<NUM_AIOCBS; i++) > - free (aiocbs[i]); > - free (bufs); > - free (aiocbs); > - close (fd); > - exit (PTS_FAIL); > - } > - > /* timed out aio_suspend should return -1 and set errno to EAGAIN */ > - if (ret != -1) { > - printf (TNAME " aio_suspend() should return -1\n"); > + if (ret != -1 || errno != EAGAIN) { > + printf (TNAME " aio_suspend() should return -1 and set errno " > + "to EAGAIN: %d, %s\n", ret, strerror(errno)); > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); > @@ -200,9 +182,8 @@ main () > exit (PTS_FAIL); > } > > - if (errno != EAGAIN) { > - printf (TNAME " aio_suspend() should set errno to EAGAIN: %d (%s)\n", > - errno, strerror (errno)); > + /* Check selected request has not completed */ > + if (!selected_request_status(aiocbs[WAIT_FOR_AIOCB], EINPROGRESS)) { > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); I think we should print here, why the test has failed. -- Cyril Hrubis ch...@su... |
From: Bian N. <bi...@cn...> - 2010-11-12 02:13:30
|
aio_suspend may be interrupted by IO request's completion signal, so we should use aio_error instead signal hander to check whether this request has complete. Signed-off-by: Bian Naimeng <bi...@cn...> --- .../conformance/interfaces/aio_suspend/4-1.c | 62 ++++++-------------- 1 files changed, 18 insertions(+), 44 deletions(-) diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c index 260c26a..bb90234 100644 --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c @@ -41,19 +41,11 @@ #define BUF_SIZE 1024*1024 #define WAIT_FOR_AIOCB 6 -int received_selected = 0; int received_all = 0; void sigrt1_handler(int signum, siginfo_t *info, void *context) { - if (info->si_value.sival_int == WAIT_FOR_AIOCB) - received_selected = 1; -} - -void -sigrt2_handler(int signum, siginfo_t *info, void *context) -{ received_all = 1; } @@ -68,7 +60,7 @@ main () char *bufs; struct sigaction action; struct sigevent event; - struct timespec ts = {0, 10000000}; /* 10 ms */ + struct timespec ts = {0, 1000}; /* 1 us */ int errors = 0; int ret; int err; @@ -121,30 +113,19 @@ main () aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; aiocbs[i]->aio_nbytes = BUF_SIZE; aiocbs[i]->aio_lio_opcode = LIO_READ; - - /* Use SIRTMIN+1 for individual completions */ - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; } - /* Use SIGRTMIN+2 for list completion */ + /* Use SIGRTMIN+1 for list completion */ event.sigev_notify = SIGEV_SIGNAL; - event.sigev_signo = SIGRTMIN+2; + event.sigev_signo = SIGRTMIN+1; event.sigev_value.sival_ptr = NULL; - /* Setup handler for individual operation completion */ + /* Setup handler for list completion */ action.sa_sigaction = sigrt1_handler; sigemptyset(&action.sa_mask); action.sa_flags = SA_SIGINFO|SA_RESTART; sigaction(SIGRTMIN+1, &action, NULL); - /* Setup handler for list completion */ - action.sa_sigaction = sigrt2_handler; - sigemptyset(&action.sa_mask); - action.sa_flags = SA_SIGINFO|SA_RESTART; - sigaction(SIGRTMIN+2, &action, NULL); - /* Setup suspend list */ plist[0] = NULL; plist[1] = aiocbs[WAIT_FOR_AIOCB]; @@ -153,7 +134,8 @@ main () ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); 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)); for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -163,9 +145,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); @@ -177,21 +160,10 @@ main () /* Suspend on selected request */ ret = aio_suspend((const struct aiocb **)plist, 2, &ts); - /* Check selected request has not completed */ - if (received_selected) { - printf (TNAME " Error : AIOCB %d should not have completed after timed out suspend\n", - WAIT_FOR_AIOCB); - for (i=0; i<NUM_AIOCBS; i++) - free (aiocbs[i]); - free (bufs); - free (aiocbs); - close (fd); - exit (PTS_FAIL); - } - /* timed out aio_suspend should return -1 and set errno to EAGAIN */ - if (ret != -1) { - printf (TNAME " aio_suspend() should return -1\n"); + if (ret != -1 || errno != EAGAIN) { + printf (TNAME " aio_suspend() should return -1 and set errno " + "to EAGAIN: %d, %s\n", ret, strerror(errno)); for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); @@ -200,9 +172,11 @@ main () exit (PTS_FAIL); } - if (errno != EAGAIN) { - printf (TNAME " aio_suspend() should set errno to EAGAIN: %d (%s)\n", - errno, strerror (errno)); + /* Check selected request has not completed */ + err = aio_error(aiocbs[WAIT_FOR_AIOCB]); + if (err != EINPROGRESS) { + printf (TNAME " Error : AIOCB %d should not have completed " + "after timed out suspend\n", WAIT_FOR_AIOCB); for (i=0; i<NUM_AIOCBS; i++) free (aiocbs[i]); free (bufs); -- 1.7.0.4 |
From: Cyril H. <ch...@su...> - 2010-11-18 18:23:01
|
Hi! > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. > > Signed-off-by: Bian Naimeng <bi...@cn...> > > --- > .../conformance/interfaces/aio_suspend/4-1.c | 62 ++++++-------------- > 1 files changed, 18 insertions(+), 44 deletions(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > index 260c26a..bb90234 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c > @@ -41,19 +41,11 @@ > #define BUF_SIZE 1024*1024 > #define WAIT_FOR_AIOCB 6 > > -int received_selected = 0; > int received_all = 0; > > void > sigrt1_handler(int signum, siginfo_t *info, void *context) > { > - if (info->si_value.sival_int == WAIT_FOR_AIOCB) > - received_selected = 1; > -} > - > -void > -sigrt2_handler(int signum, siginfo_t *info, void *context) > -{ > received_all = 1; > } > > @@ -68,7 +60,7 @@ main () > char *bufs; > struct sigaction action; > struct sigevent event; > - struct timespec ts = {0, 10000000}; /* 10 ms */ > + struct timespec ts = {0, 1000}; /* 1 us */ > int errors = 0; > int ret; > int err; > @@ -121,30 +113,19 @@ main () > aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; > aiocbs[i]->aio_nbytes = BUF_SIZE; > aiocbs[i]->aio_lio_opcode = LIO_READ; > - > - /* Use SIRTMIN+1 for individual completions */ > - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; > - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; > - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; > } > > - /* Use SIGRTMIN+2 for list completion */ > + /* Use SIGRTMIN+1 for list completion */ > event.sigev_notify = SIGEV_SIGNAL; > - event.sigev_signo = SIGRTMIN+2; > + event.sigev_signo = SIGRTMIN+1; > event.sigev_value.sival_ptr = NULL; > > - /* Setup handler for individual operation completion */ > + /* Setup handler for list completion */ > action.sa_sigaction = sigrt1_handler; > sigemptyset(&action.sa_mask); > action.sa_flags = SA_SIGINFO|SA_RESTART; > sigaction(SIGRTMIN+1, &action, NULL); > > - /* Setup handler for list completion */ > - action.sa_sigaction = sigrt2_handler; > - sigemptyset(&action.sa_mask); > - action.sa_flags = SA_SIGINFO|SA_RESTART; > - sigaction(SIGRTMIN+2, &action, NULL); > - > /* Setup suspend list */ > plist[0] = NULL; > plist[1] = aiocbs[WAIT_FOR_AIOCB]; > @@ -153,7 +134,8 @@ main () > ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); > > 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)); > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); Once again, space here. > @@ -163,9 +145,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); > } Spaces after printf and free. And the same problem with relying on AIO operation not finished yet. So I propose to change this PTS_FAIL to PTS_UNRESOLVED. > @@ -177,21 +160,10 @@ main () > /* Suspend on selected request */ > ret = aio_suspend((const struct aiocb **)plist, 2, &ts); Useless cast. > - /* Check selected request has not completed */ > - if (received_selected) { > - printf (TNAME " Error : AIOCB %d should not have completed after timed out suspend\n", > - WAIT_FOR_AIOCB); > - for (i=0; i<NUM_AIOCBS; i++) > - free (aiocbs[i]); > - free (bufs); > - free (aiocbs); > - close (fd); > - exit (PTS_FAIL); > - } > - > /* timed out aio_suspend should return -1 and set errno to EAGAIN */ > - if (ret != -1) { > - printf (TNAME " aio_suspend() should return -1\n"); > + if (ret != -1 || errno != EAGAIN) { > + printf (TNAME " aio_suspend() should return -1 and set errno " > + "to EAGAIN: %d, %s\n", ret, strerror(errno)); > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); > @@ -200,9 +172,11 @@ main () > exit (PTS_FAIL); > } Spaces after free and exit. > - if (errno != EAGAIN) { > - printf (TNAME " aio_suspend() should set errno to EAGAIN: %d (%s)\n", > - errno, strerror (errno)); > + /* Check selected request has not completed */ > + err = aio_error(aiocbs[WAIT_FOR_AIOCB]); > + if (err != EINPROGRESS) { > + printf (TNAME " Error : AIOCB %d should not have completed " > + "after timed out suspend\n", WAIT_FOR_AIOCB); > for (i=0; i<NUM_AIOCBS; i++) > free (aiocbs[i]); > free (bufs); And here spaces after printf and free. -- Cyril Hrubis ch...@su... |
From: Bian N. <bi...@cn...> - 2010-11-19 02:09:43
|
Cyril Hrubis wrote: > Hi! >> aio_suspend may be interrupted by IO request's completion signal, so we >> should use aio_error instead signal hander to check whether this request >> has complete. >> >> Signed-off-by: Bian Naimeng <bi...@cn...> >> >> --- >> .../conformance/interfaces/aio_suspend/4-1.c | 62 ++++++-------------- >> 1 files changed, 18 insertions(+), 44 deletions(-) >> >> diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c >> index 260c26a..bb90234 100644 >> --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c >> +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/4-1.c >> @@ -41,19 +41,11 @@ >> #define BUF_SIZE 1024*1024 >> #define WAIT_FOR_AIOCB 6 >> >> -int received_selected = 0; >> int received_all = 0; >> >> void >> sigrt1_handler(int signum, siginfo_t *info, void *context) >> { >> - if (info->si_value.sival_int == WAIT_FOR_AIOCB) >> - received_selected = 1; >> -} >> - >> -void >> -sigrt2_handler(int signum, siginfo_t *info, void *context) >> -{ >> received_all = 1; >> } >> >> @@ -68,7 +60,7 @@ main () >> char *bufs; >> struct sigaction action; >> struct sigevent event; >> - struct timespec ts = {0, 10000000}; /* 10 ms */ >> + struct timespec ts = {0, 1000}; /* 1 us */ >> int errors = 0; >> int ret; >> int err; >> @@ -121,30 +113,19 @@ main () >> aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; >> aiocbs[i]->aio_nbytes = BUF_SIZE; >> aiocbs[i]->aio_lio_opcode = LIO_READ; >> - >> - /* Use SIRTMIN+1 for individual completions */ >> - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; >> - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; >> - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; >> } >> >> - /* Use SIGRTMIN+2 for list completion */ >> + /* Use SIGRTMIN+1 for list completion */ >> event.sigev_notify = SIGEV_SIGNAL; >> - event.sigev_signo = SIGRTMIN+2; >> + event.sigev_signo = SIGRTMIN+1; >> event.sigev_value.sival_ptr = NULL; >> >> - /* Setup handler for individual operation completion */ >> + /* Setup handler for list completion */ >> action.sa_sigaction = sigrt1_handler; >> sigemptyset(&action.sa_mask); >> action.sa_flags = SA_SIGINFO|SA_RESTART; >> sigaction(SIGRTMIN+1, &action, NULL); >> >> - /* Setup handler for list completion */ >> - action.sa_sigaction = sigrt2_handler; >> - sigemptyset(&action.sa_mask); >> - action.sa_flags = SA_SIGINFO|SA_RESTART; >> - sigaction(SIGRTMIN+2, &action, NULL); >> - >> /* Setup suspend list */ >> plist[0] = NULL; >> plist[1] = aiocbs[WAIT_FOR_AIOCB]; >> @@ -153,7 +134,8 @@ main () >> ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); >> >> 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)); >> for (i=0; i<NUM_AIOCBS; i++) >> free (aiocbs[i]); >> free (bufs); > > Once again, space here. Thanks again for your review. It's helpful to me. ^_^ > >> @@ -163,9 +145,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); >> } > > Spaces after printf and free. > > And the same problem with relying on AIO operation not finished yet. So > I propose to change this PTS_FAIL to PTS_UNRESOLVED. > >> @@ -177,21 +160,10 @@ main () >> /* Suspend on selected request */ >> ret = aio_suspend((const struct aiocb **)plist, 2, &ts); > > Useless cast. > >> - /* Check selected request has not completed */ >> - if (received_selected) { >> - printf (TNAME " Error : AIOCB %d should not have completed after timed out suspend\n", >> - WAIT_FOR_AIOCB); >> - for (i=0; i<NUM_AIOCBS; i++) >> - free (aiocbs[i]); >> - free (bufs); >> - free (aiocbs); >> - close (fd); >> - exit (PTS_FAIL); >> - } >> - >> /* timed out aio_suspend should return -1 and set errno to EAGAIN */ >> - if (ret != -1) { >> - printf (TNAME " aio_suspend() should return -1\n"); >> + if (ret != -1 || errno != EAGAIN) { >> + printf (TNAME " aio_suspend() should return -1 and set errno " >> + "to EAGAIN: %d, %s\n", ret, strerror(errno)); >> for (i=0; i<NUM_AIOCBS; i++) >> free (aiocbs[i]); >> free (bufs); >> @@ -200,9 +172,11 @@ main () >> exit (PTS_FAIL); >> } > > Spaces after free and exit. > >> - if (errno != EAGAIN) { >> - printf (TNAME " aio_suspend() should set errno to EAGAIN: %d (%s)\n", >> - errno, strerror (errno)); >> + /* Check selected request has not completed */ >> + err = aio_error(aiocbs[WAIT_FOR_AIOCB]); >> + if (err != EINPROGRESS) { >> + printf (TNAME " Error : AIOCB %d should not have completed " >> + "after timed out suspend\n", WAIT_FOR_AIOCB); >> for (i=0; i<NUM_AIOCBS; i++) >> free (aiocbs[i]); >> free (bufs); > > And here spaces after printf and free. > -- Regards Bian Naimeng |
From: Cyril H. <ch...@su...> - 2010-11-11 14:28:48
|
Hi! > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. This seems reasonable, as in the original code any aio_read could send the signal that could interrupt aio_suspend (and even completion of the aio request we are waiting for could interrupt it with signal). And as I quick looked at the code and there is more to fix: Bugs: * relying on write() writing the whole buffer * not checking malloc() return value Conding style issues: * bunch of useless pointer cast * spaces between function names and () (all free (..), malloc (...) ...) -- Cyril Hrubis ch...@su... |
From: Bian N. <bi...@cn...> - 2010-11-12 02:11:59
|
aio_suspend may be interrupted by IO request's completion signal, so we should use aio_error instead signal hander to check whether this request has complete. Signed-off-by: Bian Naimeng <bi...@cn...> --- .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- 1 files changed, 17 insertions(+), 34 deletions(-) diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c index 7d33e62..8c85ca5 100644 --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c @@ -44,19 +44,11 @@ #define BUF_SIZE 1024*1024 #define WAIT_FOR_AIOCB 6 -int received_selected = 0; int received_all = 0; void sigrt1_handler(int signum, siginfo_t *info, void *context) { - if (info->si_value.sival_int == WAIT_FOR_AIOCB) - received_selected = 1; -} - -void -sigrt2_handler(int signum, siginfo_t *info, void *context) -{ received_all = 1; } @@ -123,30 +115,19 @@ main () aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; aiocbs[i]->aio_nbytes = BUF_SIZE; aiocbs[i]->aio_lio_opcode = LIO_READ; - - /* Use SIRTMIN+1 for individual completions */ - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; } - /* Use SIGRTMIN+2 for list completion */ + /* Use SIGRTMIN+1 for list completion */ event.sigev_notify = SIGEV_SIGNAL; - event.sigev_signo = SIGRTMIN+2; + event.sigev_signo = SIGRTMIN+1; event.sigev_value.sival_ptr = NULL; - /* Setup handler for individual operation completion */ + /* Setup handler for list completion */ action.sa_sigaction = sigrt1_handler; sigemptyset(&action.sa_mask); action.sa_flags = SA_SIGINFO|SA_RESTART; sigaction(SIGRTMIN+1, &action, NULL); - /* Setup handler for list completion */ - action.sa_sigaction = sigrt2_handler; - sigemptyset(&action.sa_mask); - action.sa_flags = SA_SIGINFO|SA_RESTART; - sigaction(SIGRTMIN+2, &action, NULL); - /* Setup suspend list */ plist[0] = NULL; plist[1] = aiocbs[WAIT_FOR_AIOCB]; @@ -155,7 +136,8 @@ main () ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); 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)); 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); @@ -178,11 +161,9 @@ main () /* Suspend on selected request */ ret = aio_suspend((const struct aiocb **)plist, 2, NULL); - - /* 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); } - - 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); -- 1.7.0.4 |
From: Cyril H. <ch...@su...> - 2010-11-18 18:17:13
|
Hi! > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. > > Signed-off-by: Bian Naimeng <bi...@cn...> > > --- > .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- > 1 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > index 7d33e62..8c85ca5 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > @@ -44,19 +44,11 @@ > #define BUF_SIZE 1024*1024 > #define WAIT_FOR_AIOCB 6 > > -int received_selected = 0; > int received_all = 0; > > void > sigrt1_handler(int signum, siginfo_t *info, void *context) > { > - if (info->si_value.sival_int == WAIT_FOR_AIOCB) > - received_selected = 1; > -} > - > -void > -sigrt2_handler(int signum, siginfo_t *info, void *context) > -{ > received_all = 1; > } > > @@ -123,30 +115,19 @@ main () > aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; > aiocbs[i]->aio_nbytes = BUF_SIZE; > aiocbs[i]->aio_lio_opcode = LIO_READ; > - > - /* Use SIRTMIN+1 for individual completions */ > - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; > - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; > - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; > } > > - /* Use SIGRTMIN+2 for list completion */ > + /* Use SIGRTMIN+1 for list completion */ > event.sigev_notify = SIGEV_SIGNAL; > - event.sigev_signo = SIGRTMIN+2; > + event.sigev_signo = SIGRTMIN+1; > event.sigev_value.sival_ptr = NULL; > > - /* Setup handler for individual operation completion */ > + /* Setup handler for list completion */ > action.sa_sigaction = sigrt1_handler; > sigemptyset(&action.sa_mask); > action.sa_flags = SA_SIGINFO|SA_RESTART; > sigaction(SIGRTMIN+1, &action, NULL); > > - /* Setup handler for list completion */ > - action.sa_sigaction = sigrt2_handler; > - sigemptyset(&action.sa_mask); > - action.sa_flags = SA_SIGINFO|SA_RESTART; > - sigaction(SIGRTMIN+2, &action, NULL); > - > /* Setup suspend list */ > plist[0] = NULL; > plist[1] = aiocbs[WAIT_FOR_AIOCB]; > @@ -155,7 +136,8 @@ main () > ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); > > 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. > 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? > @@ -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**) > - > - /* 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. -- Cyril Hrubis ch...@su... |
From: Bian N. <bi...@cn...> - 2010-11-19 02:06:05
|
Cyril Hrubis wrote: > Hi! >> aio_suspend may be interrupted by IO request's completion signal, so we >> should use aio_error instead signal hander to check whether this request >> has complete. >> >> Signed-off-by: Bian Naimeng <bi...@cn...> >> >> --- >> .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- >> 1 files changed, 17 insertions(+), 34 deletions(-) >> >> diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> index 7d33e62..8c85ca5 100644 >> --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> @@ -44,19 +44,11 @@ >> #define BUF_SIZE 1024*1024 >> #define WAIT_FOR_AIOCB 6 >> >> -int received_selected = 0; >> int received_all = 0; >> >> void >> sigrt1_handler(int signum, siginfo_t *info, void *context) >> { >> - if (info->si_value.sival_int == WAIT_FOR_AIOCB) >> - received_selected = 1; >> -} >> - >> -void >> -sigrt2_handler(int signum, siginfo_t *info, void *context) >> -{ >> received_all = 1; >> } >> >> @@ -123,30 +115,19 @@ main () >> aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; >> aiocbs[i]->aio_nbytes = BUF_SIZE; >> aiocbs[i]->aio_lio_opcode = LIO_READ; >> - >> - /* Use SIRTMIN+1 for individual completions */ >> - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; >> - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; >> - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; >> } >> >> - /* Use SIGRTMIN+2 for list completion */ >> + /* Use SIGRTMIN+1 for list completion */ >> event.sigev_notify = SIGEV_SIGNAL; >> - event.sigev_signo = SIGRTMIN+2; >> + event.sigev_signo = SIGRTMIN+1; >> event.sigev_value.sival_ptr = NULL; >> >> - /* Setup handler for individual operation completion */ >> + /* Setup handler for list completion */ >> action.sa_sigaction = sigrt1_handler; >> sigemptyset(&action.sa_mask); >> action.sa_flags = SA_SIGINFO|SA_RESTART; >> sigaction(SIGRTMIN+1, &action, NULL); >> >> - /* Setup handler for list completion */ >> - action.sa_sigaction = sigrt2_handler; >> - sigemptyset(&action.sa_mask); >> - action.sa_flags = SA_SIGINFO|SA_RESTART; >> - sigaction(SIGRTMIN+2, &action, NULL); >> - >> /* Setup suspend list */ >> plist[0] = NULL; >> plist[1] = aiocbs[WAIT_FOR_AIOCB]; >> @@ -155,7 +136,8 @@ main () >> ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); >> >> 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. > >> 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? > I do not think so, all tests are according to POSIX spec, we should not change it to fit linux, right? However, i think there are three possibility cause aio request has completed. 1. Some error occured, in this case, we should return PTS_FAIL. 2. Everything is ok, but aio request has completed too fast, in this case, we should return PTS_UNRESOLVED or PTS_UNTESTED. 3. OS is not support aio request like you said, we should return PTS_UNSUPPORTED. So, maybe PT_UNRESOLVED is ok by this viewpoint. Then, Garrett and Cyril, what's your opinion. Thanks Bian > >> @@ -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**) > >> - >> - /* 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. > -- Regards Bian Naimeng |
From: Garrett C. <yan...@gm...> - 2010-11-21 00:13:26
|
On Thu, Nov 18, 2010 at 10:29 AM, Cyril Hrubis <ch...@su...> wrote: > Hi! >> aio_suspend may be interrupted by IO request's completion signal, so we >> should use aio_error instead signal hander to check whether this request >> has complete. >> >> Signed-off-by: Bian Naimeng <bi...@cn...> >> >> --- >> .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- >> 1 files changed, 17 insertions(+), 34 deletions(-) >> >> diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> index 7d33e62..8c85ca5 100644 >> --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >> @@ -44,19 +44,11 @@ >> #define BUF_SIZE 1024*1024 >> #define WAIT_FOR_AIOCB 6 >> >> -int received_selected = 0; >> int received_all = 0; >> >> void >> sigrt1_handler(int signum, siginfo_t *info, void *context) >> { >> - if (info->si_value.sival_int == WAIT_FOR_AIOCB) >> - received_selected = 1; >> -} >> - >> -void >> -sigrt2_handler(int signum, siginfo_t *info, void *context) >> -{ >> received_all = 1; >> } >> >> @@ -123,30 +115,19 @@ main () >> aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; >> aiocbs[i]->aio_nbytes = BUF_SIZE; >> aiocbs[i]->aio_lio_opcode = LIO_READ; >> - >> - /* Use SIRTMIN+1 for individual completions */ >> - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; >> - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; >> - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; >> } >> >> - /* Use SIGRTMIN+2 for list completion */ >> + /* Use SIGRTMIN+1 for list completion */ >> event.sigev_notify = SIGEV_SIGNAL; >> - event.sigev_signo = SIGRTMIN+2; >> + event.sigev_signo = SIGRTMIN+1; >> event.sigev_value.sival_ptr = NULL; >> >> - /* Setup handler for individual operation completion */ >> + /* Setup handler for list completion */ >> action.sa_sigaction = sigrt1_handler; >> sigemptyset(&action.sa_mask); >> action.sa_flags = SA_SIGINFO|SA_RESTART; >> sigaction(SIGRTMIN+1, &action, NULL); >> >> - /* Setup handler for list completion */ >> - action.sa_sigaction = sigrt2_handler; >> - sigemptyset(&action.sa_mask); >> - action.sa_flags = SA_SIGINFO|SA_RESTART; >> - sigaction(SIGRTMIN+2, &action, NULL); >> - >> /* Setup suspend list */ >> plist[0] = NULL; >> plist[1] = aiocbs[WAIT_FOR_AIOCB]; >> @@ -155,7 +136,8 @@ main () >> ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); >> >> 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). >> 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. >> @@ -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. >> - >> - /* 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. Thanks, -Garrett |
From: Bian N. <bi...@cn...> - 2010-11-22 02:28:49
|
Garrett Cooper wrote: > On Thu, Nov 18, 2010 at 10:29 AM, Cyril Hrubis <ch...@su...> wrote: >> Hi! >>> aio_suspend may be interrupted by IO request's completion signal, so we >>> should use aio_error instead signal hander to check whether this request >>> has complete. >>> >>> Signed-off-by: Bian Naimeng <bi...@cn...> >>> >>> --- >>> .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- >>> 1 files changed, 17 insertions(+), 34 deletions(-) >>> >>> diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >>> index 7d33e62..8c85ca5 100644 >>> --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >>> +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c >>> @@ -44,19 +44,11 @@ >>> #define BUF_SIZE 1024*1024 >>> #define WAIT_FOR_AIOCB 6 >>> >>> -int received_selected = 0; >>> int received_all = 0; >>> >>> void >>> sigrt1_handler(int signum, siginfo_t *info, void *context) >>> { >>> - if (info->si_value.sival_int == WAIT_FOR_AIOCB) >>> - received_selected = 1; >>> -} >>> - ... snip ... >>> /* Setup suspend list */ >>> plist[0] = NULL; >>> plist[1] = aiocbs[WAIT_FOR_AIOCB]; >>> @@ -155,7 +136,8 @@ main () >>> ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); >>> >>> 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). > >>> 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. > >>> @@ -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. > >>> - >>> - /* 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. En..., this test is still fail, what's your opinion about this patch? Thanks Bian > > Thanks, > -Garrett > |
From: Cyril H. <ch...@su...> - 2010-11-25 19:12:02
|
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 ch...@su... |
From: Garrett C. <yan...@gm...> - 2010-11-25 19:26:06
|
On Thu, Nov 25, 2010 at 11:24 AM, Cyril Hrubis <ch...@su...> 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 ;). :). |
From: Bian N. <bi...@cn...> - 2010-11-26 01:49:34
|
Garrett Cooper wrote: > On Thu, Nov 25, 2010 at 11:24 AM, Cyril Hrubis <ch...@su...> 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 |
From: Garrett C. <yan...@gm...> - 2010-11-25 21:10:09
|
On Thu, Nov 25, 2010 at 11:24 AM, Cyril Hrubis <ch...@su...> wrote: > Hi! ... > Out of curiosity, what OS/gcc version does this? I have no warnings with > all the casts removed from 1-1.c. $ uname -a FreeBSD bayonetta.local 9.0-CURRENT FreeBSD 9.0-CURRENT #0 r215166M: Mon Nov 15 09:04:32 PST 2010 root@bayonetta.local:/usr/obj/usr/src/sys/BAYONETTA amd64 $ gcc --version gcc (GCC) 4.2.1 20070719 [FreeBSD] Copyright (C) 2007 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Thanks, -Garrett |
From: Bian N. <bi...@cn...> - 2011-03-13 02:14:55
|
Bian Naimeng wrote: > aio_suspend may be interrupted by IO request's completion signal, so we > should use aio_error instead signal hander to check whether this request > has complete. Ping, would you have time to pick up it. Regards Bian > > Signed-off-by: Bian Naimeng <bi...@cn...> > > --- > .../conformance/interfaces/aio_suspend/1-1.c | 51 +++++++------------- > 1 files changed, 17 insertions(+), 34 deletions(-) > > diff --git a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > index 7d33e62..8c85ca5 100644 > --- a/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > +++ b/testcases/open_posix_testsuite/conformance/interfaces/aio_suspend/1-1.c > @@ -44,19 +44,11 @@ > #define BUF_SIZE 1024*1024 > #define WAIT_FOR_AIOCB 6 > > -int received_selected = 0; > int received_all = 0; > > void > sigrt1_handler(int signum, siginfo_t *info, void *context) > { > - if (info->si_value.sival_int == WAIT_FOR_AIOCB) > - received_selected = 1; > -} > - > -void > -sigrt2_handler(int signum, siginfo_t *info, void *context) > -{ > received_all = 1; > } > > @@ -123,30 +115,19 @@ main () > aiocbs[i]->aio_buf = &bufs[i*BUF_SIZE]; > aiocbs[i]->aio_nbytes = BUF_SIZE; > aiocbs[i]->aio_lio_opcode = LIO_READ; > - > - /* Use SIRTMIN+1 for individual completions */ > - aiocbs[i]->aio_sigevent.sigev_notify = SIGEV_SIGNAL; > - aiocbs[i]->aio_sigevent.sigev_signo = SIGRTMIN+1; > - aiocbs[i]->aio_sigevent.sigev_value.sival_int = i; > } > > - /* Use SIGRTMIN+2 for list completion */ > + /* Use SIGRTMIN+1 for list completion */ > event.sigev_notify = SIGEV_SIGNAL; > - event.sigev_signo = SIGRTMIN+2; > + event.sigev_signo = SIGRTMIN+1; > event.sigev_value.sival_ptr = NULL; > > - /* Setup handler for individual operation completion */ > + /* Setup handler for list completion */ > action.sa_sigaction = sigrt1_handler; > sigemptyset(&action.sa_mask); > action.sa_flags = SA_SIGINFO|SA_RESTART; > sigaction(SIGRTMIN+1, &action, NULL); > > - /* Setup handler for list completion */ > - action.sa_sigaction = sigrt2_handler; > - sigemptyset(&action.sa_mask); > - action.sa_flags = SA_SIGINFO|SA_RESTART; > - sigaction(SIGRTMIN+2, &action, NULL); > - > /* Setup suspend list */ > plist[0] = NULL; > plist[1] = aiocbs[WAIT_FOR_AIOCB]; > @@ -155,7 +136,8 @@ main () > ret = lio_listio(LIO_NOWAIT, aiocbs, NUM_AIOCBS, &event); > > 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)); > 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); > @@ -178,11 +161,9 @@ main () > > /* Suspend on selected request */ > ret = aio_suspend((const struct aiocb **)plist, 2, NULL); > - > - /* 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); > } > > - > - 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); |