[LTP] [POSIX][PATCH 0/2]aio_suspend may be interrupted by completion signal.

[LTP] [POSIX][PATCH 0/2]aio_suspend may be interrupted by completion signal.

[Bian Naimeng]

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 <biannm@cn.fujitsu.com>

-----

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


------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Bian Naimeng]

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 <biannm@cn.fujitsu.com>

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




------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Cyril Hrubis]

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 <biannm@cn.fujitsu.com>
> 
> ---
>  .../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
chrubis@suse.cz

------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Cyril Hrubis]

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 <biannm@cn.fujitsu.com>
> 
> ---
>  .../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
chrubis@suse.cz

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today
http://p.sf.net/sfu/msIE9-sfdev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Cyril Hrubis]

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.
http://p.sf.net/sfu/intelisp-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Bian Naimeng]

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 <biannm@cn.fujitsu.com>

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




------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Cyril Hrubis]

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 <biannm@cn.fujitsu.com>
> 
> ---
>  .../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
chrubis@suse.cz

------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

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

[Cyril Hrubis]

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 <biannm@cn.fujitsu.com>
> 
> ---
>  .../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
chrubis@suse.cz

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today
http://p.sf.net/sfu/msIE9-sfdev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [POSIX][PATCH 0/2]aio_suspend may be interrupted by completion signal.

[Cyril Hrubis]

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
chrubis@suse.cz

------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list