Re: [LTP] [PATCH v3] Add epoll_wait05 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

> This test verifies that epoll receives EPOLLRDHUP event when we hang
> a reading half-socket we are polling on.

Commit message would deserver

Implements: https://github.com/linux-test-project/ltp/issues/860
(Although the ticket asks to cover more flags than just EPOLLET).

> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  .../kernel/syscalls/epoll_wait/.gitignore     |   1 +
>  .../kernel/syscalls/epoll_wait/epoll_wait05.c | 117 ++++++++++++++++++
Test is missing a record in runtest/syscalls.

>  2 files changed, 118 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/epoll_wait/epoll_wait05.c

> diff --git a/testcases/kernel/syscalls/epoll_wait/.gitignore b/testcases/kernel/syscalls/epoll_wait/.gitignore
> index 222955dd2..ab5a9c010 100644
> --- a/testcases/kernel/syscalls/epoll_wait/.gitignore
> +++ b/testcases/kernel/syscalls/epoll_wait/.gitignore
> @@ -2,3 +2,4 @@ epoll_wait01
>  epoll_wait02
>  epoll_wait03
>  epoll_wait04
> +epoll_wait05
> diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait05.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait05.c
> new file mode 100644
> index 000000000..e43cac933
> --- /dev/null
> +++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait05.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify that epoll receives EPOLLRDHUP event when we hang a reading
> + * half-socket we are polling on.

I'd put here the LWN article:
https://lwn.net/Articles/864947/
> + */
> +
> +#define _GNU_SOURCE
Why is _GNU_SOURCE needed?

> +#include "tst_test.h"
> +#include "tst_epoll.h"
> +
> +static int epfd;
> +static in_port_t *sock_port;
> +
> +static void create_server(void)
> +{
> +	int sockfd;
> +	socklen_t len;
> +	struct sockaddr_in serv_addr;
> +	struct sockaddr_in sin;
> +
> +	memset(&serv_addr, 0, sizeof(struct sockaddr_in));
> +	serv_addr.sin_family = AF_INET;
> +	serv_addr.sin_addr.s_addr = htonl(INADDR_ANY);
> +	serv_addr.sin_port = 0;
You could use tst_init_sockaddr_inet_bin() (include include "tst_net.h").
> +
> +	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> +	SAFE_BIND(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr));
> +	SAFE_LISTEN(sockfd, 10);
> +
> +	len = sizeof(sin);
> +	memset(&sin, 0, sizeof(struct sockaddr_in));
> +	SAFE_GETSOCKNAME(sockfd, (struct sockaddr *)&sin, &len);
> +
> +	*sock_port = sin.sin_port;
> +
> +	tst_res(TINFO, "Listening on port %d", *sock_port);
> +
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
> +
> +	SAFE_CLOSE(sockfd);
> +}
> +
> +static void run(void)
> +{
> +	int sockfd;
> +	struct sockaddr_in client_addr;
> +	struct epoll_event evt_req;
> +	struct epoll_event evt_rec;
> +
> +	if (!SAFE_FORK()) {
> +		create_server();
> +		return;
> +	}
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	tst_res(TINFO, "Connecting to port %d", *sock_port);
> +
> +	sockfd = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
> +
> +	memset(&client_addr, 0, sizeof(struct sockaddr));
> +	client_addr.sin_family = AF_INET;
> +	client_addr.sin_addr.s_addr = inet_addr("127.0.0.1");
> +	client_addr.sin_port = *sock_port;
And here as well.

> +
> +	SAFE_CONNECT(sockfd, (struct sockaddr *)&client_addr, sizeof(client_addr));
> +
> +	tst_res(TINFO, "Polling on socket");
> +
> +	epfd = SAFE_EPOLL_CREATE1(0);
> +	evt_req.events = EPOLLRDHUP;
> +	SAFE_EPOLL_CTL(epfd, EPOLL_CTL_ADD, sockfd, &evt_req);
> +
> +	tst_res(TINFO, "Hang socket");
> +
> +	TST_EXP_PASS_SILENT(shutdown(sockfd, SHUT_RD));
> +	SAFE_EPOLL_WAIT(epfd, &evt_rec, 1, 2000);
> +
> +	if (evt_rec.events & EPOLLRDHUP)
> +		tst_res(TPASS, "Received EPOLLRDHUP");
> +	else
> +		tst_res(TFAIL, "EPOLLRDHUP has not been received");
> +
> +	SAFE_CLOSE(epfd);
> +	SAFE_CLOSE(sockfd);
In extreme case when SAFE_EPOLL_WAIT() fails sockfd will not get closed, right?
This could be problem on high number of iterations. But I guess it's just a
theoretical problem.

> +
> +	TST_CHECKPOINT_WAKE(0);
> +}
> +
> +static void setup(void)
> +{
> +	sock_port = SAFE_MMAP(NULL, sizeof(in_port_t), PROT_READ | PROT_WRITE,
> +		MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (sock_port)
> +		SAFE_MUNMAP(sock_port, sizeof(in_port_t));
> +
> +	if (epfd > 0)
> +		SAFE_CLOSE(epfd);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
IMHO there should be 3a34b13a88ca commit marked in tags:

.tags = (const struct tst_tag[]) {
        {"linux-git", "3a34b13a88ca"},
        {}

The rest LGTM.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp