<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 5/4/20 1:09 PM, Viresh Kumar wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20200504050937.oassdcfg4x5zh4nm@vireshk-i7">
<pre class="moz-quote-pre" wrap="">On 30-04-20, 16:57, Xiao Yang wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">pidfd_open(2) will set close-on-exec flag on the file descriptor as it
manpage states, so check close-on-exec flag by fcntl(2).
Also avoid compiler warning by replacing (long) TST_RET with (int) pidfd:
------------------------------------------------------
In file included from pidfd_open01.c:9:
pidfd_open01.c: In function ‘run’:
../../../../include/tst_test.h:76:41: warning: format ‘%i’ expects argument of type ‘int’, but argument 5 has type ‘long int’ [-Wformat=]
76 | tst_brk_(__FILE__, __LINE__, (ttype), (arg_fmt), ##__VA_ARGS__);\
| ^~~~~~~~~
../../../../include/tst_safe_macros.h:224:5: note: in expansion of macro ‘tst_brk’
224 | tst_brk(TBROK | TERRNO, \
| ^~~~~~~
pidfd_open01.c:20:9: note: in expansion of macro ‘SAFE_FCNTL’
20 | flag = SAFE_FCNTL(TST_RET, F_GETFD);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This log isn't useful as the warning started coming after your change
only and not before.</pre>
</blockquote>
<p>Hi Viresh,</p>
<p>Thanks for your review.</p>
<p>Right，just add a hint why I use pidfd instead so I want to keep
it.</p>
<p>Of course，I will <span style="background-color: transparent;">mention
that </span><span style="background-color: transparent;"><span
style="background-color: transparent;">my change </span>introduces
the compiler warning in v2 patch.</span></p>
<blockquote type="cite"
cite="mid:20200504050937.oassdcfg4x5zh4nm@vireshk-i7">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">------------------------------------------------------
Signed-off-by: Xiao Yang <a class="moz-txt-link-rfc2396E" href="mailto:yangx.jy@cn.fujitsu.com"><yangx.jy@cn.fujitsu.com></a>
---
.../kernel/syscalls/pidfd_open/pidfd_open01.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
index 93bb86687..293e93b63 100644
--- a/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
+++ b/testcases/kernel/syscalls/pidfd_open/pidfd_open01.c
@@ -6,17 +6,27 @@
* Basic pidfd_open() test, fetches the PID of the current process and tries to
* get its file descriptor.
*/
+
+#include <sys/types.h>
+#include <unistd.h>
+#include <fcntl.h>
#include "tst_test.h"
#include "lapi/pidfd_open.h"
static void run(void)
{
- TEST(pidfd_open(getpid(), 0));
+ int pidfd = 0, flag = 0;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
None of these need to be initialized.</pre>
</blockquote>
<p>Initialization or not initialization are both fine for me.</p>
<p>Do you have any strong reason to drop Initialization?</p>
<blockquote type="cite"
cite="mid:20200504050937.oassdcfg4x5zh4nm@vireshk-i7">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ pidfd = pidfd_open(getpid(), 0);
+ if (pidfd == -1)
+ tst_brk(TFAIL | TERRNO, "pidfd_open(getpid(), 0) failed");
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This could have been written as:
TEST(pidfd = pidfd_open(getpid(), 0));</pre>
</blockquote>
<p>Why do you want to keep TEST()? I don't think it is necessary:</p>
<p>1) pidfd and TERRNO are enough to check return value and errno.</p>
<p>2) It is OK for testcase to not use TEST().</p>
<p>Thanks,</p>
<p>Xiao Yang<br>
</p>
<blockquote type="cite"
cite="mid:20200504050937.oassdcfg4x5zh4nm@vireshk-i7">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+ flag = SAFE_FCNTL(pidfd, F_GETFD);
- if (TST_RET == -1)
- tst_brk(TFAIL | TTERRNO, "pidfd_open(getpid(), 0) failed");
+ SAFE_CLOSE(pidfd);
- SAFE_CLOSE(TST_RET);
+ if (!(flag & FD_CLOEXEC))
+ tst_brk(TFAIL, "pidfd_open(getpid(), 0) didn't set close-on-exec flag");
tst_res(TPASS, "pidfd_open(getpid(), 0) passed");
}
--
2.21.0
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>