[LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Chandru]

The ppoll01 testcase has a signal handler in it but this signal handler is not registered with the kernel through the signal() syscal. The testcase fails when it sends a SIGINT to itself as part of one of the case of the test run.
The following patch
1. Adds a signal() syscall to register the signal handler
2. Corrects the expected 'revents' for the case where ppoll() is called on a file which is only opened in read/write mode, i.e case00.
3. Takes away printing the type of error ( i.e strerror(errno) ) since it only prints the last error seen and does not report the failures seen in the cases before the last failed one. The reporting of NG or OK shoudl take care of 
    letting the user know the cases that have passed and cases that have failed.

Signed-off-by: Chandru S <chandru@linux.vnet.ibm.com>
---

--- ppoll01.c.orig	2009-09-22 19:22:39.371235396 +0530
+++ ppoll01.c	2009-09-24 01:24:38.706487228 +0530
@@ -106,6 +106,8 @@ extern void cleanup() {
         tst_exit();
 }
 
+void sighandler(int sig);       /* signals handler function for the test */
+
 /* Local  Functions */
 /******************************************************************************/
 /*                                                                            */
@@ -125,7 +127,8 @@ extern void cleanup() {
 /*                                                                            */
 /******************************************************************************/
 void setup() {
-        /* Capture signals if any */
+        /* Capture signals*/
+	signal(SIGINT, &sighandler);
         /* Create temporary directories */
         TEST_PAUSE;
         tst_tmpdir();
@@ -191,7 +194,7 @@ struct test_case {
 static struct test_case tcase[] = {
 	{ // case00
                 .ttype          = NORMAL,
-                .expect_revents = POLLOUT,
+                .expect_revents = POLLOUT | POLLIN,
                 .ret            = 0,
                 .err            = 0,
         },
@@ -463,7 +466,7 @@ int main(int ac, char **av) {
 		                break;
 
 	        default:
-                 	   	tst_resm(TFAIL, "%s failed - errno = %d : %s", TCID, TEST_ERRNO, strerror(TEST_ERRNO));
+                 	   	tst_resm(TFAIL, "%s failed", TCID);
         		        RPRINTF("NG");
 				cleanup();
 				tst_exit();



------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[K.D. Lucas]

[-- Attachment #1.1: Type: text/plain, Size: 3214 bytes --]

I applied this patch, and it fixed ppoll01 on an Ubuntu Hardy box which had
been failing. Kernel is 2.6.24.

Kelly

On Wed, Sep 23, 2009 at 11:59 PM, Chandru <chandru@in.ibm.com> wrote:

> The ppoll01 testcase has a signal handler in it but this signal handler is
> not registered with the kernel through the signal() syscal. The testcase
> fails when it sends a SIGINT to itself as part of one of the case of the
> test run.
> The following patch
> 1. Adds a signal() syscall to register the signal handler
> 2. Corrects the expected 'revents' for the case where ppoll() is called on
> a file which is only opened in read/write mode, i.e case00.
> 3. Takes away printing the type of error ( i.e strerror(errno) ) since it
> only prints the last error seen and does not report the failures seen in the
> cases before the last failed one. The reporting of NG or OK shoudl take care
> of
>    letting the user know the cases that have passed and cases that have
> failed.
>
> Signed-off-by: Chandru S <chandru@linux.vnet.ibm.com>
> ---
>
> --- ppoll01.c.orig      2009-09-22 19:22:39.371235396 +0530
> +++ ppoll01.c   2009-09-24 01:24:38.706487228 +0530
> @@ -106,6 +106,8 @@ extern void cleanup() {
>         tst_exit();
>  }
>
> +void sighandler(int sig);       /* signals handler function for the test
> */
> +
>  /* Local  Functions */
>
>  /******************************************************************************/
>  /*
>    */
> @@ -125,7 +127,8 @@ extern void cleanup() {
>  /*
>    */
>
>  /******************************************************************************/
>  void setup() {
> -        /* Capture signals if any */
> +        /* Capture signals*/
> +       signal(SIGINT, &sighandler);
>         /* Create temporary directories */
>         TEST_PAUSE;
>         tst_tmpdir();
> @@ -191,7 +194,7 @@ struct test_case {
>  static struct test_case tcase[] = {
>        { // case00
>                 .ttype          = NORMAL,
> -                .expect_revents = POLLOUT,
> +                .expect_revents = POLLOUT | POLLIN,
>                 .ret            = 0,
>                 .err            = 0,
>         },
> @@ -463,7 +466,7 @@ int main(int ac, char **av) {
>                                break;
>
>                default:
> -                               tst_resm(TFAIL, "%s failed - errno = %d :
> %s", TCID, TEST_ERRNO, strerror(TEST_ERRNO));
> +                               tst_resm(TFAIL, "%s failed", TCID);
>                                RPRINTF("NG");
>                                cleanup();
>                                tst_exit();
>
>
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Ltp-list mailing list
> Ltp-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ltp-list
>



-- 
K.D. Lucas
kdlucas@gmail.com

[-- Attachment #1.2: Type: text/html, Size: 4169 bytes --]

[-- Attachment #2: Type: text/plain, Size: 401 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Garrett Cooper]

On Thu, Sep 24, 2009 at 1:28 PM, K.D. Lucas <kdlucas@gmail.com> wrote:
> I applied this patch, and it fixed ppoll01 on an Ubuntu Hardy box which had
> been failing. Kernel is 2.6.24.
>
> Kelly
>
> On Wed, Sep 23, 2009 at 11:59 PM, Chandru <chandru@in.ibm.com> wrote:
>>
>> The ppoll01 testcase has a signal handler in it but this signal handler is
>> not registered with the kernel through the signal() syscal. The testcase
>> fails when it sends a SIGINT to itself as part of one of the case of the
>> test run.
>> The following patch
>> 1. Adds a signal() syscall to register the signal handler
>> 2. Corrects the expected 'revents' for the case where ppoll() is called on
>> a file which is only opened in read/write mode, i.e case00.
>> 3. Takes away printing the type of error ( i.e strerror(errno) ) since it
>> only prints the last error seen and does not report the failures seen in the
>> cases before the last failed one. The reporting of NG or OK shoudl take care
>> of
>>    letting the user know the cases that have passed and cases that have
>> failed.
>>
>> Signed-off-by: Chandru S <chandru@linux.vnet.ibm.com>

...

>>                default:
>> -                               tst_resm(TFAIL, "%s failed - errno = %d :
>> %s", TCID, TEST_ERRNO, strerror(TEST_ERRNO));
>> +                               tst_resm(TFAIL, "%s failed", TCID);
>>                                RPRINTF("NG");
>>                                cleanup();
>>                                tst_exit();

Please don't change that line.
Thanks,
-Garrett

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 737 bytes --]

On Thursday 24 September 2009 02:59:32 Chandru wrote:
>  3. Takes away
>  printing the type of error ( i.e strerror(errno) ) since it only prints
>  the last error seen and does not report the failures seen in the cases
>  before the last failed one. The reporting of NG or OK shoudl take care of
>  letting the user know the cases that have passed and cases that have
>  failed.
>
> -                 	   	tst_resm(TFAIL, "%s failed - errno = %d : %s", TCID,
>  TEST_ERRNO, strerror(TEST_ERRNO));
>  +                 	   	tst_resm(TFAIL, "%s failed", TCID);

why dont you *fix* the issues you see with this output instead of making the 
output even more useless ?  also, this should be converted to TFAIL|TTERRNO.
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 401 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Chandru]

On Saturday 26 September 2009 16:55:28 Mike Frysinger wrote:
> On Thursday 24 September 2009 02:59:32 Chandru wrote:
> >  3. Takes away
> >  printing the type of error ( i.e strerror(errno) ) since it only prints
> >  the last error seen and does not report the failures seen in the cases
> >  before the last failed one. The reporting of NG or OK shoudl take care of
> >  letting the user know the cases that have passed and cases that have
> >  failed.
> >
> > -                 	   	tst_resm(TFAIL, "%s failed - errno = %d : %s", TCID,
> >  TEST_ERRNO, strerror(TEST_ERRNO));
> >  +                 	   	tst_resm(TFAIL, "%s failed", TCID);
> 
> why dont you *fix* the issues you see with this output instead of making the 
> output even more useless ?  also, this should be converted to TFAIL|TTERRNO.
> -mike
> 

em, the change was not useless, the existing reporting of failure was. The output on failure of only the
first case prints as
"ppoll01     1  TFAIL  :  ppoll01 failed - errno = 0 : Success"

The words 'failed' and 'Success' on the same line could confuse us. The testcase has 8 scenarios of which 5 are
made to fail by passing invalid arguments. Although we could have a fix as the following

=====================================================================

--- ppoll01.c.orig	2009-09-29 21:28:55.510487735 +0530
+++ ppoll01.c	2009-10-01 17:34:42.651235797 +0530
@@ -242,6 +242,8 @@ static struct test_case tcase[] = {
 #endif
 };
 
+int tstatus, terrno[16];
+
 #define NUM_TEST_FDS    1
 
 /*
@@ -265,6 +267,7 @@ static int do_test(struct test_case *tc)
         sigset_t *p_sigmask, sigmask;
         size_t sigsetsize = 0;
         pid_t pid = 0;
+	static int tc_no;
 
 	TEST(fd = setup_file(progdir, "test.file", fpath));
         if (fd < 0)
@@ -331,6 +334,11 @@ static int do_test(struct test_case *tc)
         errno = 0;
         TEST(sys_ret = syscall(__NR_ppoll, p_fds, nfds, p_ts, p_sigmask, sigsetsize));
         sys_errno = errno;
+	if (((tc->ret == 0) && (sys_ret == -1)) || ((tc->ret == -1) && (sys_ret == 0))) {
+		tstatus |= (1 << tc_no);
+		terrno[tc_no] = errno;
+	}
+	tc_no++;
         if (sys_ret <= 0 || tc->ret < 0)
                 goto TEST_END;
 
@@ -463,7 +471,10 @@ int main(int ac, char **av) {
 		                break;
 
 	        default:
-                 	   	tst_resm(TFAIL, "%s failed - errno = %d : %s", TCID, TEST_ERRNO, strerror(TEST_ERRNO));
+        			tst_resm(TPASS, "ppoll result = %d  ",result);
+                                for (i = 0; i < (int)(sizeof(tcase) / sizeof(tcase[0])); i++)
+                                        if (tstatus & (1 << i))
+                                                tst_resm(TFAIL|TTERRNO, "%s failed - errno = %d : %s", TCID, terrno[i], strerror(terrno[i]));
         		        RPRINTF("NG");
 				cleanup();
 				tst_exit();



======================================================================

I will retain the current existing tst_resm lines and add TFAIL|TTERRNO to it as per your suggestion. The reason being that 
there is not much gain in making the above changes to the test case.

Thanks,
Chandru




------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Chandru]

Chandru wrote:
> I will retain the current existing tst_resm lines and add TFAIL|TTERRNO to it as per your suggestion. The reason being that 
> there is not much gain in making the above changes to the test case.
Sorry, did not change that line at all.

Thanks
Chandru

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] ppoll01: add signal() syscall to register the signal handler

[Mike Frysinger]

[-- Attachment #1.1: Type: Text/Plain, Size: 3090 bytes --]

On Thursday 01 October 2009 03:27:46 Chandru wrote:
> On Saturday 26 September 2009 16:55:28 Mike Frysinger wrote:
> > On Thursday 24 September 2009 02:59:32 Chandru wrote:
> > >  3. Takes away
> > >  printing the type of error ( i.e strerror(errno) ) since it only
> > > prints the last error seen and does not report the failures seen in the
> > > cases before the last failed one. The reporting of NG or OK shoudl take
> > > care of letting the user know the cases that have passed and cases that
> > > have failed.
> > >
> > > -                 	   	tst_resm(TFAIL, "%s failed - errno = %d : %s",
> > > TCID, TEST_ERRNO, strerror(TEST_ERRNO));
> > >  +                 	   	tst_resm(TFAIL, "%s failed", TCID);
> >
> > why dont you *fix* the issues you see with this output instead of making
> > the output even more useless ?  also, this should be converted to
> > TFAIL|TTERRNO.
> 
> em, the change was not useless, the existing reporting of failure was. The
>  output on failure of only the first case prints as
> "ppoll01     1  TFAIL  :  ppoll01 failed - errno = 0 : Success"

you've shown problems in one case, not every single one.  at any rate, you've 
taken a stab at fixing the problem properly, so arguing semantics here is a 
waste of time.
 
> --- ppoll01.c.orig	2009-09-29 21:28:55.510487735 +0530
> +++ ppoll01.c	2009-10-01 17:34:42.651235797 +0530
> @@ -242,6 +242,8 @@ static struct test_case tcase[] = {
>  #endif
>  };
> 
> +int tstatus, terrno[16];

declare terrno static and then everything is initialized to 0.  then you dont 
need to track things in tstatus as non-zero should indicate an error.

hard coding 16 seems like a bad idea ... isnt there already a define/variable 
that holds the test count that you can use ?

> @@ -331,6 +334,11 @@ static int do_test(struct test_case *tc)
>          errno = 0;
>          TEST(sys_ret = syscall(__NR_ppoll, p_fds, nfds, p_ts, p_sigmask,
>  sigsetsize));
>  sys_errno = errno;
> +	if (((tc->ret == 0) && (sys_ret == -1)) || ((tc->ret == -1) && (sys_ret
>  == 0))) {
>  +		tstatus |= (1 << tc_no);
> +		terrno[tc_no] = errno;
> +	}

looks like this test was written without really understanding the LTP 
framework.  setting errno to 0 and the use of sys_errno is duplicating stuff 
LTP is already doing.  the code should be using TEST_ERRNO to get at the errno 
from the TEST() call.  this applies both to existing code and your new code.

> - tst_resm(TFAIL, "%s failed - errno = %d : %s", TCID, TEST_ERRNO,
> 			strerror(TEST_ERRNO));
> + tst_resm(TPASS, "ppoll result = %d  ",result);

how can it TPASS and then turn around and TFAIL ?  this new tst_resm() should 
just be dropped i think.

> + for (i = 0; i < (int)(sizeof(tcase) / sizeof(tcase[0])); i++)
>  +  if (tstatus & (1 << i))
> +     tst_resm(TFAIL|TTERRNO, "%s failed - errno = %d : %s", TCID,
>		 terrno[i], strerror(terrno[i]));

this isnt how TTERRNO works.  the code should be:
if (terrno[i]) {
	TEST_ERRNO = terrno[i];
	tst_resm(TFAIL|TTERNO, "test %i failed\n", i);
}
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 401 bytes --]

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf

[-- Attachment #3: Type: text/plain, Size: 155 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list