[LTP] [PATCH 1/5] kcmp.h: move it to ltp include/lapi directory

[LTP] [PATCH 1/5] kcmp.h: move it to ltp include/lapi directory

[Yang Xu]

So other test case can call kcmp directly just include lapi/kcmp.h.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 {testcases/kernel/syscalls/kcmp => include/lapi}/kcmp.h | 0
 testcases/kernel/syscalls/kcmp/kcmp01.c                 | 2 +-
 testcases/kernel/syscalls/kcmp/kcmp02.c                 | 2 +-
 testcases/kernel/syscalls/kcmp/kcmp03.c                 | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename {testcases/kernel/syscalls/kcmp => include/lapi}/kcmp.h (100%)

diff --git a/testcases/kernel/syscalls/kcmp/kcmp.h b/include/lapi/kcmp.h
similarity index 100%
rename from testcases/kernel/syscalls/kcmp/kcmp.h
rename to include/lapi/kcmp.h
diff --git a/testcases/kernel/syscalls/kcmp/kcmp01.c b/testcases/kernel/syscalls/kcmp/kcmp01.c
index a03a25a2b..903525ff0 100644
--- a/testcases/kernel/syscalls/kcmp/kcmp01.c
+++ b/testcases/kernel/syscalls/kcmp/kcmp01.c
@@ -15,7 +15,7 @@
 
 #include "tst_test.h"
 #include "lapi/fcntl.h"
-#include "kcmp.h"
+#include "lapi/kcmp.h"
 
 #define TEST_FILE "test_file"
 #define TEST_FILE2 "test_file2"
diff --git a/testcases/kernel/syscalls/kcmp/kcmp02.c b/testcases/kernel/syscalls/kcmp/kcmp02.c
index 993d9a4a4..ab07bb866 100644
--- a/testcases/kernel/syscalls/kcmp/kcmp02.c
+++ b/testcases/kernel/syscalls/kcmp/kcmp02.c
@@ -17,7 +17,7 @@
 
 #include "tst_test.h"
 #include "lapi/fcntl.h"
-#include "kcmp.h"
+#include "lapi/kcmp.h"
 
 #define TEST_FILE "test_file"
 #define TEST_FILE2 "test_file2"
diff --git a/testcases/kernel/syscalls/kcmp/kcmp03.c b/testcases/kernel/syscalls/kcmp/kcmp03.c
index df8633c5f..4b90e6d87 100644
--- a/testcases/kernel/syscalls/kcmp/kcmp03.c
+++ b/testcases/kernel/syscalls/kcmp/kcmp03.c
@@ -20,7 +20,7 @@
 #include <stdlib.h>
 #include <sys/wait.h>
 #include "tst_test.h"
-#include "kcmp.h"
+#include "lapi/kcmp.h"
 #include "lapi/sched.h"
 
 #define STACK_SIZE	(1024*1024)
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/5] lapi/kcmp.h: Replace GPL with SPDX-License-Identifier

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 include/lapi/kcmp.h | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/include/lapi/kcmp.h b/include/lapi/kcmp.h
index 59371fc07..77086191f 100644
--- a/include/lapi/kcmp.h
+++ b/include/lapi/kcmp.h
@@ -1,29 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2015 Cedric Hnyda <chnyda@suse.com>
- *
- * 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 would 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 the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#ifndef KCMP_H
-#define KCMP_H
+#ifndef LAPI_KCMP_H__
+#define LAPI_KCMP_H__
 
 #include <sys/types.h>
 #include "config.h"
 #include "lapi/syscalls.h"
 
