From: "Bird, Timothy" <Tim.Bird@sony.com>
To: Paul Elder <paul.elder@pitt.edu>, Shuah Khan <shuahkh@osg.samsung.com>,
        "shuah@kernel.org" <shuah@kernel.org>
CC: "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] selftests: kcmp: convert to TAP13 output
Thread-Topic: [PATCH v2] selftests: kcmp: convert to TAP13 output
Thread-Index: AQHS8hGqnDTDGdwmKUeiV02S8XYIpKI+QjZw
Date: Sat, 1 Jul 2017 03:00:34 +0000
Message-ID: <ECADFF3FD767C149AD96A924E7EA6EAF2E263F2E@USCULXMSG03.am.sony.com>
References: <20170630234758.348-1-shuahkh@osg.samsung.com>
 <9dc7074d-9c7e-0b64-a41a-2f4ceebd0761@pitt.edu>
In-Reply-To: <9dc7074d-9c7e-0b64-a41a-2f4ceebd0761@pitt.edu>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jul 2017 03:00:37.5233
 (UTC)
X-MS-Exchange-CrossTenant-Id: 66c65d8a-9158-4521-a2d8-664963db48e4
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=66c65d8a-9158-4521-a2d8-664963db48e4;Ip=[160.33.194.228];Helo=[usculsndmail01v.am.sony.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1301MB2027
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v6131SsC017107
Content-Length: 3848
Lines: 119


> -----Original Message-----
> From: Paul Elder on Friday, June 30, 2017 7:28 PM
> On 07/01/2017 08:47 AM, Shuah Khan wrote:
> > Convert to TAP13 output using ksft_ api. Child runs tests, increments test
> > counters, and prints test results.
> >
> > Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> > ---
> >
> > Changes since v1:
> > - Add ksft_print_header()
> >
> >  tools/testing/selftests/kcmp/kcmp_test.c | 48 +++++++++++++-------------
> ------
> >  1 file changed, 19 insertions(+), 29 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kcmp/kcmp_test.c
> b/tools/testing/selftests/kcmp/kcmp_test.c
> > index a5a4da856dfe..563018d81c45 100644
> > --- a/tools/testing/selftests/kcmp/kcmp_test.c
> > +++ b/tools/testing/selftests/kcmp/kcmp_test.c
> > @@ -34,16 +34,14 @@ int main(int argc, char **argv)
> >  	fd1 = open(kpath, O_RDWR | O_CREAT | O_TRUNC, 0644);
> >  	pid1 = getpid();
> >
> > -	if (fd1 < 0) {
> > -		perror("Can't create file");
> > -		ksft_exit_fail();
> > -	}
> > +	ksft_print_header();
> > +
> > +	if (fd1 < 0)
> > +		ksft_exit_fail_msg("Can't create file: %s\n", strerror(errno));
> >
> >  	pid2 = fork();
> > -	if (pid2 < 0) {
> > -		perror("fork failed");
> > -		ksft_exit_fail();
> > -	}
> > +	if (pid2 < 0)
> > +		ksft_exit_fail_msg("fork() failed: %s\n", strerror(errno));
> >
> >  	if (!pid2) {
> >  		int pid2 = getpid();
> > @@ -51,14 +49,14 @@ int main(int argc, char **argv)
> >
> >  		fd2 = open(kpath, O_RDWR, 0644);
> >  		if (fd2 < 0) {
> > -			perror("Can't open file");
> > -			ksft_exit_fail();
> > +			ksft_print_msg("Can't open file: %s\n",
> > +				strerror(errno));
> > +			exit(KSFT_FAIL);
> >  		}
> >
> >  		/* An example of output and arguments */
> > -		printf("pid1: %6d pid2: %6d FD: %2ld FILES: %2ld VM: %2ld "
> > -		       "FS: %2ld SIGHAND: %2ld IO: %2ld SYSVSEM: %2ld "
> > -		       "INV: %2ld\n",
> > +		ksft_print_msg(
> > +			"pid1: %6d pid2: %6d FD: %2ld\n  FILES: %2ld VM:
> %2ld FS: %2ld SIGHAND: %2ld\n  IO: %2ld SYSVSEM: %2ld INV: %2ld\n",
> Is it okay that there's no # after the newlines? Will that confuse test output
> parsers?

Probably.  I envisioned ksft_print_msg() as parsing the string for newlines,
and replacing them with: "\n# ", to work around this problem.
I can code something up if desired.

> 
> >  		       pid1, pid2,
> >  		       sys_kcmp(pid1, pid2, KCMP_FILE,		fd1, fd2),
> >  		       sys_kcmp(pid1, pid2, KCMP_FILES,		0, 0),
> > @@ -74,28 +72,22 @@ int main(int argc, char **argv)
> >  		/* This one should return same fd */
> >  		ret = sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd1);
> >  		if (ret) {
> > -			printf("FAIL: 0 expected but %d returned (%s)\n",
> > +			ksft_test_result_fail(
> > +				"0 expected but %d returned (%s)\n",
> >  				ret, strerror(errno));
> > -			ksft_inc_fail_cnt();
> >  			ret = -1;
> > -		} else {
> > -			printf("PASS: 0 returned as expected\n");
> > -			ksft_inc_pass_cnt();
> > -		}
> > +		} else
> > +			ksft_test_result_pass("0 returned as expected\n");
> I remember Tim Bird mentioning before that the test descriptions should be
> non-dynamic to not confuse diffs. What did we decide on about that?
> 
> Also specifically with this test output (seems like Tim had a similar
> comment before), the output doesn't really describe the test.

Yeah - I don't like this as a test description.

I would restructure this as:

+ const char *test_name = "kcmp with KCMP_FILE"
...
-			printf("FAIL: 0 expected but %d returned (%s)\n",
+			ksft_test_result_fail(test_name);
+			ksft_print_msg("0 expected but %d returned (%s)\n",
+  				ret, strerror(errno));
...
- (minus stuff)
+		} else
+			ksft_test_result_pass(test_name);

And do a similar re-structuring, using test_name, and adding
diagnostic information only on failure (with ksft_print_msg),
with:
const char *test_name = "kcmp with KCMP_VM"

 -- Tim