Re: [LTP] [PATCH v2 1/2] Add shutdown01 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

TL;DR: generally LGTM, I have few minor details, which I offer to fix before
merge (diff below).

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

> +++ b/testcases/kernel/syscalls/shutdown/shutdown01.c
...
> +static struct tcase {
> +	int shutdown_op;
> +	int recv_flag;
> +	int recv_err;
> +	int send_flag;
> +	int send_err;
> +	char *flag_str;
> +} tcases[] = {
> +	{SHUT_RD, 0, 0, 0, 0, "SHUT_RD"},
> +	{SHUT_WR, MSG_DONTWAIT, EWOULDBLOCK, MSG_NOSIGNAL, EPIPE, "SHUT_WR"},
> +	{SHUT_RDWR, 0, 0, MSG_NOSIGNAL, EPIPE, "SHUT_RDWR"}

Would you mind to use
1) designated initializers (e.g. .recv_flag = MSG_DONTWAIT,)
2) stringify macro

IMHO it helps readability, that's why it's often used.

#define OP_DESC(x) .shutdown_op = x, .desc = #x
static struct tcase {
	int shutdown_op;
	int recv_flag;
	int recv_err;
	int send_flag;
	int send_err;
	char *desc;
} tcases[] = {
	{OP_DESC(SHUT_RD)},
	{OP_DESC(SHUT_WR), .recv_flag = MSG_DONTWAIT, .recv_err = EWOULDBLOCK,
		.send_flag = MSG_NOSIGNAL, .send_err = EPIPE},
	{OP_DESC(SHUT_RDWR), .send_flag = MSG_NOSIGNAL, .send_err = EPIPE}
};

> +};
> +
> +static struct sockaddr_un *sock_addr;
> +
> +static void run_server(void)
> +{
> +	int server_sock;
> +
> +	server_sock = SAFE_SOCKET(sock_addr->sun_family, SOCK_STREAM, 0);
> +
> +	SAFE_BIND(server_sock,
> +		(struct sockaddr *)sock_addr,
> +		sizeof(struct sockaddr_un));
> +	SAFE_LISTEN(server_sock, 10);
> +
> +	tst_res(TINFO, "Running server on socket file");
> +
> +	TST_CHECKPOINT_WAKE_AND_WAIT(0);
> +
> +	SAFE_CLOSE(server_sock);
> +	SAFE_UNLINK(SOCKETFILE);
> +}
> +
> +static int start_test(void)
> +{
> +	int client_sock;
> +
> +	if (!SAFE_FORK()) {
> +		run_server();
> +		_exit(0);
> +	}
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	tst_res(TINFO, "Connecting to the server");
> +
> +	client_sock = SAFE_SOCKET(sock_addr->sun_family, SOCK_STREAM, 0);
> +	SAFE_CONNECT(client_sock,
> +		(struct sockaddr *)sock_addr,
> +		sizeof(struct sockaddr_un));
> +
> +	return client_sock;
> +}
> +
> +static void run(unsigned int n)
> +{
> +	struct tcase *tc = &tcases[n];
> +	int client_sock;
> +	char buff[MSGSIZE] = {0};
> +
> +	client_sock = start_test();
> +
> +	tst_res(TINFO, "Testing %s flag", tc->flag_str);
> +
> +	TST_EXP_PASS(shutdown(client_sock, tc->shutdown_op));
> +
> +	if (!tc->recv_err)
> +		SAFE_RECV(0, client_sock, buff, MSGSIZE, tc->recv_flag);
> +	else
> +		TST_EXP_FAIL(recv(client_sock, buff, MSGSIZE, tc->recv_flag), tc->recv_err);

very nit:

	if (tc->recv_err)
		TST_EXP_FAIL(recv(client_sock, buff, MSGSIZE, tc->recv_flag), tc->recv_err);
	else
		SAFE_RECV(0, client_sock, buff, MSGSIZE, tc->recv_flag);

If you agree, I can merge with following changes.

Kind regards,
Petr

diff --git testcases/kernel/syscalls/shutdown/shutdown01.c testcases/kernel/syscalls/shutdown/shutdown01.c
index ba3853d9c..8e58f23e6 100644
--- testcases/kernel/syscalls/shutdown/shutdown01.c
+++ testcases/kernel/syscalls/shutdown/shutdown01.c
@@ -19,17 +19,19 @@
 #define MSGSIZE 4
 #define SOCKETFILE "socket"
 
+#define OP_DESC(x) .shutdown_op = x, .desc = #x
 static struct tcase {
 	int shutdown_op;
 	int recv_flag;
 	int recv_err;
 	int send_flag;
 	int send_err;
-	char *flag_str;
+	char *desc;
 } tcases[] = {
-	{SHUT_RD, 0, 0, 0, 0, "SHUT_RD"},
-	{SHUT_WR, MSG_DONTWAIT, EWOULDBLOCK, MSG_NOSIGNAL, EPIPE, "SHUT_WR"},
-	{SHUT_RDWR, 0, 0, MSG_NOSIGNAL, EPIPE, "SHUT_RDWR"}
+	{OP_DESC(SHUT_RD)},
+	{OP_DESC(SHUT_WR), .recv_flag = MSG_DONTWAIT, .recv_err = EWOULDBLOCK,
+		.send_flag = MSG_NOSIGNAL, .send_err = EPIPE},
+	{OP_DESC(SHUT_RDWR), .send_flag = MSG_NOSIGNAL, .send_err = EPIPE}
 };
 
 static struct sockaddr_un *sock_addr;
@@ -82,23 +84,22 @@ static void run(unsigned int n)
 
 	client_sock = start_test();
 
-	tst_res(TINFO, "Testing %s flag", tc->flag_str);
+	tst_res(TINFO, "Testing %s flag", tc->desc);
 
 	TST_EXP_PASS(shutdown(client_sock, tc->shutdown_op));
 
-	if (!tc->recv_err)
-		SAFE_RECV(0, client_sock, buff, MSGSIZE, tc->recv_flag);
-	else
+	if (tc->recv_err)
 		TST_EXP_FAIL(recv(client_sock, buff, MSGSIZE, tc->recv_flag), tc->recv_err);
-
-	if (!tc->send_err)
-		SAFE_SEND(MSGSIZE, client_sock, buff, MSGSIZE, tc->send_flag);
 	else
+		SAFE_RECV(0, client_sock, buff, MSGSIZE, tc->recv_flag);
+
+	if (tc->send_err)
 		TST_EXP_FAIL(send(client_sock, buff, MSGSIZE, tc->send_flag), tc->send_err);
+	else
+		SAFE_SEND(MSGSIZE, client_sock, buff, MSGSIZE, tc->send_flag);
 
 	SAFE_CLOSE(client_sock);
 	TST_CHECKPOINT_WAKE(0);
-
 }
 
 static void setup(void)

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