[PATCH v1] tests: extend coverage for fcntl options and flags
Dmitry V. Levin
ldv at altlinux.org
Mon Jun 4 10:29:09 UTC 2018
On Mon, May 28, 2018 at 12:56:13AM +0800, Zhibin Li wrote:
> * tests/fcntl64.c: Test short read of struct flock.
> * tests/struct_flock.c: Likewise.
> * tests/fcntl.c: Include assert.h.
> (struct fcntl_cmd_check): New struct definition.
> (print_retval_flags, test_set_cmd, test_get_cmd,
> print_flags_getfd, print_flags_getfl, print_flags_getlease,
> print_flags_getseals, print_flags_getsig, test_fcntl): New functions.
> (main): Use test_fcntl.
> ---
> tests/fcntl.c | 226 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/fcntl64.c | 20 +++--
> tests/struct_flock.c | 20 +++--
> 3 files changed, 252 insertions(+), 14 deletions(-)
>
> diff --git a/tests/fcntl.c b/tests/fcntl.c
> index 45c20d9d..7999f81f 100644
> --- a/tests/fcntl.c
> +++ b/tests/fcntl.c
> @@ -28,6 +28,7 @@
>
> #include "tests.h"
> #include <asm/unistd.h>
> +#include <assert.h>
>
> #ifdef __NR_fcntl
>
> @@ -67,6 +68,9 @@ test_flock64(void)
> #if !defined(F_SETOWN_EX) || F_SETOWN_EX != F_GETLK64
> TEST_FLOCK64_EINVAL(F_GETLK64);
> #endif
> +#ifdef F_OFD_GETLK
> + TEST_FLOCK64_EINVAL(F_OFD_GETLK);
> +#endif
> }
There is a check for F_OFD_GETLK added by commit v4.22-291-gd476d26.
> /*
> @@ -137,6 +141,227 @@ test_f_owner_ex(void)
> }
> #endif /* TEST_F_OWNER_EX */
>
> +struct fcntl_cmd_check {
> + int fd;
> + int cmd;
> + const char *cmd_str;
> + int arg;
> + const char *arg_str;
> + void (*print_flags)(long rc);
> +};
> +
> +static void
> +print_retval_flags(const struct fcntl_cmd_check *check, long rc)
> +{
> + if (check->print_flags) {
> + check->print_flags(rc);
> + } else {
> + printf("%s", sprintrc(rc));
I wouldn't recommend calling sprintrc() this way because errno may have
already been clobbered. Let's just print errstr instead (available
since commit v4.22-290-gb6542c5).
> + }
> + printf("\n");
> +}
> +
> +static void
> +test_set_cmd(const struct fcntl_cmd_check *check)
> +{
> + long rc = syscall(TEST_SYSCALL_NR, check->fd, check->cmd, check->arg);
Starting with commit v4.22-289-g419da8e you can use invoke_test_syscall()
instead of manual syscall invocation.
> + printf("%s(%d, %s, %s) = %s\n",
> + TEST_SYSCALL_STR, check->fd, check->cmd_str,
> + check->arg_str, sprintrc(rc));
> +
> + /*bad file fd*/
Please put a space after /* and before */
> + syscall(TEST_SYSCALL_NR, -1, check->cmd, check->arg);
> + printf("%s(-1, %s, %s) = -1 EBADF (Bad file descriptor)\n",
Please do not hardcode "Bad file descriptor", it may differ between libcs.
> + TEST_SYSCALL_STR, check->cmd_str, check->arg_str);
> +}
> +
> +static void
> +test_get_cmd(const struct fcntl_cmd_check *check)
> +{
> + long rc = syscall(TEST_SYSCALL_NR, check->fd, check->cmd);
> + printf("%s(%d, %s) = ",
> + TEST_SYSCALL_STR, check->fd, check->cmd_str);
> + print_retval_flags(check, rc);
> +
> + /*bad file fd*/
> + syscall(TEST_SYSCALL_NR, -1, check->cmd);
> + printf("%s(-1, %s) = -1 EBADF (Bad file descriptor)\n",
> + TEST_SYSCALL_STR, check->cmd_str);
> +}
All comments about test_set_cmd apply here as well.
> +static void
> +print_flags_getfd(long rc)
> +{
> + assert(rc >= 0);
> + const char *text;
> + text = rc & 1 ? " (flags FD_CLOEXEC)" : "";
> + printf("%#lx%s", rc, text);
> +}
> +
> +static void
> +print_flags_getfl(long rc)
> +{
> + assert(rc >= 0);
> + const char *text;
> +
> + switch (rc) {
> + case 00:
> + text = "O_RDONLY";
> + break;
> + case 01:
> + text = "O_WRONLY";
> + break;
> + case 02:
> + text = "O_RDWR";
> + break;
> + case 0100000|00:
> + text = "O_RDONLY|O_LARGEFILE";
> + break;
> + case 0100000|01:
> + text = "O_WRONLY|O_LARGEFILE";
> + break;
> + case 0100000|02:
> + text = "O_RDWR|O_LARGEFILE";
> + break;
I don't think hardcoding these constants is a good idea, especially
O_LARGEFILE which is known to be defined to different values on many
architectures.
> + default:
> + error_msg_and_fail("fcntl returned %#lx, does the"
> + " test have to be updated?", rc);
> + }
> + printf("%#lx (flags %s)", rc, text);
> +}
> +
> +static void
> +print_flags_getlease(long rc)
> +{
> + assert(rc >= 0);
> + const char *text;
> +
> + switch (rc) {
> + case 0:
> + text = "F_RDLCK";
> + break;
> + case 1:
> + text = "F_WRLCK";
> + break;
> + case 2:
> + text = "F_UNLCK";
> + break;
> + default:
> + error_msg_and_fail("fcntl returned %#lx, does the"
> + " test have to be updated?", rc);
> + }
> + printf("%#lx (%s)", rc, text);
> +}
> +
> +#ifdef HAVE_LINUX_MEMFD_H
> +static void
> +print_flags_getseals(long rc)
> +{
> + assert(rc >= 0);
> + const char *text;
> + if (!rc) {
> + text = "";
> + printf("%ld", rc);
> + } else {
> + switch (rc) {
> + case 0x0001:
> + text = "F_SEAL_SEAL";
> + break;
> + case 0x0002:
> + text = "F_SEAL_SHRINK";
> + break;
> + case 0x0004:
> + text = "F_SEAL_GROW";
> + break;
> + case 0x0008:
> + text = "F_SEAL_WRITE";
> + break;
> + default:
> + error_msg_and_fail("fcntl returned %#lx, does the"
> + " test have to be updated?", rc);
> + }
> + printf("%#lx (seals %s)", rc, text);
> + }
> +}
> +#endif
> +
> +static void
> +print_flags_getsig(long rc)
> +{
> + assert(rc >= 0);
> + const char *text;
> +
> + if (!rc) {
> + text = "";
> + printf("%ld", rc);
> + } else {
> + switch (rc) {
> + case 1:
> + text = "SIGHUP";
> + break;
> + default:
> + error_msg_and_fail("fcntl returned %#lx, does the"
> + " test have to be updated?", rc);
> + }
I think you can use signal2name() here.
> + printf("%ld (%s)", rc, text);
> + }
> +}
> +
> +static void
> +test_fcntl(void)
This name is too generic for the test that is all about fcntl.
> +{
> +#ifdef __NR_memfd_create
> +
> +# ifdef HAVE_LINUX_MEMFD_H
> +# include <linux/memfd.h>
> +# endif
> + int memfd = syscall(__NR_memfd_create, "seal.sample", MFD_ALLOW_SEALING);
Where MFD_ALLOW_SEALING is going to be defined when <linux/memfd.h> is not
included?
> + struct fcntl_cmd_check check_seals[] = {
> + { memfd, ARG_STR(F_ADD_SEALS),
> + ARG_STR(0) },
> + { memfd, ARG_STR(F_ADD_SEALS),
> + ARG_STR(F_SEAL_SEAL) },
> + { memfd, ARG_STR(F_GET_SEALS),
> + .print_flags = print_flags_getseals} };
> + test_set_cmd(&check_seals[0]);
> + test_get_cmd(&check_seals[2]);
> + test_set_cmd(&check_seals[1]);
> + test_get_cmd(&check_seals[2]);
> +#endif
> + static const struct fcntl_cmd_check set_checks[] = {
> + { 0, ARG_STR(F_SETFD), ARG_STR(FD_CLOEXEC) },
> + { 0, ARG_STR(F_SETOWN), ARG_STR(20) },
> + { 0, ARG_STR(F_SETPIPE_SZ), ARG_STR(4097) },
> + { 0, ARG_STR(F_DUPFD), ARG_STR(0) },
> + { 0, ARG_STR(F_DUPFD_CLOEXEC), ARG_STR(0) },
> + { 0, ARG_STR(F_SETFL), ARG_STR(O_RDWR|O_LARGEFILE) },
> + { 0, ARG_STR(F_NOTIFY), ARG_STR(DN_ACCESS) },
> + { 1, ARG_STR(F_SETLEASE), ARG_STR(F_RDLCK) },
> + { 0, ARG_STR(F_SETSIG), 0, "SIG_0" },
> + { 1, ARG_STR(F_SETSIG), 1, "SIGHUP" }
> + };
> +
> + static const struct fcntl_cmd_check get_checks[] = {
> + { 0, ARG_STR(F_GETFD), .print_flags = print_flags_getfd },
> + { 1, ARG_STR(F_GETFD), .print_flags = print_flags_getfd },
> + { 0, ARG_STR(F_GETOWN) },
> + { 0, ARG_STR(F_GETPIPE_SZ) },
> + { 0, ARG_STR(F_GETFL), .print_flags = print_flags_getfl },
> + { 1, ARG_STR(F_GETLEASE), .print_flags = print_flags_getlease },
> + { 0, ARG_STR(F_GETSIG), .print_flags = print_flags_getsig },
> + { 1, ARG_STR(F_GETSIG), .print_flags = print_flags_getsig }
> + };
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(set_checks); i++) {
> + test_set_cmd(set_checks + i);
> + }
> +
> + for (unsigned int j = 0; j < ARRAY_SIZE(get_checks); j++) {
> + test_get_cmd(get_checks + j);
> + }
> +
> +}
> +
> int
> main(void)
> {
> @@ -146,6 +371,7 @@ main(void)
> #ifdef TEST_F_OWNER_EX
> test_f_owner_ex();
> #endif
> + test_fcntl();
I think all these new tests should go to fcntl-common.c to cover fcntl64.
> puts("+++ exited with 0 +++");
> return 0;
> diff --git a/tests/fcntl64.c b/tests/fcntl64.c
> index 068956ed..c7da0a44 100644
> --- a/tests/fcntl64.c
> +++ b/tests/fcntl64.c
> @@ -40,15 +40,21 @@
> static void
> test_flock64_einval(const int cmd, const char *name)
> {
> - struct_kernel_flock64 fl = {
> - .l_type = F_RDLCK,
> - .l_start = 0xdefaced1facefeedULL,
> - .l_len = 0xdefaced2cafef00dULL
> - };
> - long rc = invoke_test_syscall(cmd, &fl);
> + TAIL_ALLOC_OBJECT_CONST_PTR(struct_kernel_flock64, fl);
> + fl->l_type = F_RDLCK;
> + fl->l_whence = SEEK_SET;
> + fl->l_start = 0xdefaced1facefeedULL;
> + fl->l_len = 0xdefaced2cafef00dULL;
> + long rc;
> + rc = invoke_test_syscall(cmd, fl);
> printf("%s(0, %s, {l_type=F_RDLCK, l_whence=SEEK_SET"
> ", l_start=%jd, l_len=%jd}) = %s\n", TEST_SYSCALL_STR, name,
> - (intmax_t) fl.l_start, (intmax_t) fl.l_len, sprintrc(rc));
> + (intmax_t) fl->l_start, (intmax_t) fl->l_len, sprintrc(rc));
> +
> + void *bad_addr = (void *) fl + 1;
> + rc = invoke_test_syscall(cmd, bad_addr);
> + printf("%s(0, %s, %p) = %s\n",
> + TEST_SYSCALL_STR, name, bad_addr, sprintrc(rc));
> }
>
> static void
> diff --git a/tests/struct_flock.c b/tests/struct_flock.c
> index 428c038d..3c5007fa 100644
> --- a/tests/struct_flock.c
> +++ b/tests/struct_flock.c
> @@ -54,15 +54,21 @@ invoke_test_syscall(const unsigned int cmd, void *const p)
> static void
> test_flock_einval(const int cmd, const char *name)
> {
> - struct_kernel_flock fl = {
> - .l_type = F_RDLCK,
> - .l_start = (TYPEOF_FLOCK_OFF_T) 0xdefaced1facefeedULL,
> - .l_len = (TYPEOF_FLOCK_OFF_T) 0xdefaced2cafef00dULL
> - };
> - long rc = invoke_test_syscall(cmd, &fl);
> + TAIL_ALLOC_OBJECT_CONST_PTR(struct_kernel_flock, fl);
> + fl->l_type = F_RDLCK;
> + fl->l_whence = SEEK_SET;
> + fl->l_start = (TYPEOF_FLOCK_OFF_T) 0xdefaced1facefeedULL;
> + fl->l_len = (TYPEOF_FLOCK_OFF_T) 0xdefaced2cafef00dULL;
> + long rc;
> + rc = invoke_test_syscall(cmd, fl);
> printf("%s(0, %s, {l_type=F_RDLCK, l_whence=SEEK_SET"
> ", l_start=%jd, l_len=%jd}) = %s\n", TEST_SYSCALL_STR, name,
> - (intmax_t) fl.l_start, (intmax_t) fl.l_len, sprintrc(rc));
> + (intmax_t) fl->l_start, (intmax_t) fl->l_len, sprintrc(rc));
> +
> + void *bad_addr = (void *) fl + 1;
> + rc = invoke_test_syscall(cmd, bad_addr);
> + printf("%s(0, %s, %p) = %s\n",
> + TEST_SYSCALL_STR, name, bad_addr, sprintrc(rc));
> }
>
> static void
Thanks, I've applied the last hunk as commit v4.22-293-g08b2c1b.
--
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20180604/ed4b16f7/attachment.bin>
More information about the Strace-devel
mailing list