On Mon, 2023-09-25 at 16:00 +0500, Stas Sergeev wrote:
> F_UNLCK can be used with F_OFD_GETLK to get the locks from
> particular fd. This patch adds testing for that functionality.
>
> The following options are added to t_ofd_locks:
> -u selects F_UNLCK for testing
> -G sets the unix socket name for fd receive
> -S sets the unix socket name for fd send
> -U sets the message to be printed if the requested functionality
> unsupported.
>
> Test works as follows:
> - locker process sets the lock and sends an fd via unix socket
> - lock-tester process receives an fd and uses F_UNLCK to check the lock
> - the test is provided with a "success" message that it should print
> only if the tested functionality is not supported. In that case
> printing a "success" message avoids the test failure on older kernels.
>
> To allow passing the "success" messages with spaces, eval has to be
> used in a shell script when starting the test case.
>
> CC: fstests@xxxxxxxxxxxxxxx
> CC: Murphy Zhou <xzhou@xxxxxxxxxx>
> CC: Jeff Layton <jlayton@xxxxxxxxxx>
> CC: Zorro Lang <zlang@xxxxxxxxxx>
>
> Signed-off-by: Stas Sergeev <stsp2@xxxxxxxxx>
> ---
> src/t_ofd_locks.c | 183 +++++++++++++++++++++++++++++++++++++++---
> tests/generic/478 | 24 ++++--
> tests/generic/478.out | 15 ++++
> 3 files changed, 204 insertions(+), 18 deletions(-)
>
Sorry for the long review delay. It's been a busy cycle.
> diff --git a/src/t_ofd_locks.c b/src/t_ofd_locks.c
> index 58cb0959..a78fd575 100644
> --- a/src/t_ofd_locks.c
> +++ b/src/t_ofd_locks.c
> @@ -14,6 +14,8 @@
> #include <sys/wait.h>
> #include <sys/ipc.h>
> #include <sys/sem.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>
> /*
> * In distributions that do not have these macros ready in glibc-headers,
> @@ -134,7 +136,7 @@ static void usage(char *arg0)
> printf("\t-F : clone with CLONE_FILES in setlk to setup test condition\n");
> printf("\t-d : dup and close in setlk\n");
> printf("\twithout both -F/d, use clone without CLONE_FILES\n");
> - printf("\t-r/-w : set/get rdlck/wrlck\n");
> + printf("\t-r/-w/-u : set/get rdlck/wrlck/unlck\n");
No explanation of -G/-S/-U ?
> printf("\t-o num : offset start to lock, default 0\n");
> printf("\t-l num : lock length, default 10\n");
> printf("\t-R/-W : open file RDONLY/RDWR\n\n");
> @@ -164,6 +166,117 @@ static int child_fn(void* p)
> return 0;
> }
>
> +static int connect_unix(const char *sname)
> +{
> + struct sockaddr_un addr = { .sun_family = AF_UNIX };
> +
> + int unix_sock = socket(AF_UNIX, SOCK_DGRAM, 0);
> + if (unix_sock == -1)
> + return -1;
> +
> + strncpy(addr.sun_path, sname, sizeof(addr.sun_path) - 1);
> + if (connect(unix_sock, (struct sockaddr *)&addr, sizeof(addr)) == -1)
> + {
> + close(unix_sock);
> + perror("connect()");
> + return -1;
> + }
> +
> + return unix_sock;
> +}
> +
> +static int send_fd(const char *sname, int fd)
> +{
> + int ret;
> + int unix_sock;
> + struct iovec iov = {.iov_base = ":)", // Must send at least one byte
> + .iov_len = 2};
> +
> + union {
> + char buf[CMSG_SPACE(sizeof(fd))];
> + struct cmsghdr align;
> + } u;
> +
> + struct msghdr msg = {.msg_iov = &iov,
> + .msg_iovlen = 1,
> + .msg_control = u.buf,
> + .msg_controllen = sizeof(u.buf)};
> +
> + struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
> + *cmsg = (struct cmsghdr){.cmsg_level = SOL_SOCKET,
> + .cmsg_type = SCM_RIGHTS,
> + .cmsg_len = CMSG_LEN(sizeof(fd))};
> +
> + memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
> +
> + unix_sock = connect_unix(sname);
> + if (unix_sock == -1) {
> + perror("sendmsg()");
> + return -1;
> + }
> + ret = sendmsg(unix_sock, &msg, 0);
> + close(unix_sock);
> + if (ret == -1)
> + perror("sendmsg()");
> + return ret;
> +}
> +
> +static int bind_unix(const char *sname)
> +{
> + struct sockaddr_storage storage = {};
> + struct sockaddr_un *addr;
> + int sock;
> +
> + sock = socket(AF_UNIX, SOCK_DGRAM, 0);
> + if (sock == -1)
> + return -1;
> + addr = (struct sockaddr_un *)&storage;
> + addr->sun_family = AF_UNIX;
> + strncpy(addr->sun_path, sname, sizeof(addr->sun_path) - 1);
> + unlink(sname);
> + if (bind(sock, (struct sockaddr *)addr, SUN_LEN(addr)) == -1) {
> + perror("bind()");
> + close(sock);
> + return -1;
> + }
> + return sock;
> +}
> +
> +static int recv_fd(int sock)
> +{
> + struct msghdr msg;
> + struct cmsghdr *cmsghdr;
> + struct iovec iov;
> + ssize_t nbytes;
> + int *p;
> + char buf[CMSG_SPACE(sizeof(int))], c[16];
> + struct cmsghdr *cmsgp;
> +
> + iov.iov_base = &c;
> + iov.iov_len = sizeof(c);
> + cmsghdr = (struct cmsghdr *)buf;
> + cmsghdr->cmsg_len = CMSG_LEN(sizeof(int));
> + cmsghdr->cmsg_level = SOL_SOCKET;
> + cmsghdr->cmsg_type = SCM_RIGHTS;
> + msg.msg_name = NULL;
> + msg.msg_namelen = 0;
> + msg.msg_iov = &iov;
> + msg.msg_iovlen = 1;
> + msg.msg_control = cmsghdr;
> + msg.msg_controllen = CMSG_LEN(sizeof(int));
> + msg.msg_flags = 0;
> +
> + nbytes = recvmsg(sock, &msg, 0);
> + if (nbytes == -1)
> + return -1;
> +
> + cmsgp = CMSG_FIRSTHDR(&msg);
> + p = (int *)CMSG_DATA(cmsgp);
> + if (!p)
> + return -1;
> + return *p;
> +}
> +
> int main(int argc, char **argv)
> {
> int posix = 0;
> @@ -184,10 +297,14 @@ int main(int argc, char **argv)
> struct semid_ds sem_ds;
> struct sembuf sop;
> int opt, ret;
> + int rcv_sock = -1;
> + const char *snd_sock_name = NULL;
> + const char *unsup_msg = NULL;
> + int unlck_unsup = 0;
>
> //avoid libcap errno bug
> errno = 0;
> - while((opt = getopt(argc, argv, "sgrwo:l:PRWtFdK:")) != -1) {
> + while((opt = getopt(argc, argv, "sgrwuo:l:PRWtFdK:G:S:U:")) != -1) {
> switch(opt) {
> case 's':
> lock_cmd = 1;
> @@ -201,6 +318,9 @@ int main(int argc, char **argv)
> case 'w':
> lock_rw = 1;
> break;
> + case 'u':
> + lock_rw = 2;
> + break;
> case 'o':
> lock_start = atoi(optarg);
> break;
> @@ -228,6 +348,15 @@ int main(int argc, char **argv)
> case 'K':
> semkey = strtol(optarg, NULL, 16);
> break;
> + case 'G':
> + rcv_sock = bind_unix(optarg);
> + break;
> + case 'S':
> + snd_sock_name = optarg;
> + break;
> + case 'U':
> + unsup_msg = optarg;
> + break;
> default:
> usage(argv[0]);
> return -1;
> @@ -256,17 +385,27 @@ int main(int argc, char **argv)
> getlk_macro = F_GETLK;
> }
>
> - if (lock_rw == 1)
> + switch (lock_rw) {
> + case 1:
> flk.l_type = F_WRLCK;
> - else
> + break;
> + case 0:
> flk.l_type = F_RDLCK;
> + break;
> + case 2:
> + flk.l_type = F_UNLCK;
> + break;
> + }
>
nit: fix the ordering above? Not sure why you decided to do 1,0,2...
> - if (open_rw == 0)
> - fd = open(argv[optind], O_RDONLY);
> - else
> - fd = open(argv[optind], O_RDWR);
> - if (fd == -1)
> - err_exit("open", errno);
> + if (rcv_sock == -1) {
> + if (open_rw == 0) {
> + fd = open(argv[optind], O_RDONLY);
> + } else {
> + fd = open(argv[optind], O_RDWR);
> + }
> + if (fd == -1)
> + err_exit("open", errno);
> + }
>
> /*
> * In a testun, we do a fcntl getlk call and exit
> @@ -322,6 +461,9 @@ int main(int argc, char **argv)
> if (fcntl(fd, setlk_macro, &flk) < 0)
> err_exit("setlkw", errno);
>
> + if (snd_sock_name)
> + send_fd(snd_sock_name, fd);
> +
> if (use_dup == 1) {
> /* dup fd and close the newfd */
> int dfd = dup(fd);
> @@ -400,13 +542,30 @@ int main(int argc, char **argv)
> if (semtimedop(semid, &sop, 1, &ts) == -1)
> err_exit("wait sem0 0", errno);
>
> - if (fcntl(fd, getlk_macro, &flk) < 0)
> - err_exit("getlk", errno);
> + if (rcv_sock != -1) {
> + fd = recv_fd(rcv_sock);
> + close(rcv_sock);
> + if (fd == -1)
> + err_exit("SCM_RIGHTS", errno);
> + }
> +
> + if (fcntl(fd, getlk_macro, &flk) < 0) {
> + if (lock_rw == 2 && errno == EINVAL)
> + unlck_unsup++;
> + else
> + err_exit("getlk", errno);
> + }
>
> /* set sem1 = 0 (getlk done) */
> semu.val = 0;
> if (semctl(semid, 1, SETVAL, semu) == -1)
> err_exit("set sem1 0", errno);
> + if (unlck_unsup) {
> + if (unsup_msg)
> + puts(unsup_msg);
> + close(fd);
> + return 0;
> + }
>
> /* check result */
> switch (flk.l_type) {
> diff --git a/tests/generic/478 b/tests/generic/478
> index 419acc94..1399aa6f 100755
> --- a/tests/generic/478
> +++ b/tests/generic/478
> @@ -128,9 +128,9 @@ do_test()
> soptions="$1 -K $SEMKEY"
> goptions="$2 -K $SEMKEY"
> # -s : do setlk
> - $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> + eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> # -g : do getlk
> - $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> + eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> tee -a $seqres.full
> wait $!
> rm_sem
> @@ -140,8 +140,8 @@ do_test()
> soptions="$1 -F -K $SEMKEY"
> goptions="$2 -K $SEMKEY"
> # with -F, new locks are always file to place
> - $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> - $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> + eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> + eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> tee -a $seqres.full
> wait $!
> rm_sem
> @@ -150,8 +150,8 @@ do_test()
> # add -d to dup and close
> soptions="$1 -d -K $SEMKEY"
> goptions="$2 -K $SEMKEY"
> - $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> - $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> + eval $here/src/t_ofd_locks $soptions $TEST_DIR/testfile &
> + eval $here/src/t_ofd_locks $goptions $TEST_DIR/testfile | \
> tee -a $seqres.full
> wait $!
> rm_sem
> @@ -197,6 +197,11 @@ do_test "-s -w -o 0 -l 10 -P" "-g -r -o 5 -l 20" "wrlck" "unlck" "unlck"
> # setlk posix wrlck [0,9], getlk rdlck [20,29]
> do_test "-s -w -o 0 -l 10 -P" "-g -r -o 20 -l 10" "unlck" "unlck" "unlck"
>
> +# setlk wrlck [0,9], getlk unlck [0,9]
> +do_test "-s -w -o 0 -l 10 -W -S /tmp/fstests.sock" "-g -u -o 0 -l 10 -W -G /tmp/fstests.sock -U get\ wrlck" "wrlck" "wrlck" "wrlck"
> +# setlk wrlck [0,9], getlk unlck [20,29]
> +do_test "-s -w -o 0 -l 10 -S /tmp/fstests.sock" "-g -u -o 20 -l 10 -G /tmp/fstests.sock -U lock\ could\ be\ placed" "unlck" "unlck" "unlck"
> +
> # setlk rdlck [0,9], getlk wrlck [0,9], open RDONLY
> do_test "-s -r -o 0 -l 10 -R" "-g -w -o 0 -l 10 -R" "rdlck" "unlck" "rdlck"
> # setlk rdlck [0,9], getlk wrlck [5,24], open RDONLY
> @@ -204,6 +209,13 @@ do_test "-s -r -o 0 -l 10 -R" "-g -w -o 5 -l 20 -R -P" "rdlck" "unlck" "rdlck"
> # setlk posix rdlck [0,9], getlk wrlck [5,24], open RDONLY
> do_test "-s -r -o 0 -l 10 -R -P" "-g -w -o 5 -l 20 -R" "rdlck" "unlck" "unlck"
>
> +# setlk rdlck [0,9], getlk unlck [0,9], open RDONLY
> +do_test "-s -r -o 0 -l 10 -R -S /tmp/fstests.sock" "-g -u -o 0 -l 10 -R -G /tmp/fstests.sock -U get\ rdlck" "rdlck" "rdlck" "rdlck"
> +# setlk posix rdlck [0,9], getlk unlck [5,24], open RDONLY
> +do_test "-s -r -o 0 -l 10 -R -S /tmp/fstests.sock" "-g -u -o 5 -l 20 -R -G /tmp/fstests.sock -U get\ rdlck" "rdlck" "rdlck" "rdlck"
> +# setlk rdlck [0,9], getlk unlck [20,29], open RDONLY
> +do_test "-s -r -o 0 -l 10 -R -S /tmp/fstests.sock" "-g -u -o 20 -l 10 -R -G /tmp/fstests.sock -U lock\ could\ be\ placed" "unlck" "unlck" "unlck"
> +
> # setlk rdlck [0,9], getlk wrlck [0,9]
> do_test "-s -r -o 0 -l 10" "-g -w -o 0 -l 10" "rdlck" "unlck" "rdlck"
> # setlk rdlck [0,9], getlk posix wrlck [5,24]
> diff --git a/tests/generic/478.out b/tests/generic/478.out
> index 0b4b344a..f3ca2473 100644
> --- a/tests/generic/478.out
> +++ b/tests/generic/478.out
> @@ -29,6 +29,12 @@ lock could be placed
> lock could be placed
> lock could be placed
> lock could be placed
> +get wrlck
> +get wrlck
> +get wrlck
> +lock could be placed
> +lock could be placed
> +lock could be placed
> get rdlck
> lock could be placed
> get rdlck
> @@ -39,6 +45,15 @@ get rdlck
> lock could be placed
> lock could be placed
> get rdlck
> +get rdlck
> +get rdlck
> +get rdlck
> +get rdlck
> +get rdlck
> +lock could be placed
> +lock could be placed
> +lock could be placed
> +get rdlck
> lock could be placed
> get rdlck
> get rdlck
The problem here is that old kernels are going to fail the new tests,
and it's going to be hard to engineer the output of 478 to account for
that.
I think instead of extending generic/478, you should add these in a new
testcase, and roll a _requires_ofd_getlk_unlck test or something that
allows older kernels to skip this test. The modifications to
t_ofd_locks.c look fine overall though.
--
Jeff Layton <jlayton@xxxxxxxxxx>