-#if !defined(HAVE_ENUM_KCMP_TYPE)
+#ifndef HAVE_ENUM_KCMP_TYPE
 
 enum kcmp_type {
 	KCMP_FILE,
@@ -42,7 +29,7 @@ enum kcmp_type {
 
 #endif
 
-#if !defined(HAVE_KCMP)
+#ifndef HAVE_KCMP
 
 int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
 {
@@ -51,4 +38,4 @@ int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
 
 #endif
 
-#endif /* KCMP_H */
+#endif /* LAPI_KCMP_H__ */
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/5] pidfd_getfd.h: add fallback

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 configure.ac               |  1 +
 include/lapi/pidfd_getfd.h | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 include/lapi/pidfd_getfd.h

diff --git a/configure.ac b/configure.ac
index 8d2c5b1c4..49499704e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -115,6 +115,7 @@ AC_CHECK_FUNCS_ONCE([ \
     open_tree \
     openat \
     openat2 \
+    pidfd_getfd \
     pidfd_open \
     pidfd_send_signal \
     pkey_mprotect \
diff --git a/include/lapi/pidfd_getfd.h b/include/lapi/pidfd_getfd.h
new file mode 100644
index 000000000..1f488a518
--- /dev/null
+++ b/include/lapi/pidfd_getfd.h
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+#ifndef LAPI_PIDFD_GETFD_H__
+#define LAPI_PIDFD_GETFD_H__
+
+#include "lapi/syscalls.h"
+#include "config.h"
+
+static inline void pidfd_getfd_supported(void)
+{
+	/* allow the tests to fail early */
+	tst_syscall(__NR_pidfd_getfd);
+}
+
+#ifndef HAVE_PIDFD_GETFD
+static inline int pidfd_getfd(int pidfd, int targetfd, unsigned int flags)
+{
+	return tst_syscall(__NR_pidfd_getfd, pidfd, targetfd, flags);
+}
+#endif
+
+#endif /* LAPI_PIDFD_GETFD_H__ */
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Yang Xu]

The pidfd_getfd() system call allocates a new file descriptor in the calling process.
This new file descriptor is a duplicate of an existing file descriptor, targetfd, in
the process referred to by the PID file descriptor pidfd.

pidfd_getfd was introduced since kernel 5.6 and pidfd_open was introduced since kernel 5.3.
pidfd_getfd is not similar to pidfd_send_signal because it didn't support fd from
/proc/pid directory. So we must check kernel whether support pidfd_open because some linux
distribution backports pidfd_getfd but not backport pidfd_open.

Here we check whether the newfd set close-on-exce flag and use kcmp to check the two fds whether
point to the same open file instead of using fstat to check.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 runtest/syscalls                              |  2 +
 .../kernel/syscalls/pidfd_getfd/.gitignore    |  1 +
 .../kernel/syscalls/pidfd_getfd/Makefile      |  6 ++
 .../syscalls/pidfd_getfd/pidfd_getfd01.c      | 96 +++++++++++++++++++
 4 files changed, 105 insertions(+)
 create mode 100644 testcases/kernel/syscalls/pidfd_getfd/.gitignore
 create mode 100644 testcases/kernel/syscalls/pidfd_getfd/Makefile
 create mode 100644 testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c

diff --git a/runtest/syscalls b/runtest/syscalls
index ce6f89f88..6155712cc 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -951,6 +951,8 @@ pause03 pause03
 personality01 personality01
 personality02 personality02
 
+pidfd_getfd01 pidfd_getfd01
+
 pidfd_open01 pidfd_open01
 pidfd_open02 pidfd_open02
 pidfd_open03 pidfd_open03
diff --git a/testcases/kernel/syscalls/pidfd_getfd/.gitignore b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
new file mode 100644
index 000000000..c193ffee7
--- /dev/null
+++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
@@ -0,0 +1 @@
+pidfd_getfd01
diff --git a/testcases/kernel/syscalls/pidfd_getfd/Makefile b/testcases/kernel/syscalls/pidfd_getfd/Makefile
new file mode 100644
index 000000000..5ea7d67db
--- /dev/null
+++ b/testcases/kernel/syscalls/pidfd_getfd/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
new file mode 100644
index 000000000..f3f5132fe
--- /dev/null
+++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Basic pidfd_getfd() test:
+ *
+ * - the close-on-exec flag is set on the file descriptor returned by
+ *   pidfd_getfd
+ * - use kcmp to check whether a file descriptor idx1 in the process pid1
+ *   refers to the same open file description as file descriptor idx2 in
+ *   the process pid2
+ */
+
+#include <unistd.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include "tst_test.h"
+#include "lapi/pidfd_open.h"
+#include "lapi/pidfd_getfd.h"
+#include "lapi/kcmp.h"
+#include "tst_safe_macros.h"
+
+#define TESTFILE "testfile"
+
+static int fds[2];
+
+static void do_child(void)
+{
+	int fd;
+
+	SAFE_CLOSE(fds[0]);
+	fd = SAFE_CREAT(TESTFILE, 0644);
+	SAFE_WRITE(1, fds[1], &fd, sizeof(fd));
+	TST_CHECKPOINT_WAIT(0);
+	SAFE_CLOSE(fd);
+	SAFE_CLOSE(fds[1]);
+	exit(0);
+}
+
+static void run(void)
+{
+	int flag, pid, pidfd, targetfd, remotefd;
+
+	SAFE_PIPE(fds);
+	pid = SAFE_FORK();
+	if (!pid)
+		do_child();
+
+	SAFE_CLOSE(fds[1]);
+	TST_PROCESS_STATE_WAIT(pid, 'S', 0);
+
+	TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open(pid, 0)");
+	pidfd = TST_RET;
+	SAFE_READ(1, fds[0], &targetfd, sizeof(targetfd));
+	TST_EXP_FD_SILENT(pidfd_getfd(pidfd, targetfd, 0),
+		"pidfd_getfd(%d, %d , 0)", pidfd, targetfd);
+
+	remotefd = TST_RET;
+	flag = fcntl(remotefd, F_GETFD);
+	if (flag == -1)
+		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
+	if (!(flag & FD_CLOEXEC))
+		tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag");
+
+	TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
+	if (TST_RET != 0)
+		tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description");
+
+	TST_CHECKPOINT_WAKE(0);
+	SAFE_CLOSE(remotefd);
+
+	tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);
+	SAFE_CLOSE(pidfd);
+	SAFE_CLOSE(fds[0]);
+	tst_reap_children();
+}
+
+static void setup(void)
+{
+	pidfd_open_supported();
+	pidfd_getfd_supported();
+}
+
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_checkpoints = 1,
+	.forks_child = 1,
+	.setup = setup,
+	.test_all = run,
+};
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/pidfd_getfd/.gitignore    |   1 +
 .../syscalls/pidfd_getfd/pidfd_getfd02.c      | 108 ++++++++++++++++++
 3 files changed, 110 insertions(+)
 create mode 100644 testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 6155712cc..436cc2a0a 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -952,6 +952,7 @@ personality01 personality01
 personality02 personality02
 
 pidfd_getfd01 pidfd_getfd01
+pidfd_getfd02 pidfd_getfd02
 
 pidfd_open01 pidfd_open01
 pidfd_open02 pidfd_open02
diff --git a/testcases/kernel/syscalls/pidfd_getfd/.gitignore b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
index c193ffee7..f944adc6f 100644
--- a/testcases/kernel/syscalls/pidfd_getfd/.gitignore
+++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
@@ -1 +1,2 @@
 pidfd_getfd01
+pidfd_getfd02
diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c
new file mode 100644
index 000000000..e4f5a1467
--- /dev/null
+++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * Tests basic error handling of the pidfd_open syscall.
+ *
+ * - EBADF pidfd is not a valid PID file descriptor
+ * - EBADF targetfd is not an open file descriptor in the process referred
+ *   to by pidfd
+ * - EINVAL flags is not 0
+ * - ESRCH the process referred to by pidfd does not exist(it has terminated and
+ *   been waited on))
+ * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions
+ *   over the process referred to by pidfd
+ */
+
+#include <stdlib.h>
+#include <pwd.h>
+#include "tst_test.h"
+#include "lapi/pidfd_open.h"
+#include "lapi/pidfd_getfd.h"
+
+static int valid_pidfd, expire_pidfd, invalid_pidfd = -1;
+static uid_t uid;
+
+static struct tcase {
+	char *name;
+	int *pidfd;
+	int targetfd;
+	int flags;
+	int exp_errno;
+} tcases[] = {
+	{"invalid pidfd", &invalid_pidfd, 0, 0, EBADF},
+	{"invalid targetfd", &valid_pidfd, -1, 0, EBADF},
+	{"invalid flags", &valid_pidfd, 0, 1, EINVAL},
+	{"the process referred to by pidfd doesn't exist", &expire_pidfd, 0, 0, ESRCH},
+	{"lack of required permission", &valid_pidfd, 0, 0, EPERM},
+};
+
+static void setup(void)
+{
+	struct passwd *pw;
+
+	pw = SAFE_GETPWNAM("nobody");
+	uid = pw->pw_uid;
+
+	pidfd_open_supported();
+	pidfd_getfd_supported();
+
+	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
+	if (!TST_PASS)
+		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");
+	valid_pidfd = TST_RET;
+}
+
+static void run(unsigned int n)
+{
+	struct tcase *tc = &tcases[n];
+	int pid;
+
+	switch (tc->exp_errno) {
+	case EPERM:
+		pid = SAFE_FORK();
+		if (!pid) {
+			SAFE_SETUID(uid);
+			TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags),
+				tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
+				valid_pidfd, tc->targetfd, tc->flags, tc->name);
+			TST_CHECKPOINT_WAKE(0);
+			exit(0);
+		}
+		TST_CHECKPOINT_WAIT(0);
+		SAFE_WAIT(NULL);
+		return;
+	break;
+	case ESRCH:
+		pid = SAFE_FORK();
+		if (!pid) {
+			TST_CHECKPOINT_WAIT(0);
+			exit(0);
+		}
+		TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open");
+		*tc->pidfd = TST_RET;
+		TST_CHECKPOINT_WAKE(0);
+		SAFE_WAIT(NULL);
+	break;
+	default:
+	break;
+	};
+
+	TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags),
+		tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
+		*tc->pidfd, tc->targetfd, tc->flags, tc->name);
+}
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tcases),
+	.test = run,
+	.setup = setup,
+	.needs_root = 1,
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+};
-- 
2.23.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 1/5] kcmp.h: move it to ltp include/lapi directory

