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!
> >
> >
> > {},
> > };
> >
> >
