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

[Mike Frysinger <vapier@gentoo.org>]

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