[Petr Vorel]

Hi Xu,

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

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] lapi/kcmp.h: Replace GPL with SPDX-License-Identifier

[Petr Vorel]

And this one qas well (although there is cleanup of guards and #ifndef).

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

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] pidfd_getfd.h: add fallback

[Petr Vorel]

Hi Xu,

> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  configure.ac               |  1 +
>  include/lapi/pidfd_getfd.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>  create mode 100644 include/lapi/pidfd_getfd.h

> diff --git a/configure.ac b/configure.ac
> index 8d2c5b1c4..49499704e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -115,6 +115,7 @@ AC_CHECK_FUNCS_ONCE([ \
>      open_tree \
>      openat \
>      openat2 \
> +    pidfd_getfd \
>      pidfd_open \
>      pidfd_send_signal \
>      pkey_mprotect \
> diff --git a/include/lapi/pidfd_getfd.h b/include/lapi/pidfd_getfd.h
> new file mode 100644
> index 000000000..1f488a518
> --- /dev/null
> +++ b/include/lapi/pidfd_getfd.h
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +#ifndef LAPI_PIDFD_GETFD_H__
> +#define LAPI_PIDFD_GETFD_H__
> +
> +#include "lapi/syscalls.h"
> +#include "config.h"
nit: IMHO it's better to always put config.h at the first place.

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

Kind regards,
Petr

> +
> +static inline void pidfd_getfd_supported(void)
> +{
> +	/* allow the tests to fail early */
> +	tst_syscall(__NR_pidfd_getfd);
> +}
> +
> +#ifndef HAVE_PIDFD_GETFD
> +static inline int pidfd_getfd(int pidfd, int targetfd, unsigned int flags)
> +{
> +	return tst_syscall(__NR_pidfd_getfd, pidfd, targetfd, flags);
> +}
> +#endif
> +
> +#endif /* LAPI_PIDFD_GETFD_H__ */

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] lapi/kcmp.h: Replace GPL with SPDX-License-Identifier

[Petr Vorel]

Hi Xu,

> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  include/lapi/kcmp.h | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)

> diff --git a/include/lapi/kcmp.h b/include/lapi/kcmp.h
> index 59371fc07..77086191f 100644
> --- a/include/lapi/kcmp.h
> +++ b/include/lapi/kcmp.h
> @@ -1,29 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2015 Cedric Hnyda <chnyda@suse.com>
> - *
> - * 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 would 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 the Free Software Foundation,
> - * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   */

> -#ifndef KCMP_H
> -#define KCMP_H
> +#ifndef LAPI_KCMP_H__
> +#define LAPI_KCMP_H__

>  #include <sys/types.h>
>  #include "config.h"
>  #include "lapi/syscalls.h"

> -#if !defined(HAVE_ENUM_KCMP_TYPE)
> +#ifndef HAVE_ENUM_KCMP_TYPE

