Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Jun 2023 07:51:34 +0000
From:   Bobby Eshleman <bobbyeshleman@gmail.com>
To:     Arseniy Krasnov <avkrasnov@sberdevices.ru>
Cc:     Bobby Eshleman <bobby.eshleman@bytedance.com>,
        Jiang Wang <jiang.wang@bytedance.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Bryan Tan <bryantan@vmware.com>,
        Vishnu Dasa <vdasa@vmware.com>,
        VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
        Krasnov Arseniy <oxffffaa@gmail.com>, kvm@vger.kernel.org,
        virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Subject: Re: [PATCH RFC net-next v3 8/8] tests: add vsock dgram tests
Message-ID: <ZHhOBrmU7tzSX3zE@bullseye>
References: <7dbec78e-ea44-4684-6d02-5d6d5051187e@sberdevices.ru>
 <0bd40fd8-e666-e2a3-04da-501a0e7b97a9@sberdevices.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0bd40fd8-e666-e2a3-04da-501a0e7b97a9@sberdevices.ru>
Precedence: bulk

On Tue, Jun 06, 2023 at 12:34:22PM +0300, Arseniy Krasnov wrote:
> Sorry, CC mailing lists
> 
> On 06.06.2023 12:29, Arseniy Krasnov wrote:
> > Hello Bobby and Jiang! Small remarks(sorry for this letter layout, I add multiple newline over comments):
> > 

Hey Arseniy!

> > diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
> > index 01b636d3039a..45e35da48b40 100644
> > --- a/tools/testing/vsock/util.c
> > +++ b/tools/testing/vsock/util.c
> > @@ -260,6 +260,57 @@ void send_byte(int fd, int expected_ret, int flags)
> >  	}
> >  }
> >  
> > +/* Transmit one byte and check the return value.
> > + *
> > + * expected_ret:
> > + *  <0 Negative errno (for testing errors)
> > + *   0 End-of-file
> > + *   1 Success
> > + */
> > +void sendto_byte(int fd, const struct sockaddr *dest_addr, int len, int expected_ret,
> > +		 int flags)
> > +{
> > +	const uint8_t byte = 'A';
> > +	ssize_t nwritten;
> > +
> > +	timeout_begin(TIMEOUT);
> > +	do {
> > +		nwritten = sendto(fd, &byte, sizeof(byte), flags, dest_addr,
> > +				  len);
> > +		timeout_check("write");
> > +	} while (nwritten < 0 && errno == EINTR);
> > +	timeout_end();
> > +
> > +	if (expected_ret < 0) {
> > +		if (nwritten != -1) {
> > +			fprintf(stderr, "bogus sendto(2) return value %zd\n",
> > +				nwritten);
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		if (errno != -expected_ret) {
> > +			perror("write");
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		return;
> > +	}
> > +
> > +	if (nwritten < 0) {
> > +		perror("write");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	if (nwritten == 0) {
> > +		if (expected_ret == 0)
> > +			return;
> > +
> > +		fprintf(stderr, "unexpected EOF while sending byte\n");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	if (nwritten != sizeof(byte)) {
> > +		fprintf(stderr, "bogus sendto(2) return value %zd\n", nwritten);
> > +		exit(EXIT_FAILURE);
> > +
> > 	}
> > 
> > 
> > 
> > ^^^
> > May be short check that 'nwritten' != 'expected_ret' will be enough? Then print expected and
> > real value. Here and in 'recvfrom_byte()' below.
> > 

Right now this is really just a copy/paste of the send_byte() that
stream uses, so that would probably make the two report errors slightly
different. If desired for some specific reason, I'm open to it.

> > 
> > 
> > 
> > +}
> > +
> >  /* Receive one byte and check the return value.
> >   *
> >   * expected_ret:
> > @@ -313,6 +364,60 @@ void recv_byte(int fd, int expected_ret, int flags)
> >  	}
> >  }
> >  
> > +/* Receive one byte and check the return value.
> > + *
> > + * expected_ret:
> > + *  <0 Negative errno (for testing errors)
> > + *   0 End-of-file
> > + *   1 Success
> > + */
> > +void recvfrom_byte(int fd, struct sockaddr *src_addr, socklen_t *addrlen,
> > +		   int expected_ret, int flags)
> > +{
> > +	uint8_t byte;
> > +	ssize_t nread;
> > +
> > +	timeout_begin(TIMEOUT);
> > +	do {
> > +		nread = recvfrom(fd, &byte, sizeof(byte), flags, src_addr, addrlen);
> > +		timeout_check("read");
> > +	} while (nread < 0 && errno == EINTR);
> > +	timeout_end();
> > +
> > +	if (expected_ret < 0) {
> > +		if (nread != -1) {
> > +			fprintf(stderr, "bogus recvfrom(2) return value %zd\n",
> > +				nread);
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		if (errno != -expected_ret) {
> > +			perror("read");
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		return;
> > +	}
> > +
> > +	if (nread < 0) {
> > +		perror("read");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	if (nread == 0) {
> > +		if (expected_ret == 0)
> > +			return;
> > +
> > +		fprintf(stderr, "unexpected EOF while receiving byte\n");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	if (nread != sizeof(byte)) {
> > +		fprintf(stderr, "bogus recvfrom(2) return value %zd\n", nread);
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	if (byte != 'A') {
> > +		fprintf(stderr, "unexpected byte read %c\n", byte);
> > +		exit(EXIT_FAILURE);
> > +	}
> > +}
> > +
> >  /* Run test cases.  The program terminates if a failure occurs. */
> >  void run_tests(const struct test_case *test_cases,
> >  	       const struct test_opts *opts)
> > diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
> > index fb99208a95ea..6e5cd610bf05 100644
> > --- a/tools/testing/vsock/util.h
> > +++ b/tools/testing/vsock/util.h
> > @@ -43,7 +43,11 @@ int vsock_seqpacket_accept(unsigned int cid, unsigned int port,
> >  			   struct sockaddr_vm *clientaddrp);
> >  void vsock_wait_remote_close(int fd);
> >  void send_byte(int fd, int expected_ret, int flags);
> > +void sendto_byte(int fd, const struct sockaddr *dest_addr, int len, int expected_ret,
> > +		 int flags);
> >  void recv_byte(int fd, int expected_ret, int flags);
> > +void recvfrom_byte(int fd, struct sockaddr *src_addr, socklen_t *addrlen,
> > +		   int expected_ret, int flags);
> >  void run_tests(const struct test_case *test_cases,
> >  	       const struct test_opts *opts);
> >  void list_tests(const struct test_case *test_cases);
> > diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
> > index ac1bd3ac1533..851c3d65178d 100644
> > --- a/tools/testing/vsock/vsock_test.c
> > +++ b/tools/testing/vsock/vsock_test.c
> > @@ -202,6 +202,113 @@ static void test_stream_server_close_server(const struct test_opts *opts)
> >  	close(fd);
> >  }
> >  
> > +static void test_dgram_sendto_client(const struct test_opts *opts)
> > +{
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_vm svm;
> > +	} addr = {
> > +		.svm = {
> > +			.svm_family = AF_VSOCK,
> > +			.svm_port = 1234,
> > +			.svm_cid = opts->peer_cid,
> > +		},
> > +	};
> > +	int fd;
> > +
> > +	/* Wait for the server to be ready */
> > +	control_expectln("BIND");
> > +
> > +	fd = socket(AF_VSOCK, SOCK_DGRAM, 0);
> > +	if (fd < 0) {
> > +		perror("socket");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	sendto_byte(fd, &addr.sa, sizeof(addr.svm), 1, 0);
> > +
> > +	/* Notify the server that the client has finished */
> > +	control_writeln("DONE");
> > +
> > +	close(fd);
> > +}
> > +
> > +static void test_dgram_sendto_server(const struct test_opts *opts)
> > +{
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_vm svm;
> > +	} addr = {
> > +		.svm = {
> > +			.svm_family = AF_VSOCK,
> > +			.svm_port = 1234,
> > +			.svm_cid = VMADDR_CID_ANY,
> > +		},
> > +	};
> > +	int fd;
> > +	int len = sizeof(addr.sa);
> > +
> > +	fd = socket(AF_VSOCK, SOCK_DGRAM, 0);
> > 
> > 
> > 
> > ^^^
> > I think we can check 'socket()' return value;
> > 

Gotcha, I'll add in next rev.

> > 
> > 
> > 
> > +
> > +	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
> > +		perror("bind");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	/* Notify the client that the server is ready */
> > +	control_writeln("BIND");
> > +
> > +	recvfrom_byte(fd, &addr.sa, &len, 1, 0);
> > +
> > +	/* Wait for the client to finish */
> > +	control_expectln("DONE");
> > +
> > +	close(fd);
> > +}
> > +
> > +static void test_dgram_connect_client(const struct test_opts *opts)
> > +{
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_vm svm;
> > +	} addr = {
> > +		.svm = {
> > +			.svm_family = AF_VSOCK,
> > +			.svm_port = 1234,
> > +			.svm_cid = opts->peer_cid,
> > +		},
> > +	};
> > +	int fd;
> > +	int ret;
> > +
> > +	/* Wait for the server to be ready */
> > +	control_expectln("BIND");
> > +
> > +	fd = socket(AF_VSOCK, SOCK_DGRAM, 0);
> > +	if (fd < 0) {
> > +		perror("bind");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	ret = connect(fd, &addr.sa, sizeof(addr.svm));
> > +	if (ret < 0) {
> > +		perror("connect");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	send_byte(fd, 1, 0);
> > +
> > +	/* Notify the server that the client has finished */
> > +	control_writeln("DONE");
> > +
> > +	close(fd);
> > +}
> > +
> > +static void test_dgram_connect_server(const struct test_opts *opts)
> > +{
> > +	test_dgram_sendto_server(opts);
> > +}
> > +
> >  /* With the standard socket sizes, VMCI is able to support about 100
> >   * concurrent stream connections.
> >   */
> > @@ -255,6 +362,77 @@ static void test_stream_multiconn_server(const struct test_opts *opts)
> >  		close(fds[i]);
> >  }
> >  
> > +static void test_dgram_multiconn_client(const struct test_opts *opts)
> > +{
> > +	int fds[MULTICONN_NFDS];
> > +	int i;
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_vm svm;
> > +	} addr = {
> > +		.svm = {
> > +			.svm_family = AF_VSOCK,
> > +			.svm_port = 1234,
> > +			.svm_cid = opts->peer_cid,
> > +		},
> > +	};
> > +
> > +	/* Wait for the server to be ready */
> > +	control_expectln("BIND");
> > +
> > +	for (i = 0; i < MULTICONN_NFDS; i++) {
> > +		fds[i] = socket(AF_VSOCK, SOCK_DGRAM, 0);
> > +		if (fds[i] < 0) {
> > +			perror("socket");
> > +			exit(EXIT_FAILURE);
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < MULTICONN_NFDS; i++)
> > +		sendto_byte(fds[i], &addr.sa, sizeof(addr.svm), 1, 0);
> > +
> > +	/* Notify the server that the client has finished */
> > +	control_writeln("DONE");
> > +
> > +	for (i = 0; i < MULTICONN_NFDS; i++)
> > +		close(fds[i]);
> > +}
> > +
> > +static void test_dgram_multiconn_server(const struct test_opts *opts)
> > +{
> > +	union {
> > +		struct sockaddr sa;
> > +		struct sockaddr_vm svm;
> > +	} addr = {
> > +		.svm = {
> > +			.svm_family = AF_VSOCK,
> > +			.svm_port = 1234,
> > +			.svm_cid = VMADDR_CID_ANY,
> > +		},
> > +	};
> > +	int fd;
> > +	int len = sizeof(addr.sa);
> > +	int i;
> > +
> > +	fd = socket(AF_VSOCK, SOCK_DGRAM, 0);
> > 
> > 
> > 
> > ^^^
> > I think we can check 'socket()' return value;
> > 
> > 
> > 
> > +
> > +	if (bind(fd, &addr.sa, sizeof(addr.svm)) < 0) {
> > +		perror("bind");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	/* Notify the client that the server is ready */
> > +	control_writeln("BIND");
> > +
> > +	for (i = 0; i < MULTICONN_NFDS; i++)
> > +		recvfrom_byte(fd, &addr.sa, &len, 1, 0);
> > +
> > +	/* Wait for the client to finish */
> > +	control_expectln("DONE");
> > +
> > +	close(fd);
> > +}
> > +
> >  static void test_stream_msg_peek_client(const struct test_opts *opts)
> >  {
> >  	int fd;
> > @@ -1128,6 +1306,21 @@ static struct test_case test_cases[] = {
> >  		.run_client = test_stream_virtio_skb_merge_client,
> >  		.run_server = test_stream_virtio_skb_merge_server,
> >  	},
> > +	{
> > +		.name = "SOCK_DGRAM client close",
> > +		.run_client = test_dgram_sendto_client,
> > +		.run_server = test_dgram_sendto_server,
> > +	},
> > +	{
> > +		.name = "SOCK_DGRAM client connect",
> > +		.run_client = test_dgram_connect_client,
> > +		.run_server = test_dgram_connect_server,
> > +	},
> > +	{
> > +		.name = "SOCK_DGRAM multiple connections",
> > +		.run_client = test_dgram_multiconn_client,
> > +		.run_server = test_dgram_multiconn_server,
> > +	},
> > 
> > 
> > 
> > 
> > SOCK_DGRAM guarantees message bounds, I think it will be good to add such test like in SOCK_SEQPACKET tests.

Agreed, I'll write one for the next rev.

> > 
> > Thanks, Arseniy

Thanks for the review!
> > 
> > 
> >  	{},
> >  };
> >  
> >