[LTP] [PATCH v2] recvmsg01: Refactor using new LTP API
Petr Vorel
pvorel@suse.cz
Fri Apr 28 14:46:48 CEST 2023
Hi Wei,
First, for some reason, with higher number of run "bad file descriptor" will
fail, returning 0 instead of -1.
It starts to fail from 200th:
$ ./recvmsg01 -i 200
...
recvmsg01.c:283: TPASS: invalid iovec count successful
recvmsg01.c:283: TPASS: rights reception successful
recvmsg01.c:283: TPASS: invalid MSG_OOB flag set successful
recvmsg01.c:283: TPASS: invalid MSG_ERRQUEUE flag set successful
recvmsg01.c:283: TPASS: large cmesg length successful
recvmsg01.c:276: TFAIL: bad file descriptor ; returned 0 (expected -1), errno 0 (expected 9): SUCCESS (0)
Summary:
passed 1999
failed 1
broken 0
skipped 0
warnings 0
I also did some review, but I didn't have time to check properly whether subject
of testing is valid (quite complex test).
> testcases/kernel/syscalls/recvmsg/recvmsg01.c | 734 +++++++++---------
...
> + * Copyright (c) 2001 Wayne Boyer International Business Machines
nit: maybe
* Copyright (c) 2001 Wayne Boyer, International Business Machines Corp.
> + * Copyright (c) Linux Test Project, 2002-2022
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> */
> -/*
> - * Test Name: recvmsg01
> - *
> - * Test Description:
> - * Verify that recvmsg() returns the proper errno for various failure cases
> - *
> - * Usage: <for command-line>
> - * recvmsg01 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
> - * where, -c n : Run n copies concurrently.
> - * -e : Turn on errno logging.
> - * -i n : Execute test n times.
> - * -I x : Execute test for x seconds.
> - * -P x : Pause for x seconds between iterations.
> - * -t : Turn on syscall timing.
> +/*\
> + * [Description]
> *
> - * HISTORY
> - * 07/2001 Ported by Wayne Boyer
> - *
> - * RESTRICTIONS:
> - * None.
> + * Verify that recvmsg() returns the proper errno for various failure cases
nit: missing dot at the end.
> *
nit: Please remove this blank line.
> */
> +static pid_t pid;
> +static char tmpsunpath[BUF_SIZE];
> +
> +static void setup(void);
> +static void setup_invalid_sock(int);
> +static void setup_valid_sock(int);
> +static void setup_valid_msg_control(int);
> +static void setup_large_msg_control(int);
> +static void cleanup(void);
> +static void cleanup_reset_sock(void);
> +static void cleanup_close_sock(void);
> +static void cleanup_reset_all(void);
> +static void do_child(void);
> +static pid_t start_server(struct sockaddr_in *, struct sockaddr_un *);
nit: I'd personally move struct tcase below to get rid of these function
prototypes, but feel free to ignore.
> +
> +static struct tcase {
> + int domain;
> + int type;
> + int protocol;
> struct iovec *iov;
> int iovcnt;
> - void *buf; /* recv data buffer */
> - int buflen; /* recv buffer length */
> + void *recv_buf;
> + int buflen;
> struct msghdr *msg;
> - unsigned flags;
> - struct sockaddr *from; /* from address */
> - int fromlen; /* from address value/result buffer length */
> - int retval; /* syscall return value */
> - int experrno; /* expected errno */
> - void (*setup) (void);
> - void (*cleanup) (void);
> + unsigned int flags;
> + struct sockaddr *from;
> + int fromlen;
> + int ret;
> + int exp_errno;
> + void (*setup)(int n);
> + void (*cleanup)(void);
> char *desc;
> -} tdat[] = {
> -/* 1 */
> +} tcases[] = {
> {
> - PF_INET, SOCK_STREAM, 0, iov, 1, buf, sizeof(buf), &msgdat, 0,
> - (struct sockaddr *)&from, sizeof(from),
> - -1, EBADF, setup0, cleanup0, "bad file descriptor"}
> - ,
> -/* 2 */
> + .domain = PF_INET,
> + .type = SOCK_STREAM,
> + .protocol = 0,
You can omit values with 0.
> + .iov = iov,
> + .iovcnt = 1,
> + .recv_buf = recv_buf,
> + .buflen = sizeof(recv_buf),
> + .msg = &msgdat,
> + .flags = 0,
Here as well (and on many places)
> + .from = (struct sockaddr *)&from,
> + .fromlen = sizeof(from),
> + .ret = -1,
> + .exp_errno = EBADF,
> + .setup = setup_invalid_sock,
> + .cleanup = cleanup_reset_sock,
> + .desc = "bad file descriptor",
> + },
...
> +static void run(unsigned int n)
> {
> - int lc;
> -
> - tst_parse_opts(argc, argv, NULL, NULL);
> -#ifdef UCLINUX
> - argv0 = argv[0];
> - maybe_run_child(&do_child, "dd", &sfd, &ufd);
> -#endif
> -
> setup();
nit: One would think this is LTP API setup, but it's needed for all tests
(+ the same applies to cleanup()). setup_all() would made it clearer that is
used for each test, but it not important.
> + struct tcase *tc = &tcases[n];
> - tdat[testno].setup();
> -
> - /* setup common to all tests */
> - iov[0].iov_base = tdat[testno].buf;
> - iov[0].iov_len = tdat[testno].buflen;
> - msgdat.msg_name = tdat[testno].from;
> - msgdat.msg_namelen = tdat[testno].fromlen;
> - msgdat.msg_iov = tdat[testno].iov;
> - msgdat.msg_iovlen = tdat[testno].iovcnt;
> - msgdat.msg_control = control;
> - msgdat.msg_controllen = controllen;
> - msgdat.msg_flags = 0;
> -
> - TEST(recvmsg(s, tdat[testno].msg, tdat[testno].flags));
> - if (TEST_RETURN >= 0)
> - TEST_RETURN = 0; /* all nonzero equal here */
> - if (TEST_RETURN != tdat[testno].retval ||
> - (TEST_RETURN < 0 &&
> - TEST_ERRNO != tdat[testno].experrno)) {
> - tst_resm(TFAIL, "%s ; returned"
> - " %ld (expected %d), errno %d (expected"
> - " %d)", tdat[testno].desc,
> - TEST_RETURN, tdat[testno].retval,
> - TEST_ERRNO, tdat[testno].experrno);
> - } else {
> - tst_resm(TPASS, "%s successful",
> - tdat[testno].desc);
> - }
> - tdat[testno].cleanup();
> - }
> + if ((tst_kvercmp(3, 17, 0) < 0)
> + && (tc->flags & MSG_ERRQUEUE)
> + && (tc->type & SOCK_STREAM)) {
> + tst_res(TCONF, "skip MSG_ERRQUEUE test, "
> + "it's supported from 3.17");
> }
if (tst_kvercmp(3, 17, 0) < 0
&& tc->flags & MSG_ERRQUEUE
&& tc->type & SOCK_STREAM) {
tst_res(TCONF, "MSG_ERRQUEUE requires kernel >= 3.17");
}
> - cleanup();
> - tst_exit();
> + tc->setup(n);
> +
> + iov[0].iov_base = tc->recv_buf;
> + iov[0].iov_len = tc->buflen;
> + msgdat.msg_name = tc->from;
> + msgdat.msg_namelen = tc->fromlen;
> + msgdat.msg_iov = tc->iov;
> + msgdat.msg_iovlen = tc->iovcnt;
> + msgdat.msg_control = control;
> + msgdat.msg_controllen = controllen;
> + msgdat.msg_flags = 0;
> +
> + TEST(recvmsg(sock, tc->msg, tc->flags));
> + if (TST_RET >= 0)
> + TST_RET = 0;
> +
> + if (TST_RET != tc->ret ||
> + (TST_ERR != tc->exp_errno)) {
if (TST_RET != tc->ret || TST_ERR != tc->exp_errno) {
> + tst_res(TFAIL | TTERRNO,
> + "%s ; returned"
> + " %ld (expected %d), errno %d (expected"
> + " %d)", tc->desc,
> + TST_RET, tc->ret,
> + TST_ERR, tc->exp_errno);
> + } else {
> + tst_res(TPASS, "%s successful",
> + tc->desc);
> + }
Maybe split return and errno (will be easier to quickly see which one is wrong).
TEST(recvmsg(sock, tc->msg, tc->flags));
if (TST_RET >= 0)
TST_RET = 0;
if (TST_RET != tc->ret) {
tst_res(TFAIL | TTERRNO, "%s: expected %d, returned %ld",
tc->desc, tc->ret, TST_RET);
} else if (TST_ERR != tc->exp_errno) {
tst_res(TFAIL | TTERRNO,
"%s: expected %s",
tc->desc, tst_strerrno(tc->exp_errno));
} else {
tst_res(TPASS, "%s passed", tc->desc);
}
tc->cleanup();
cleanup();
> + tc->cleanup();
> +
> + cleanup();
> }
...
> +static void setup_valid_sock(int n)
> {
> fd_set rdfds;
> struct timeval timeout;
> - int n;
> - s = SAFE_SOCKET(cleanup, tdat[testno].domain, tdat[testno].type,
> - tdat[testno].proto);
> - if (tdat[testno].type == SOCK_STREAM) {
> - if (tdat[testno].domain == PF_INET) {
> - SAFE_CONNECT(cleanup, s, (struct sockaddr *)&sin1,
> + sock = SAFE_SOCKET(tcases[n].domain, tcases[n].type,
> + tcases[n].protocol);
> + if (tcases[n].type == SOCK_STREAM) {
> + if (tcases[n].domain == PF_INET) {
> + SAFE_CONNECT(sock, (struct sockaddr *)&sin1,
> sizeof(sin1));
> /* Wait for something to be readable, else we won't detect EFAULT on recv */
> FD_ZERO(&rdfds);
> - FD_SET(s, &rdfds);
> + FD_SET(sock, &rdfds);
> timeout.tv_sec = 2;
> timeout.tv_usec = 0;
> - n = select(s + 1, &rdfds, 0, 0, &timeout);
> - if (n != 1 || !FD_ISSET(s, &rdfds))
> - tst_brkm(TBROK, cleanup,
> - "client setup1 failed - no message ready in 2 sec");
> - } else if (tdat[testno].domain == PF_UNIX) {
> - SAFE_CONNECT(cleanup, s, (struct sockaddr *)&sun1,
> + n = select(sock + 1, &rdfds, 0, 0, &timeout);
> + if (n != 1 || !FD_ISSET(sock, &rdfds))
> + tst_brk(TBROK, "client setup_valid_sock failed - no message ready in 2 sec");
make check-recvmsg01
recvmsg01.c:348: WARNING: Prefer using '"%s...", __func__' to using 'setup_valid_sock', this function's name, in a string
But I'd just put:
tst_brk(TBROK, "no message ready in 2 sec");
because tst_brk() shows affected line.
> +static void setup_large_msg_control(int n)
> {
> - setup2();
> - controllen = sizeof(struct cmsghdr) - 1;
> + setup_valid_msg_control(n);
> + controllen = 128 * BUF_SIZE;
Any idea why 128? What is this number for?
> }
> -void setup4(void)
> +static void cleanup_close_sock(void)
> {
> - setup2();
> - controllen = 128 * 1024;
> + SAFE_CLOSE(sock);
> + sock = -1;
This sock = -1; is redundant, because SAFE_CLOSE() set it.
> }
> +static void cleanup_reset_all(void)
> {
> - (void)close(s);
> - close(ufd);
> - close(sfd);
> - s = -1;
> -}
> + SAFE_CLOSE(sock);
> + sock = -1;
And here as well.
...
pid_t start_server(struct sockaddr_in *ssin, struct sockaddr_un *ssun)
{
pid_t pid;
socklen_t slen = sizeof(*ssin);
...
SAFE_CLOSE(sfd);
SAFE_CLOSE(ufd);
return pid;
exit(1);
=> IMHO this exit 1 is redundant and should be removed.
}
> -void do_child(void)
> +/* special for rights-passing test */
> +static void sender(int fd)
> +{
> + struct msghdr mh;
Set struct with {}
struct msghdr mh = {};
> + struct cmsghdr *control;
> + char tmpfn[BUF_SIZE], snd_cbuf[BUF_SIZE];
> + int tfd;
> +
> + (void)strcpy(tmpfn, "smtXXXXXX");
> + tfd = mkstemp(tmpfn);
> + if (tfd < 0)
> + return;
> +
> + memset(&mh, 0x00, sizeof(mh));
... will allow to skip memset here.
Also this memset will not be needed:
memset(control, 0x00, sizeof(struct cmsghdr));
> +static void do_child(void)
> {
> struct sockaddr_in fsin;
> struct sockaddr_un fsun;
> fd_set afds, rfds;
> - int nfds, cc, fd;
> + int nfds, fd;
> FD_ZERO(&afds);
> FD_SET(sfd, &afds);
> @@ -455,19 +484,19 @@ void do_child(void)
> int newfd;
> fromlen = sizeof(fsin);
> - newfd = accept(sfd, (struct sockaddr *)&fsin, &fromlen);
> + newfd = SAFE_ACCEPT(sfd, (struct sockaddr *)&fsin, &fromlen);
> if (newfd >= 0) {
> FD_SET(newfd, &afds);
> nfds = MAX(nfds, newfd + 1);
> /* send something back */
> - (void)write(newfd, "hoser\n", 6);
> + SAFE_SEND(1, newfd, "hi", 2, 0);
> }
> }
> if (FD_ISSET(ufd, &rfds)) {
> int newfd;
> fromlen = sizeof(fsun);
> - newfd = accept(ufd, (struct sockaddr *)&fsun, &fromlen);
> + newfd = SAFE_ACCEPT(ufd, (struct sockaddr *)&fsun, &fromlen);
> if (newfd >= 0) {
> FD_SET(newfd, &afds);
> nfds = MAX(nfds, newfd + 1);
> @@ -475,55 +504,22 @@ void do_child(void)
> }
> for (fd = 0; fd < nfds; ++fd)
> if (fd != sfd && fd != ufd && FD_ISSET(fd, &rfds)) {
> - char rbuf[1024];
> + char rbuf[BUF_SIZE];
> - cc = read(fd, rbuf, sizeof(rbuf));
> - if (cc && rbuf[0] == 'R')
> + TEST(read(fd, rbuf, sizeof(rbuf)));
> + if (TST_RET > 0 && rbuf[0] == 'R')
> sender(fd);
> - if (cc == 0 || (cc < 0 && errno != EINTR)) {
> - (void)close(fd);
> + if (TST_RET == 0 || (TST_RET < 0 && TST_ERR != EINTR)) {
> + SAFE_CLOSE(fd);
Please use simple close() here:
/* Here cannot be SAFE_CLOSE() which sets fd to -1,
* otherwise glibc complains:
* *** bit out of range 0 - FD_SETSIZE on fd_set ***: terminated
*/
close(fd);
With higher numbers of -i you get error message from:
glibc >= 2.36 [1]:
$ ./recvmsg01 -i 500
...
recvmsg01.c:283: TPASS: invalid socket length successful
*** bit out of range 0 - FD_SETSIZE on fd_set ***: terminated
recvmsg01.c:283: TPASS: invalid recv buffer successful
FYI The problem is somehow related to max FD_SETSIZE, see man 2 select [2]:
WARNING: select() can monitor only file descriptors numbers that
are less than FD_SETSIZE (1024)—an unreasonably low limit for
many modern applications—and this limitation will not change.
All modern applications should instead use poll(2) or epoll(7),
which do not suffer this limitation.
But in our case I suppose it's really setting the file descriptor to -1.
At least the warning disappears when using just close(fd).
Kind regards,
Petr
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=debug/fdelt_chk.c;h=d1d3a19460fb99212f91f3c9e5a5d17e593530ca;hb=HEAD#l26
[2] https://man7.org/linux/man-pages/man2/select.2.html
More information about the ltp
mailing list