Re: connect01: Convert to new API

On Thu, Jun 04, 2026 at 04:49:55 +0000, Wei Gao wrote:
> connect01: Convert to new API
> connect01: Add negative tests

Hi Wei,

Overall the series is in good shape. The conversion to the new API is
clean and the new EPROTOTYPE/EACCES test cases make sense. One bug needs
fixing and there are a few minor observations.

---
Patch 1: connect01: Convert to new API

> Convert the connect01 test case from the legacy LTP API to the new
> tst_test API.
>
> Refactor the test according to reviewer feedback:
> - Simplified the server child process to a single accept/exit loop.
> - Moved the server lifecycle to the global setup/cleanup.
> - The server now accepts a single connection for the EISCONN test case
>   and exits immediately, which avoids blocking the test runner.
> - Centralized test case initialization into the global setup.
> - Used tst_get_bad_addr() for the EFAULT test case to ensure
>   architectural compatibility.

The bulleted list is fine, though LTP commit messages generally prefer
plain prose over bullet points for the body.

> +static void start_server(struct sockaddr_in *sock)
> +{
> +	socklen_t slen = sizeof(*sock);
> +
> +	sock->sin_family = AF_INET;
> +	sock->sin_port = 0;
> +	sock->sin_addr.s_addr = INADDR_ANY;
> +
> +	fd_server = SAFE_SOCKET(PF_INET, SOCK_STREAM, 0);
> +	SAFE_BIND(fd_server, (struct sockaddr *)sock, slen);
> +	SAFE_LISTEN(fd_server, 10);
> +	SAFE_GETSOCKNAME(fd_server, (struct sockaddr *)sock, &slen);
> +
> +	pid = SAFE_FORK();
> +
> +	if (!pid) {
> +		int nfd = SAFE_ACCEPT(fd_server, NULL, NULL);
> +
> +		SAFE_CLOSE(nfd);
> +		exit(0);
> +	}
> +	SAFE_CLOSE(fd_server);
> +}

The single-accept server child is a nice simplification. Note that
fd_server is closed in the parent here but the static variable still
holds the (now-closed) fd number. cleanup() doesn't close fd_server,
which is correct since the parent already closed it. No issue, but
mentioning for clarity.

> +	if (fd_socket > 0)
> +		SAFE_CLOSE(fd_socket);
> +	if (fd_null > 0)
> +		SAFE_CLOSE(fd_null);
> +	if (fd_connected > 0)
> +		SAFE_CLOSE(fd_connected);

Nit: these guards should be `>= 0` (or `!= -1`) since a valid fd can
theoretically be 0 if stdin is closed before the test runs. Using `> 0`
works in practice (socket fds are typically > 2) but is technically
incorrect. This is a pre-existing LTP pattern, so not a blocker.

Patch 1 otherwise looks correct. The test case order is fine — after
ECONNREFUSED the TCP socket returns to TCP_CLOSE state, and the
subsequent EAFNOSUPPORT check happens before any connection attempt, so
fd_socket reuse is safe here.

---
Patch 2: connect01: Add negative tests

> +	{&fd_unix_dgram, &sock4, sizeof(sock4), EPROTOTYPE,
> +		"socket type does not support the protocol"},

The EPROTOTYPE errno is correct: connecting an AF_UNIX SOCK_DGRAM
socket to a path bound by a SOCK_STREAM server returns -EPROTOTYPE in
unix_find_other(). ✓

> +	{&fd_unix_stream, &sock4, sizeof(sock4), EACCES,
> +		"write permission is denied on the socket file"},

> +	SAFE_CHMOD(SOCK_FILE, 0700);

Correct: the socket file is owned by root with mode 0700. The "nobody"
user has no write permission on it, so connect() returns EACCES. ✓

> +static void verify_connect(unsigned int i)
> +{
> +	struct test_case_t *tc = &tcases[i];
> +	void *addr = tc->addr ? tc->addr : bad_addr;
> +
> +	if (tc->exp_errno == EACCES) {
> +		if (!SAFE_FORK()) {
> +			SAFE_SETUID(pw->pw_uid);
> +			TST_EXP_FAIL(sys_connect(*tc->fd, addr, tc->salen),
> +				tc->exp_errno, "%s", tc->desc);
> +			exit(0);
> +		}
> +	} else {

BUG: The parent never waits for the child. After SAFE_FORK() returns
the child PID (the `if (!SAFE_FORK())` branch is false in the parent),
the parent returns immediately from verify_connect() without reaping
the child. The child becomes a zombie and there is a race condition:
the next test iteration may start before the child has reported its
result via TST_EXP_FAIL.

The established LTP pattern for this (see e.g. msgget02.c) is to call
tst_reap_children() in the parent after the fork:

	if (tc->exp_errno == EACCES) {
		if (!SAFE_FORK()) {
			SAFE_SETUID(pw->pw_uid);
			TST_EXP_FAIL(sys_connect(*tc->fd, addr, tc->salen),
				tc->exp_errno, "%s", tc->desc);
			exit(0);
		}
		tst_reap_children();
	} else {
		TST_EXP_FAIL(sys_connect(*tc->fd, addr, tc->salen),
			tc->exp_errno, "%s", tc->desc);
	}

> +	.needs_root = 1,
> +	.needs_tmpdir = 1,

Both are necessary and correct given the chmod and socket file creation.

---
Summary:

- Patch 1: Reviewed, looks good (nits only, not blocking)
- Patch 2: Needs fix — add tst_reap_children() after the SAFE_FORK()
  block in verify_connect()

Please send a v2 with the fix applied to patch 2.

Thanks,

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