>  enum kcmp_type {
>  	KCMP_FILE,
> @@ -42,7 +29,7 @@ enum kcmp_type {

>  #endif

> -#if !defined(HAVE_KCMP)
> +#ifndef HAVE_KCMP

>  int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
$ make check-pidfd_getfd01
../../../../include/lapi/kcmp.h:34:5: warning: Symbol 'kcmp' has no prototype or library ('tst_') prefix. Should it be static?

Could you please mark this as static inline?

Not sure if this should be with the same commit as SPDX.
When doing cleanup it'd be worth to fix it.

Kind regards,
Petr

>  {
> @@ -51,4 +38,4 @@ int kcmp(int pid1, int pid2, int type, int fd1, int fd2)

>  #endif

> -#endif /* KCMP_H */
> +#endif /* LAPI_KCMP_H__ */

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Xu,

...
> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
...
> +	remotefd = TST_RET;
> +	flag = fcntl(remotefd, F_GETFD);
> +	if (flag == -1)
> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
Just:
flag = SAFE_FCNTL(remotefd, F_GETFD);

> +	if (!(flag & FD_CLOEXEC))
> +		tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag");
> +
> +	TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
> +	if (TST_RET != 0)
> +		tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description");
Maybe just:
       TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));

       if (!TST_PASS)
               return;
Although your version is more descriptive.

> +
> +	TST_CHECKPOINT_WAKE(0);
> +	SAFE_CLOSE(remotefd);
> +
> +	tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);
> +	SAFE_CLOSE(pidfd);
Shouldn't be pidfd closed in cleanup? In case fcntl() fails it's kept open.
> +	SAFE_CLOSE(fds[0]);
The same is for fds, which is already static.

These are very minor and you can change it before merge.

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

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Xu,

> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
> @@ -0,0 +1 @@
> +pidfd_getfd01
/pidfd_getfd01

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[Petr Vorel]

Hi Xu,

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

Thanks! Few comments below, can be fixed before merge.

> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
> @@ -1 +1,2 @@
>  pidfd_getfd01
> +pidfd_getfd02
Again /pidfd_getfd02

> diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c
...
> +/*\
> + * [Description]
> + *
> + * Tests basic error handling of the pidfd_open syscall.
> + *
> + * - EBADF pidfd is not a valid PID file descriptor
> + * - EBADF targetfd is not an open file descriptor in the process referred
> + *   to by pidfd
> + * - EINVAL flags is not 0
> + * - ESRCH the process referred to by pidfd does not exist(it has terminated and
> + *   been waited on))

nit: add space and remove redundant bracket
 * - ESRCH the process referred to by pidfd does not exist (it has terminated and
 *   been waited on)

> + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions
> + *   over the process referred to by pidfd

+1 (only ENFILE "The system-wide limit on the total number of open files has been
reached." which is probably not worth of implementing).
...

> +static void setup(void)
> +{
> +	struct passwd *pw;
> +
> +	pw = SAFE_GETPWNAM("nobody");
> +	uid = pw->pw_uid;
> +
> +	pidfd_open_supported();
> +	pidfd_getfd_supported();
nit: I'd put these before SAFE_GETPWNAM().

> +
> +	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
If you wait for Cyril's patch adding auto stringification
https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/

you can use just:
TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));

and get more info.

> +	if (!TST_PASS)
> +		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");

@Cyril: would it be possible to to allow using also tst_brk() in macros in
tst_test_macros.h?

Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated

> +	valid_pidfd = TST_RET;
> +}
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	int pid;
> +
> +	switch (tc->exp_errno) {
> +	case EPERM:
> +		pid = SAFE_FORK();
SAFE_FORK() can be before switch.

> +		if (!pid) {
> +			SAFE_SETUID(uid);
> +			TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags),
> +				tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
> +				valid_pidfd, tc->targetfd, tc->flags, tc->name);
> +			TST_CHECKPOINT_WAKE(0);
> +			exit(0);
> +		}
> +		TST_CHECKPOINT_WAIT(0);
> +		SAFE_WAIT(NULL);
> +		return;
> +	break;
> +	case ESRCH:
> +		pid = SAFE_FORK();
> +		if (!pid) {
> +			TST_CHECKPOINT_WAIT(0);
> +			exit(0);
> +		}
> +		TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open");
> +		*tc->pidfd = TST_RET;
> +		TST_CHECKPOINT_WAKE(0);
> +		SAFE_WAIT(NULL);
> +	break;
> +	default:
> +	break;
> +	};

IMHO more readable would be instead of switch use if/else if or 2 functions.


Kind regards,
Petr

> +
> +	TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags),
> +		tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
> +		*tc->pidfd, tc->targetfd, tc->flags, tc->name);
> +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] lapi/kcmp.h: Replace GPL with SPDX-License-Identifier

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   include/lapi/kcmp.h | 25 ++++++-------------------
>>   1 file changed, 6 insertions(+), 19 deletions(-)
>
>> diff --git a/include/lapi/kcmp.h b/include/lapi/kcmp.h
>> index 59371fc07..77086191f 100644
>> --- a/include/lapi/kcmp.h
>> +++ b/include/lapi/kcmp.h
>> @@ -1,29 +1,16 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>   /*
>>    * Copyright (c) 2015 Cedric Hnyda<chnyda@suse.com>
>> - *
>> - * 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 would 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 the Free Software Foundation,
>> - * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>    */
>
>> -#ifndef KCMP_H
>> -#define KCMP_H
>> +#ifndef LAPI_KCMP_H__
>> +#define LAPI_KCMP_H__
>
>>   #include<sys/types.h>
>>   #include "config.h"
>>   #include "lapi/syscalls.h"
>
>> -#if !defined(HAVE_ENUM_KCMP_TYPE)
>> +#ifndef HAVE_ENUM_KCMP_TYPE
>
>>   enum kcmp_type {
>>   	KCMP_FILE,
>> @@ -42,7 +29,7 @@ enum kcmp_type {
>
>>   #endif
>
>> -#if !defined(HAVE_KCMP)
>> +#ifndef HAVE_KCMP
>
>>   int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> $ make check-pidfd_getfd01
> ../../../../include/lapi/kcmp.h:34:5: warning: Symbol 'kcmp' has no prototype or library ('tst_') prefix. Should it be static?
>
> Could you please mark this as static inline?
Of course, in fact, I have changed, but forgot to add it into the final 
version.
>
> Not sure if this should be with the same commit as SPDX.
IMO, I will add it into the same commit and mention that we also replace 
GPL with SPDX. Title maybe changed into "lapi/kcmp.h: Simplify code ".

Best Regards
Yang Xu
> When doing cleanup it'd be worth to fix it.
>
> Kind regards,
> Petr
>
>>   {
>> @@ -51,4 +38,4 @@ int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
>
>>   #endif
>
>> -#endif /* KCMP_H */
>> +#endif /* LAPI_KCMP_H__ */

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/5] pidfd_getfd.h: add fallback

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   configure.ac               |  1 +
>>   include/lapi/pidfd_getfd.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>   create mode 100644 include/lapi/pidfd_getfd.h
>
>> diff --git a/configure.ac b/configure.ac
>> index 8d2c5b1c4..49499704e 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -115,6 +115,7 @@ AC_CHECK_FUNCS_ONCE([ \
>>       open_tree \
>>       openat \
>>       openat2 \
>> +    pidfd_getfd \
>>       pidfd_open \
>>       pidfd_send_signal \
>>       pkey_mprotect \
>> diff --git a/include/lapi/pidfd_getfd.h b/include/lapi/pidfd_getfd.h
>> new file mode 100644
>> index 000000000..1f488a518
>> --- /dev/null
>> +++ b/include/lapi/pidfd_getfd.h
>> @@ -0,0 +1,26 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2022 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu<xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +#ifndef LAPI_PIDFD_GETFD_H__
>> +#define LAPI_PIDFD_GETFD_H__
>> +
>> +#include "lapi/syscalls.h"
>> +#include "config.h"
> nit: IMHO it's better to always put config.h at the first place.

Will do.

Best Regards
Yang Xu
>
> Otherwise LGTM.
> Reviewed-by: Petr Vorel<pvorel@suse.cz>
>
> Kind regards,
> Petr
>
>> +
>> +static inline void pidfd_getfd_supported(void)
>> +{
>> +	/* allow the tests to fail early */
>> +	tst_syscall(__NR_pidfd_getfd);
>> +}
>> +
>> +#ifndef HAVE_PIDFD_GETFD
>> +static inline int pidfd_getfd(int pidfd, int targetfd, unsigned int flags)
>> +{
>> +	return tst_syscall(__NR_pidfd_getfd, pidfd, targetfd, flags);
>> +}
>> +#endif
>> +
>> +#endif /* LAPI_PIDFD_GETFD_H__ */

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> ...
>> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
> ...
>> +	remotefd = TST_RET;
>> +	flag = fcntl(remotefd, F_GETFD);
>> +	if (flag == -1)
>> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
> Just:
> flag = SAFE_FCNTL(remotefd, F_GETFD);
Yes, I almost forgot we have this macro.
>
>> +	if (!(flag&  FD_CLOEXEC))
>> +		tst_res(TFAIL, "pidfd_getfd() didn't set close-on-exec flag");
>> +
>> +	TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>> +	if (TST_RET != 0)
>> +		tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description");
> Maybe just:
>         TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
I think we can't use this macro here see below(kcmp manpage about return 
value):
0 v1 is equal to v2; in other words, the two processes share the resource.

1   v1 is less than v2.

2   v1 is greater than v2.

3   v1 is not equal to v2, but ordering information is unavailable.

On error, -1 is returned, and errno is set appropriately.

So 1,2,3 are not invalid return value.
TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
if (TST_RET==-1) {
	tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
	goto free;
} else {
	if (TST_RET < 0 || TST_RET > 3)
	tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET);
	goto free;
}

free:
  ....


>
>         if (!TST_PASS)
>                 return;
> Although your version is more descriptive.
>
>> +
>> +	TST_CHECKPOINT_WAKE(0);
>> +	SAFE_CLOSE(remotefd);
>> +
>> +	tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);
>> +	SAFE_CLOSE(pidfd);
> Shouldn't be pidfd closed in cleanup? In case fcntl() fails it's kept open.
Will do.
>> +	SAFE_CLOSE(fds[0]);
> The same is for fds, which is already static.
ok.
>
> These are very minor and you can change it before merge.
>
> Reviewed-by: Petr Vorel<pvorel@suse.cz>
Thanks for your review.

ps: I merged the first 3 patches, and will send a v2 for the remaining 2 
patches.

Best Regards
Yang Xu
>
> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
>> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
>> @@ -0,0 +1 @@
>> +pidfd_getfd01
> /pidfd_getfd01
Thanks, will add it in v2.

The two ways also work well.

ps:pidfd_open also miss "/". Shoud we add this rule in maintainer review 
list and .gitignore info in doc?

Best Regards
Yang Xu

>
> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> Reviewed-by: Petr Vorel<pvorel@suse.cz>
>
> Thanks! Few comments below, can be fixed before merge.
>
>> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
>> @@ -1 +1,2 @@
>>   pidfd_getfd01
>> +pidfd_getfd02
> Again /pidfd_getfd02
>
>> diff --git a/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd02.c
> ...
>> +/*\
>> + * [Description]
>> + *
>> + * Tests basic error handling of the pidfd_open syscall.
>> + *
>> + * - EBADF pidfd is not a valid PID file descriptor
>> + * - EBADF targetfd is not an open file descriptor in the process referred
>> + *   to by pidfd
>> + * - EINVAL flags is not 0
>> + * - ESRCH the process referred to by pidfd does not exist(it has terminated and
>> + *   been waited on))
>
> nit: add space and remove redundant bracket
>   * - ESRCH the process referred to by pidfd does not exist (it has terminated and
>   *   been waited on)
OK.
>
>> + * - EPERM the calling process doesn't have PTRACE_MODE_ATTACH_REALCREDS permissions
>> + *   over the process referred to by pidfd
>
> +1 (only ENFILE "The system-wide limit on the total number of open files has been
> reached." which is probably not worth of implementing).
> ...
>
>> +static void setup(void)
>> +{
>> +	struct passwd *pw;
>> +
>> +	pw = SAFE_GETPWNAM("nobody");
>> +	uid = pw->pw_uid;
>> +
>> +	pidfd_open_supported();
>> +	pidfd_getfd_supported();
> nit: I'd put these before SAFE_GETPWNAM().
OK.
>
>> +
>> +	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
> If you wait for Cyril's patch adding auto stringification
> https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/
>
> you can use just:
> TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));
>
> and get more info.
I will look Cyril's patch and wait.
>
>> +	if (!TST_PASS)
>> +		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");
>
> @Cyril: would it be possible to to allow using also tst_brk() in macros in
> tst_test_macros.h?
>
Maybe can add SAFE_PIDFD_OPEN.
> Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated
>
>> +	valid_pidfd = TST_RET;
>> +}
>> +
>> +static void run(unsigned int n)
>> +{
>> +	struct tcase *tc =&tcases[n];
>> +	int pid;
>> +
>> +	switch (tc->exp_errno) {
>> +	case EPERM:
>> +		pid = SAFE_FORK();
> SAFE_FORK() can be before switch.
>
>> +		if (!pid) {
>> +			SAFE_SETUID(uid);
>> +			TST_EXP_FAIL2(pidfd_getfd(valid_pidfd, tc->targetfd, tc->flags),
>> +				tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
>> +				valid_pidfd, tc->targetfd, tc->flags, tc->name);
>> +			TST_CHECKPOINT_WAKE(0);
>> +			exit(0);
>> +		}
>> +		TST_CHECKPOINT_WAIT(0);
>> +		SAFE_WAIT(NULL);
>> +		return;
>> +	break;
>> +	case ESRCH:
>> +		pid = SAFE_FORK();
>> +		if (!pid) {
>> +			TST_CHECKPOINT_WAIT(0);
>> +			exit(0);
>> +		}
>> +		TST_EXP_FD_SILENT(pidfd_open(pid, 0), "pidfd_open");
>> +		*tc->pidfd = TST_RET;
>> +		TST_CHECKPOINT_WAKE(0);
>> +		SAFE_WAIT(NULL);
>> +	break;
>> +	default:
>> +	break;
>> +	};
>
> IMHO more readable would be instead of switch use if/else if or 2 functions.
>
Will try.

Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
>> +
>> +	TST_EXP_FAIL2(pidfd_getfd(*tc->pidfd, tc->targetfd, tc->flags),
>> +		tc->exp_errno, "pidfd_getfd(%d, %d, %d) with %s",
>> +		*tc->pidfd, tc->targetfd, tc->flags, tc->name);
>> +}

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[Petr Vorel]

Hi Xu,

> > and get more info.
> I will look Cyril's patch and wait.
Thx!

> >> +	if (!TST_PASS)
> >> +		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");

> > @Cyril: would it be possible to to allow using also tst_brk() in macros in
> > tst_test_macros.h?

> Maybe can add SAFE_PIDFD_OPEN.
I was thinking about it. But you use TFAIL - part of testing.
But using just SAFE_PIDFD_OPEN() even it's using TBROK might be best.

Kind regards,
Petr

> > Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Xu,

> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/.gitignore
> >> @@ -0,0 +1 @@
> >> +pidfd_getfd01
> > /pidfd_getfd01
> Thanks, will add it in v2.

> The two ways also work well.
/ causes pattern to be relative.  Because we have mostly unique files, we
wouldn't worry whether the pattern is relative or global. And we have 47
.gitignore files in testcases/ not following this rule :).

> ps:pidfd_open also miss "/". Shoud we add this rule in maintainer review 
> list and .gitignore info in doc?
I'm not sure myself if we want to follow this rule. If yes, it should be there.

Kind regards,
Petr

> Best Regards
> Yang Xu

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Richie, Xu,

> Hi Petr
> > Hi Xu,

> > ...
> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
> > ...
> >> +	remotefd = TST_RET;
> >> +	flag = fcntl(remotefd, F_GETFD);
> >> +	if (flag == -1)
> >> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
> > Just:
> > flag = SAFE_FCNTL(remotefd, F_GETFD);
> Yes, I almost forgot we have this macro.
@Richie: It'd be useful if sparse checks would suggest to use SAFE_*()
functions, but not sure if easily detectable. Something like setup() and
cleanup() function and syscall followed by if (foo == -1) followed by tst_brk().

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
>>> and get more info.
>> I will look Cyril's patch and wait.
> Thx!
The Cyril's patch seems only a single patch and doesn't affect the other 
TST_TEST macros. So how I just only use 
TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));?


>
>>>> +	if (!TST_PASS)
>>>> +		tst_brk(TFAIL | TTERRNO, "pidfd_open failed");
>
>>> @Cyril: would it be possible to to allow using also tst_brk() in macros in
>>> tst_test_macros.h?
>
>> Maybe can add SAFE_PIDFD_OPEN.
> I was thinking about it. But you use TFAIL - part of testing.
> But using just SAFE_PIDFD_OPEN() even it's using TBROK might be best.
My v2 just keep this.

ps: I also remove useless static expire_pidfd because we can use TST_RET 
as pidfd when tesing ESRCH error.

Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
>>> Having TST_*_BRK() (i.e. TST_EXP_FD_SILENT_BRK()) looks too complicated

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/5] lapi/kcmp.h: Replace GPL with SPDX-License-Identifier

[Petr Vorel]

Hi Xu,

> >>   int kcmp(int pid1, int pid2, int type, int fd1, int fd2)
> > $ make check-pidfd_getfd01
> > ../../../../include/lapi/kcmp.h:34:5: warning: Symbol 'kcmp' has no prototype or library ('tst_') prefix. Should it be static?

> > Could you please mark this as static inline?
> Of course, in fact, I have changed, but forgot to add it into the final 
> version.
+1

> > Not sure if this should be with the same commit as SPDX.
> IMO, I will add it into the same commit and mention that we also replace 
> GPL with SPDX. Title maybe changed into "lapi/kcmp.h: Simplify code ".
+1

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Xu,

...
> >> +	TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
> >> +	if (TST_RET != 0)
> >> +		tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description");
> > Maybe just:
> >         TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
> I think we can't use this macro here see below(kcmp manpage about return 
> value):
> 0 v1 is equal to v2; in other words, the two processes share the resource.

> 1   v1 is less than v2.

> 2   v1 is greater than v2.

> 3   v1 is not equal to v2, but ordering information is unavailable.

> On error, -1 is returned, and errno is set appropriately.

> So 1,2,3 are not invalid return value.
I'm sorry, you're right.

> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
> if (TST_RET==-1) {
> 	tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
> 	goto free;
> } else {
> 	if (TST_RET < 0 || TST_RET > 3)
+1 (very nit: instead of if/else I'd use 2x if and (TST_RET < -1 || TST_RET > 3)
- readability).

Kind regards,
Petr

> 	tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET);
> 	goto free;
> }

> free:
>   ....

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> ...
>>>> +	TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>>>> +	if (TST_RET != 0)
>>>> +		tst_res(TFAIL, "pidfd_getfd() didn't get the same open file description");
>>> Maybe just:
>>>          TST_EXP_PASS_SILENT(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>> I think we can't use this macro here see below(kcmp manpage about return
>> value):
>> 0 v1 is equal to v2; in other words, the two processes share the resource.
>
>> 1   v1 is less than v2.
>
>> 2   v1 is greater than v2.
>
>> 3   v1 is not equal to v2, but ordering information is unavailable.
>
>> On error, -1 is returned, and errno is set appropriately.
>
>> So 1,2,3 are not invalid return value.
> I'm sorry, you're right.
>
>> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>> if (TST_RET==-1) {
>> 	tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
>> 	goto free;
>> } else {
>> 	if (TST_RET<  0 || TST_RET>  3)
> +1 (very nit: instead of if/else I'd use 2x if and (TST_RET<  -1 || TST_RET>  3)
> - readability).
Good catch. I add the last "if" otherwise 1,2,3 are ignored.

TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
         if (TST_RET == -1) {
                 tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
                 goto free;
         }
         if (TST_RET < 0 || TST_RET > 3) {
                 tst_res(TFAIL, "kcmp invalid returns value(%d)", 
(int)TST_RET);
                 goto free;
         }
         if (TST_RET != 0) {
                 tst_res(TFAIL, "kcmp returns unexpected value(%d) 
instead of 0",
                                 (int)TST_RET);
                 goto free;
         }

         tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);

free:


Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
>> 	tst_res(TFAIL, "kcmp invalid return value %ld", TST_RET);
>> 	goto free;
>> }
>
>> free:
>>    ....

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Xu,

...
> >> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
> >> if (TST_RET==-1) {
> >> 	tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
> >> 	goto free;
> >> } else {
> >> 	if (TST_RET<  0 || TST_RET>  3)
> > +1 (very nit: instead of if/else I'd use 2x if and (TST_RET<  -1 || TST_RET>  3)
> > - readability).
> Good catch. I add the last "if" otherwise 1,2,3 are ignored.

> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>          if (TST_RET == -1) {
>                  tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
>                  goto free;
>          }
>          if (TST_RET < 0 || TST_RET > 3) {
>                  tst_res(TFAIL, "kcmp invalid returns value(%d)", 
> (int)TST_RET);
>                  goto free;
>          }
>          if (TST_RET != 0) {
>                  tst_res(TFAIL, "kcmp returns unexpected value(%d) 
> instead of 0",
>                                  (int)TST_RET);
>                  goto free;
>          }

>          tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);

FYI Unless we want to have explanations why it failed, we could use TST_EXP_VAL() or
TST_EXP_VAL_SILENT().

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[Petr Vorel]

Hi Xu,

...
> >> +	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
> > If you wait for Cyril's patch adding auto stringification
> > https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/

> > you can use just:
> > TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));

> > and get more info.
> I will look Cyril's patch and wait.

FYI Cyril is not planning to merge that patch, replaced by
https://lore.kernel.org/ltp/20220218103413.18540-1-chrubis@suse.cz/

But I'd still drop "pidfd_open", because
pidfd_getfd02.c:55: TFAIL: pidfd_open(getpid(), -9) failed: EINVAL (22)
is better than:
pidfd_getfd02.c:55: TFAIL: pidfd_open failed: EINVAL (22)

But as fanotify21.c also needs SAFE_PIDFD_OPEN() (and more tests will come in
the future I'd vote for adding SAFE_PIDFD_OPEN() as you suggested.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie, Xu,
>
>> Hi Petr
>> > Hi Xu,
>
>> > ...
>> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
>> > ...
>> >> +	remotefd = TST_RET;
>> >> +	flag = fcntl(remotefd, F_GETFD);
>> >> +	if (flag == -1)
>> >> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
>> > Just:
>> > flag = SAFE_FCNTL(remotefd, F_GETFD);
>> Yes, I almost forgot we have this macro.
> @Richie: It'd be useful if sparse checks would suggest to use SAFE_*()
> functions, but not sure if easily detectable. Something like setup() and
> cleanup() function and syscall followed by if (foo == -1) followed by tst_brk().
>
> Kind regards,
> Petr

Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not
sure, but it is one of the main use cases IMO.

Hopefully the IR produced by these code patterns is fairly stable. In
that case we can do some simple pattern matching.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Petr Vorel]

Hi Richie,

> Hello Petr,

> Petr Vorel <pvorel@suse.cz> writes:

> > Hi Richie, Xu,

> >> Hi Petr
> >> > Hi Xu,

> >> > ...
> >> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
> >> > ...
> >> >> +	remotefd = TST_RET;
> >> >> +	flag = fcntl(remotefd, F_GETFD);
> >> >> +	if (flag == -1)
> >> >> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
> >> > Just:
> >> > flag = SAFE_FCNTL(remotefd, F_GETFD);
> >> Yes, I almost forgot we have this macro.
> > @Richie: It'd be useful if sparse checks would suggest to use SAFE_*()
> > functions, but not sure if easily detectable. Something like setup() and
> > cleanup() function and syscall followed by if (foo == -1) followed by tst_brk().

> > Kind regards,
> > Petr

> Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not
> sure, but it is one of the main use cases IMO.
I remember few years ago (2017) metan did big cleanup with Coccinelle.
If much simpler, we might want to introduce some basic scripts (without
vendoring Coccinelle, it'd have to be installed).

> Hopefully the IR produced by these code patterns is fairly stable. In
> that case we can do some simple pattern matching.
Frankly speaking, if ever implemented it's probably you who is going to
implement it, thus your choice what you use :).

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[Richard Palethorpe]

Hello Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Richie,
>
>> Hello Petr,
>
>> Petr Vorel <pvorel@suse.cz> writes:
>
>> > Hi Richie, Xu,
>
>> >> Hi Petr
>> >> > Hi Xu,
>
>> >> > ...
>> >> >> +++ b/testcases/kernel/syscalls/pidfd_getfd/pidfd_getfd01.c
>> >> > ...
>> >> >> +	remotefd = TST_RET;
>> >> >> +	flag = fcntl(remotefd, F_GETFD);
>> >> >> +	if (flag == -1)
>> >> >> +		tst_brk(TFAIL | TERRNO, "fcntl(F_GETFD) failed");
>> >> > Just:
>> >> > flag = SAFE_FCNTL(remotefd, F_GETFD);
>> >> Yes, I almost forgot we have this macro.
>> > @Richie: It'd be useful if sparse checks would suggest to use SAFE_*()
>> > functions, but not sure if easily detectable. Something like setup() and
>> > cleanup() function and syscall followed by if (foo == -1) followed by tst_brk().
>
>> > Kind regards,
>> > Petr
>
>> Yeah, this would be relatively simple in Coccinelle. In Sparse I'm not
>> sure, but it is one of the main use cases IMO.
> I remember few years ago (2017) metan did big cleanup with Coccinelle.
> If much simpler, we might want to introduce some basic scripts (without
> vendoring Coccinelle, it'd have to be installed).

Indeed, we already have some in scripts/coccinelle. These are mainly
useful for refactoring.

>
>> Hopefully the IR produced by these code patterns is fairly stable. In
>> that case we can do some simple pattern matching.
> Frankly speaking, if ever implemented it's probably you who is going to
> implement it, thus your choice what you use :).
>
> Kind regards,
> Petr

Yeah, although it still needs reviewing :-p

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 4/5] Add pidfd_getfd01 test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> ...
>>>> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>>>> if (TST_RET==-1) {
>>>> 	tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
>>>> 	goto free;
>>>> } else {
>>>> 	if (TST_RET<   0 || TST_RET>   3)
>>> +1 (very nit: instead of if/else I'd use 2x if and (TST_RET<   -1 || TST_RET>   3)
>>> - readability).
>> Good catch. I add the last "if" otherwise 1,2,3 are ignored.
>
>> TEST(kcmp(getpid(), pid, KCMP_FILE, remotefd, targetfd));
>>           if (TST_RET == -1) {
>>                   tst_res(TFAIL | TTERRNO, "kcmp failed unexpectedly");
>>                   goto free;
>>           }
>>           if (TST_RET<  0 || TST_RET>  3) {
>>                   tst_res(TFAIL, "kcmp invalid returns value(%d)",
>> (int)TST_RET);
>>                   goto free;
>>           }
>>           if (TST_RET != 0) {
>>                   tst_res(TFAIL, "kcmp returns unexpected value(%d)
>> instead of 0",
>>                                   (int)TST_RET);
>>                   goto free;
>>           }
>
>>           tst_res(TPASS, "pidfd_getfd(%d, %d, 0) passed", pidfd, targetfd);
>
> FYI Unless we want to have explanations why it failed, we could use TST_EXP_VAL() or
> TST_EXP_VAL_SILENT().
Ok, I will use TST_EXP_VAL_SILENT.

Best Regards
Yang Xu
>
> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 5/5] syscalls/pidfd_getfd02: add basic error test

[xuyang2018.jy]

Hi Petr
> Hi Xu,
>
> ...
>>>> +	TST_EXP_FD_SILENT(pidfd_open(getpid(), 0), "pidfd_open");
>>> If you wait for Cyril's patch adding auto stringification
>>> https://lore.kernel.org/ltp/20220217142730.19726-1-chrubis@suse.cz/
>
>>> you can use just:
>>> TST_EXP_FD_SILENT(pidfd_open(getpid(), 0));
>
>>> and get more info.
>> I will look Cyril's patch and wait.
>
> FYI Cyril is not planning to merge that patch, replaced by
> https://lore.kernel.org/ltp/20220218103413.18540-1-chrubis@suse.cz/
>
> But I'd still drop "pidfd_open", because
> pidfd_getfd02.c:55: TFAIL: pidfd_open(getpid(), -9) failed: EINVAL (22)
> is better than:
> pidfd_getfd02.c:55: TFAIL: pidfd_open failed: EINVAL (22)
>
> But as fanotify21.c also needs SAFE_PIDFD_OPEN() (and more tests will come in
> the future I'd vote for adding SAFE_PIDFD_OPEN() as you suggested.
I will add SAFE_PIDFD_OPEN into lapi/pidfd_open.h. But it seems kernel 
doesn't have pidfd_getfd.h/pidfd_send_signal.h/pidfd_open.h, I think we 
can merge them into lapi/pidfd.h. So in the future, we can introduce 
other pidfd macro and case wants to use these pidfd macro they just only 
include one header(lapi/pidfd.h). What do you think about this?

Best Regards
Yang Xu
>
> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp