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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> 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.

Let's just assume it is a problem because it is easy to add the cleanup
code.

Something to think about is that regardless of test iterations, the
kernel may behave differently when cleaning up a socket on process
termination rather than a deliberate close. For e.g. it could do a bulk
cleanup after some delay, so you may get errors happening as the next
test executes.

Of course that can happen with a deliberate close as well, but you are
bringing the close forward at least. Also it makes the behaviour of the
test after an error more similar to if there were not an error. So
comparing the output may be easier.

>
>> +
>> +	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>
>
>> +};

I'll set this to changes requested in patchwork

-- 
Thank you,
Richard.

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