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

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

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