[LTP] [PATCH 0/2] syscalls/tkill: Convert tkill{01, 02} to the new API

[LTP] [PATCH 0/2] syscalls/tkill: Convert tkill{01, 02} to the new API

[Xie Ziyao]

1. Capture signals to verify success in tkill01.c, while the previous code
didn't make it work;
2. Cleanup and convert tkill{01, 02} to the new API.

Xie Ziyao (2):
  syscalls/tkill: Convert tkill01 to the new API
  syscalls/tkill: Convert tkill02 to the new API

 testcases/kernel/syscalls/tkill/tkill01.c | 122 +++++++---------------
 testcases/kernel/syscalls/tkill/tkill02.c | 105 ++++++-------------
 2 files changed, 70 insertions(+), 157 deletions(-)

--
2.17.1

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[Xie Ziyao]

1. Convert tkill01 to the new API;
2. Capture signals to verify success, while the previous code
didn't make it work.

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 testcases/kernel/syscalls/tkill/tkill01.c | 122 +++++++---------------
 1 file changed, 38 insertions(+), 84 deletions(-)

diff --git a/testcases/kernel/syscalls/tkill/tkill01.c b/testcases/kernel/syscalls/tkill/tkill01.c
index 20c28f1bc..094b0d942 100644
--- a/testcases/kernel/syscalls/tkill/tkill01.c
+++ b/testcases/kernel/syscalls/tkill/tkill01.c
@@ -1,42 +1,18 @@
-/******************************************************************************/
-/* Copyright (c) Crackerjack Project., 2007				   */
-/*									    */
-/* This program is free software;  you can redistribute it and/or modify      */
-/* it under the terms of the GNU General Public License as published by       */
-/* the Free Software Foundation; either version 2 of the License, or	  */
-/* (at your option) any later version.					*/
-/*									    */
-/* This program is distributed in the hope that it will be useful,	    */
-/* but WITHOUT ANY WARRANTY;  without even the implied warranty of	    */
-/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See		  */
-/* the GNU General Public License for more details.			   */
-/*									    */
-/* You should have received a copy of the GNU General Public License	  */
-/* along with this program;  if not, write to the Free Software	       */
-/* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA    */
-/*									    */
-/******************************************************************************/
-/******************************************************************************/
-/*									    */
-/* File:	tkill01.c					   */
-/*									    */
-/* Description: This tests the tkill() syscall		      */
-/*									    */
-/* Usage:  <for command-line>						 */
-/* tkill01 [-c n] [-e][-i n] [-I x] [-p x] [-t]		     */
-/*      where,  -c n : Run n copies concurrently.			     */
-/*	      -e   : Turn on errno logging.				 */
-/*	      -i n : Execute test n times.				  */
-/*	      -I x : Execute test for x seconds.			    */
-/*	      -P x : Pause for x seconds between iterations.		*/
-/*	      -t   : Turn on syscall timing.				*/
-/*									    */
-/* Total Tests: 1							     */
-/*									    */
-/* Test Name:   tkill01					     */
-/* History:     Porting from Crackerjack to LTP is done by		    */
-/*	      Manas Kumar Nayak maknayak@in.ibm.com>			*/
-/******************************************************************************/
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) Crackerjack Project., 2007
+ * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Basic tests for the tkill syscall.
+ *
+ * [Algorithm]
+ *
+ * Calls tkill and capture signals to verify success.
+ */

 #include <stdio.h>
 #include <stdlib.h>
@@ -48,59 +24,37 @@
 #include <linux/unistd.h>
 #include <sys/types.h>

-#include "test.h"
 #include "lapi/syscalls.h"
+#include "tst_test.h"

-char *TCID = "tkill01";
-int testno;
-int TST_TOTAL = 2;
+int sig_flag = 0;

-void cleanup(void)
+static void sighandler(int sig)
 {
-
-	tst_rmdir();
-}
-
-void setup(void)
-{
-	TEST_PAUSE;
-	tst_tmpdir();
+	if (sig == SIGUSR1)
+		sig_flag = 1;
 }

-int sig_count = 0;
-
-void sig_action(int sig)
-{
-	sig_count = 1;
-}
-
-int main(int ac, char **av)
+static void run(void)
 {
 	int tid;
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);

-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); ++lc) {
-		tst_count = 0;
-		for (testno = 0; testno < TST_TOTAL; ++testno) {
-			if (signal(SIGUSR1, &sig_action) == SIG_ERR)
-				tst_brkm(TBROK | TERRNO, cleanup,
-					 "signal(SIGUSR1, ..) failed");
-			TEST(tid = ltp_syscall(__NR_gettid));
-			if (TEST_RETURN == -1) {
-				tst_resm(TFAIL | TTERRNO, "tkill failed");
-			}
-			TEST(ltp_syscall(__NR_tkill, tid, SIGUSR1));
-			if (TEST_RETURN == 0) {
-				tst_resm(TPASS, "tkill call succeeded");
-			} else {
-				tst_resm(TFAIL | TTERRNO, "tkill failed");
-			}
-		}
+	SAFE_SIGNAL(SIGUSR1, sighandler);
+	TEST(tid = tst_syscall(__NR_gettid));
+	if (TST_RET == -1)
+		tst_res(TFAIL | TTERRNO, "tst_syscall(__NR_gettid) failed");
+
+	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
+	if (TST_RET == 0) {
+		while (!sig_flag);
+		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
+	} else {
+		tst_res(TFAIL | TTERRNO,
+			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
 	}
-	cleanup();
-	tst_exit();
 }
+
+static struct tst_test test = {
+	.needs_tmpdir = 1,
+	.test_all = run,
+};
--
2.17.1

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[Petr Vorel]

Hi,

> 1. Convert tkill01 to the new API;
> 2. Capture signals to verify success, while the previous code
> didn't make it work.

Generally LGTM, with comments below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -48,59 +24,37 @@
>  #include <linux/unistd.h>
>  #include <sys/types.h>
I removed these as not needed. The only one which is still relevant is <signal.h>
(I kept it although it's not needed to be included, as it's included in
tst_safe_macros.h which are included in tst_test.h).

> -#include "test.h"
>  #include "lapi/syscalls.h"
> +#include "tst_test.h"

> +int sig_flag = 0;

It should be
static int sig_flag;

...
> +static void run(void)
...
> +	SAFE_SIGNAL(SIGUSR1, sighandler);
> +	TEST(tid = tst_syscall(__NR_gettid));
> +	if (TST_RET == -1)
> +		tst_res(TFAIL | TTERRNO, "tst_syscall(__NR_gettid) failed");
gettid() manpage says "ERRORS: This call is alway successful". I suppose this is
true also for raw syscall. And it's certainly true for tst_syscall(__NR_gettid).

BTW if it really needed to be checked, tst_brk() or tst_res() with return would
need to be used.
> +
> +	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
> +	if (TST_RET == 0) {
> +		while (!sig_flag);
Not sure why you required this.
> +		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> +	} else {
> +		tst_res(TFAIL | TTERRNO,
> +			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
>  	}
> -	cleanup();
> -	tst_exit();
>  }
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.test_all = run,
> +};
In the end going to merge code below.

Kind regards,
Petr

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright (c) Linux Test Project, 2009-2021
 * Copyright (c) Crackerjack Project., 2007
 * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>
 */

/*\
 * [Description]
 *
 * Basic tests for the tkill syscall.
 *
 * [Algorithm]
 *
 * Calls tkill and capture signals to verify success.
 */

#include <signal.h>

#include "lapi/syscalls.h"
#include "tst_test.h"

static int sig_flag;

static void sighandler(int sig)
{
	if (sig == SIGUSR1)
		sig_flag = 1;
}

static void run(void)
{
	int tid;

	SAFE_SIGNAL(SIGUSR1, sighandler);

	tid = tst_syscall(__NR_gettid);

	TST_EXP_PASS(tst_syscall(__NR_tkill, tid, SIGUSR1));
}

static struct tst_test test = {
	.needs_tmpdir = 1,
	.test_all = run,
};

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[xieziyao]

Hi,

Thanks for your review, Petr.

> +	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
> +	if (TST_RET == 0) {
> +		while (!sig_flag);

This while loop is written to check whether the sighandler function captures the SIGUSR1 signal and set sig_flag to 1.

> +		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> +	} else {
> +		tst_res(TFAIL | TTERRNO,
> +			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
>  	}
> -	cleanup();
> -	tst_exit();
>  }

Other comments are fine to me.

Best Regards,
Ziyao

-----Original Message-----
From: Petr Vorel [mailto:pvorel@suse.cz] 
Sent: Monday, April 26, 2021 6:32 PM
To: xieziyao <xieziyao@huawei.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

Hi,

> 1. Convert tkill01 to the new API;
> 2. Capture signals to verify success, while the previous code didn't 
> make it work.

Generally LGTM, with comments below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -48,59 +24,37 @@
>  #include <linux/unistd.h>
>  #include <sys/types.h>
I removed these as not needed. The only one which is still relevant is <signal.h> (I kept it although it's not needed to be included, as it's included in tst_safe_macros.h which are included in tst_test.h).

> -#include "test.h"
>  #include "lapi/syscalls.h"
> +#include "tst_test.h"

> +int sig_flag = 0;

It should be
static int sig_flag;

...
> +static void run(void)
...
> +	SAFE_SIGNAL(SIGUSR1, sighandler);
> +	TEST(tid = tst_syscall(__NR_gettid));
> +	if (TST_RET == -1)
> +		tst_res(TFAIL | TTERRNO, "tst_syscall(__NR_gettid) failed");
gettid() manpage says "ERRORS: This call is alway successful". I suppose this is true also for raw syscall. And it's certainly true for tst_syscall(__NR_gettid).

BTW if it really needed to be checked, tst_brk() or tst_res() with return would need to be used.
> +
> +	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
> +	if (TST_RET == 0) {
> +		while (!sig_flag);
Not sure why you required this.
> +		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> +	} else {
> +		tst_res(TFAIL | TTERRNO,
> +			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
>  	}
> -	cleanup();
> -	tst_exit();
>  }
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.test_all = run,
> +};
In the end going to merge code below.

Kind regards,
Petr

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright (c) Linux Test Project, 2009-2021
 * Copyright (c) Crackerjack Project., 2007
 * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>  */

/*\
 * [Description]
 *
 * Basic tests for the tkill syscall.
 *
 * [Algorithm]
 *
 * Calls tkill and capture signals to verify success.
 */

#include <signal.h>

#include "lapi/syscalls.h"
#include "tst_test.h"

static int sig_flag;

static void sighandler(int sig)
{
	if (sig == SIGUSR1)
		sig_flag = 1;
}

static void run(void)
{
	int tid;

	SAFE_SIGNAL(SIGUSR1, sighandler);

	tid = tst_syscall(__NR_gettid);

	TST_EXP_PASS(tst_syscall(__NR_tkill, tid, SIGUSR1)); }

static struct tst_test test = {
	.needs_tmpdir = 1,
	.test_all = run,
};

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[Petr Vorel]

> Hi,

> Thanks for your review, Petr.

> > +	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
> > +	if (TST_RET == 0) {
> > +		while (!sig_flag);

> This while loop is written to check whether the sighandler function captures the SIGUSR1 signal and set sig_flag to 1.

Oh, correct. But not sure if it's good to use plain while.
Maybe using usleep(1000) in while loop?

TST_EXP_PASS(tst_syscall(__NR_tkill, tid, SIGUSR1));

while (!sig_flag)
	usleep(1000);

Kind regards,
Petr

> > +		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> > +	} else {
> > +		tst_res(TFAIL | TTERRNO,
> > +			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> >  	}
> > -	cleanup();
> > -	tst_exit();
> >  }

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[xieziyao]

Hi, Petr, Cyril,

I think adding usleep(1000) in while loop is a good idea, thanks.

Best Regards,
Ziyao

-----Original Message-----
From: Petr Vorel [mailto:pvorel@suse.cz] 
Sent: Monday, April 26, 2021 8:55 PM
To: xieziyao <xieziyao@huawei.com>
Cc: ltp@lists.linux.it; Cyril Hrubis <chrubis@suse.cz>
Subject: Re: [LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

> Hi,

> Thanks for your review, Petr.

> > +	TEST(tst_syscall(__NR_tkill, tid, SIGUSR1));
> > +	if (TST_RET == 0) {
> > +		while (!sig_flag);

> This while loop is written to check whether the sighandler function captures the SIGUSR1 signal and set sig_flag to 1.

Oh, correct. But not sure if it's good to use plain while.
Maybe using usleep(1000) in while loop?

TST_EXP_PASS(tst_syscall(__NR_tkill, tid, SIGUSR1));

while (!sig_flag)
	usleep(1000);

Kind regards,
Petr

> > +		tst_res(TPASS, "tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> > +	} else {
> > +		tst_res(TFAIL | TTERRNO,
> > +			"tst_syscall(__NR_tkill, %d, SIGUSR1)", tid);
> >  	}
> > -	cleanup();
> > -	tst_exit();
> >  }

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[Cyril Hrubis]

Hi!
> > -#include "test.h"
> >  #include "lapi/syscalls.h"
> > +#include "tst_test.h"
> 
> > +int sig_flag = 0;
> 
> It should be
> static int sig_flag;

It has to be volatile as well, if we are waiting in a bussy loop on it
and it's changed ansynchronously from a signal handler, otherwise
compiler may misoptimize the code.

Generally the code that waits for a signal should look like:

static volatile sig_atomic_t sig_flag;

static void setup(void)
{
	SAFE_SIGNAL(SIGUSR1, sighandler);
}

static void run(void)
{
	int timeout_ms = 1000;
	sig_flag = 0;

	TST_EXP_PASS(tst_syscall(__NR_tkill, tid, SIGUSR1));

	while (timeout_ms--) {
		if (sig_flag)
			break;

		usleep(1000);
	}

	if (sig_flag)
		tst_res(TPASS, ...);
	else
		tst_res(TFAIL, ...);
}

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[Petr Vorel]

Hi Cyril, Xie,

> > > +int sig_flag = 0;

> > It should be
> > static int sig_flag;

> It has to be volatile as well, if we are waiting in a bussy loop on it
> and it's changed ansynchronously from a signal handler, otherwise
> compiler may misoptimize the code.

> Generally the code that waits for a signal should look like:

> static volatile sig_atomic_t sig_flag;

Oh, yes, volatile. Thanks for other hints, code adjusted and whole patchset
merged.

Kind regards,
Petr

[LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

[xieziyao]

Hi, Petr, Cyril,

Learned a lot. Thanks for your review and modifications to the code.

Kind regards,
Ziyao

-----Original Message-----
From: Petr Vorel [mailto:pvorel@suse.cz] 
Sent: Thursday, April 29, 2021 2:05 AM
To: Cyril Hrubis <chrubis@suse.cz>
Cc: xieziyao <xieziyao@huawei.com>; ltp@lists.linux.it
Subject: Re: [LTP] [PATCH 1/2] syscalls/tkill: Convert tkill01 to the new API

Hi Cyril, Xie,

> > > +int sig_flag = 0;

> > It should be
> > static int sig_flag;

> It has to be volatile as well, if we are waiting in a bussy loop on it 
> and it's changed ansynchronously from a signal handler, otherwise 
> compiler may misoptimize the code.

> Generally the code that waits for a signal should look like:

> static volatile sig_atomic_t sig_flag;

Oh, yes, volatile. Thanks for other hints, code adjusted and whole patchset merged.

Kind regards,
Petr

[LTP] [PATCH 2/2] syscalls/tkill: Convert tkill02 to the new API

[Xie Ziyao]

Cleanup and convert tkill02 to the new API.

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 testcases/kernel/syscalls/tkill/tkill02.c | 105 +++++++---------------
 1 file changed, 32 insertions(+), 73 deletions(-)

diff --git a/testcases/kernel/syscalls/tkill/tkill02.c b/testcases/kernel/syscalls/tkill/tkill02.c
index 48431755b..20b461705 100644
--- a/testcases/kernel/syscalls/tkill/tkill02.c
+++ b/testcases/kernel/syscalls/tkill/tkill02.c
@@ -1,28 +1,18 @@
-/******************************************************************************
- * Copyright (c) Crackerjack Project., 2007                                   *
- *                                                                            *
- * This program is free software;  you can redistribute it and/or modify      *
- * it under the terms of the GNU General Public License as published by       *
- * the Free Software Foundation; either version 2 of the License, or          *
- * (at your option) any later version.                                        *
- *                                                                            *
- * This program is distributed in the hope that it will be useful,            *
- * but WITHOUT ANY WARRANTY;  without even the implied warranty of            *
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See                  *
- * the GNU General Public License for more details.                           *
- *                                                                            *
- * You should have received a copy of the GNU General Public License          *
- * along with this program;  if not, write to the Free Software Foundation,   *
- * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA           *
- *                                                                            *
- ******************************************************************************/
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * File:	tkill02.c
+ * Copyright (c) Crackerjack Project., 2007
+ * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Basic tests for the tkill errors.
  *
- * Description: This tests the tkill() syscall
+ * [Algorithm]
  *
- * History:     Porting from Crackerjack to LTP is done by
- *              Manas Kumar Nayak maknayak@in.ibm.com>
+ * - EINVAL on an invalid thread ID
+ * - ESRCH when no process with the specified thread ID exists
  */

 #include <stdio.h>
@@ -32,66 +22,35 @@
 #include <signal.h>
 #include <sys/syscall.h>

-#include "test.h"
 #include "lapi/syscalls.h"
+#include "tst_test.h"

-char *TCID = "tkill02";
-int testno;
-
+static pid_t expired_pid;
 static pid_t inval_tid = -1;
-static pid_t unused_tid;
-
-void cleanup(void)
-{
-	tst_rmdir();
-}
-
-void setup(void)
-{
-	TEST_PAUSE;
-	tst_tmpdir();
-
-	unused_tid = tst_get_unused_pid(cleanup);
-}

 struct test_case_t {
 	int *tid;
 	int exp_errno;
-} test_cases[] = {
-	{&inval_tid, EINVAL},
-	{&unused_tid, ESRCH}
+	const char *desc;
+} tc[] = {
+	{&inval_tid, EINVAL, "inval_tid"},
+	{&expired_pid, ESRCH, "expired_pid"}
 };

-int TST_TOTAL = sizeof(test_cases) / sizeof(test_cases[0]);
-
-int main(int ac, char **av)
+static void setup(void)
 {
-	int i;
-
-	setup();
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	for (i = 0; i < TST_TOTAL; i++) {
-
-		TEST(ltp_syscall(__NR_tkill, *(test_cases[i].tid), SIGUSR1));
+	expired_pid = tst_get_unused_pid();
+}

-		if (TEST_RETURN == -1) {
-			if (TEST_ERRNO == test_cases[i].exp_errno) {
-				tst_resm(TPASS | TTERRNO,
-					 "tkill(%d, SIGUSR1) failed as expected",
-					 *(test_cases[i].tid));
-			} else {
-				tst_brkm(TFAIL | TTERRNO, cleanup,
-					 "tkill(%d, SIGUSR1) failed unexpectedly",
-					 *(test_cases[i].tid));
-			}
-		} else {
-			tst_brkm(TFAIL, cleanup,
-				 "tkill(%d) succeeded unexpectedly",
-				 *(test_cases[i].tid));
-		}
-	}
-	cleanup();
-	tst_exit();
+static void run(unsigned int i)
+{
+	TST_EXP_FAIL(tst_syscall(__NR_tkill, *(tc[i].tid), SIGUSR1),
+		     tc[i].exp_errno, "tst_syscall(__NR_tkill, %s)", tc[i].desc);
 }
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tc),
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test = run,
+};
--
2.17.1

[LTP] [PATCH 2/2] syscalls/tkill: Convert tkill02 to the new API

[Petr Vorel]

Hi,

LGTM with very minor changes.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +static pid_t expired_pid;
>  static pid_t inval_tid = -1;
> -static pid_t unused_tid;

IMHO unused_tid is better describe what the variable holds.

> -
> -void cleanup(void)
> -{
> -	tst_rmdir();
> -}
> -
> -void setup(void)
> -{
> -	TEST_PAUSE;
> -	tst_tmpdir();
> -
> -	unused_tid = tst_get_unused_pid(cleanup);
> -}

>  struct test_case_t {
>  	int *tid;
>  	int exp_errno;
> -} test_cases[] = {
> -	{&inval_tid, EINVAL},
> -	{&unused_tid, ESRCH}
> +	const char *desc;
> +} tc[] = {
> +	{&inval_tid, EINVAL, "inval_tid"},
> +	{&expired_pid, ESRCH, "expired_pid"}
Well, there is no point to print variable name.  Better would be "invalid TID"
and "unused TID". But IMHO just writing what we expect is enough.

It could be:
#define ERRNO_DESC(x) .exp_errno = x, .desc = "exp" #x
...

	{&inval_tid, ERRNO_DESC(EINVAL},
	{&expired_pid, ERRNO_DESC(ESRCH}

But we have tst_strerrno(), thus just:

struct test_case_t {
	int *tid;
	int exp_errno;
} tc[] = {
	{&inval_tid, EINVAL},
	{&unused_tid, ESRCH}
};

...
	TST_EXP_FAIL(tst_syscall(__NR_tkill, *(tc[i].tid), SIGUSR1),
		     tc[i].exp_errno, "tst_syscall(__NR_tkill) expecting %s",
			 tst_strerrno(tc[i].exp_errno));

I'll merge code below.

Kind regards,
Petr

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright (c) Crackerjack Project., 2007
 * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>
 */

/*\
 * [Description]
 *
 * Basic tests for the tkill() errors.
 *
 * [Algorithm]
 *
 * - EINVAL on an invalid thread ID
 * - ESRCH when no process with the specified thread ID exists
 */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <unistd.h>
#include <signal.h>
#include <sys/syscall.h>

#include "lapi/syscalls.h"
#include "tst_test.h"

static pid_t unused_tid;
static pid_t inval_tid = -1;

struct test_case_t {
	int *tid;
	int exp_errno;
} tc[] = {
	{&inval_tid, EINVAL},
	{&unused_tid, ESRCH}
};

static void setup(void)
{
	unused_tid = tst_get_unused_pid();
}

static void run(unsigned int i)
{
	TST_EXP_FAIL(tst_syscall(__NR_tkill, *(tc[i].tid), SIGUSR1),
		     tc[i].exp_errno, "tst_syscall(__NR_tkill) expecting %s",
			 tst_strerrno(tc[i].exp_errno));
}

static struct tst_test test = {
	.tcnt = ARRAY_SIZE(tc),
	.needs_tmpdir = 1,
	.setup = setup,
	.test = run,
};

[LTP] [PATCH 2/2] syscalls/tkill: Convert tkill02 to the new API

[xieziyao]

Hi, Petr,

LGTM, thanks for your review.

Best Regards,
Ziyao

-----Original Message-----
From: Petr Vorel [mailto:pvorel@suse.cz] 
Sent: Monday, April 26, 2021 7:12 PM
To: xieziyao <xieziyao@huawei.com>
Cc: ltp@lists.linux.it
Subject: Re: [LTP] [PATCH 2/2] syscalls/tkill: Convert tkill02 to the new API

Hi,

LGTM with very minor changes.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +static pid_t expired_pid;
>  static pid_t inval_tid = -1;
> -static pid_t unused_tid;

IMHO unused_tid is better describe what the variable holds.

> -
> -void cleanup(void)
> -{
> -	tst_rmdir();
> -}
> -
> -void setup(void)
> -{
> -	TEST_PAUSE;
> -	tst_tmpdir();
> -
> -	unused_tid = tst_get_unused_pid(cleanup);
> -}

>  struct test_case_t {
>  	int *tid;
>  	int exp_errno;
> -} test_cases[] = {
> -	{&inval_tid, EINVAL},
> -	{&unused_tid, ESRCH}
> +	const char *desc;
> +} tc[] = {
> +	{&inval_tid, EINVAL, "inval_tid"},
> +	{&expired_pid, ESRCH, "expired_pid"}
Well, there is no point to print variable name.  Better would be "invalid TID"
and "unused TID". But IMHO just writing what we expect is enough.

It could be:
#define ERRNO_DESC(x) .exp_errno = x, .desc = "exp" #x ...

	{&inval_tid, ERRNO_DESC(EINVAL},
	{&expired_pid, ERRNO_DESC(ESRCH}

But we have tst_strerrno(), thus just:

struct test_case_t {
	int *tid;
	int exp_errno;
} tc[] = {
	{&inval_tid, EINVAL},
	{&unused_tid, ESRCH}
};

...
	TST_EXP_FAIL(tst_syscall(__NR_tkill, *(tc[i].tid), SIGUSR1),
		     tc[i].exp_errno, "tst_syscall(__NR_tkill) expecting %s",
			 tst_strerrno(tc[i].exp_errno));

I'll merge code below.

Kind regards,
Petr

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright (c) Crackerjack Project., 2007
 * Ported from Crackerjack to LTP by Manas Kumar Nayak maknayak@in.ibm.com>  */

/*\
 * [Description]
 *
 * Basic tests for the tkill() errors.
 *
 * [Algorithm]
 *
 * - EINVAL on an invalid thread ID
 * - ESRCH when no process with the specified thread ID exists  */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <unistd.h>
#include <signal.h>
#include <sys/syscall.h>

#include "lapi/syscalls.h"
#include "tst_test.h"

static pid_t unused_tid;
static pid_t inval_tid = -1;

struct test_case_t {
	int *tid;
	int exp_errno;
} tc[] = {
	{&inval_tid, EINVAL},
	{&unused_tid, ESRCH}
};

static void setup(void)
{
	unused_tid = tst_get_unused_pid();
}

static void run(unsigned int i)
{
	TST_EXP_FAIL(tst_syscall(__NR_tkill, *(tc[i].tid), SIGUSR1),
		     tc[i].exp_errno, "tst_syscall(__NR_tkill) expecting %s",
			 tst_strerrno(tc[i].exp_errno));
}

static struct tst_test test = {
	.tcnt = ARRAY_SIZE(tc),
	.needs_tmpdir = 1,
	.setup = setup,
	.test = run,
};