Re: [RFC PATCH v1 2/3] test/vsock: add big message test

On 23.11.2022 18:40, Bobby Eshleman wrote:

On Wed, Nov 23, 2022 at 7:22 AM Stefano Garzarella [off-list ref]
wrote:

quoted

On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:

quoted

On 21.11.2022 19:50, Arseniy Krasnov wrote:

quoted

On 21.11.2022 17:52, Stefano Garzarella wrote:

quoted

On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:

quoted

This adds test for sending message, bigger than peer's buffer size.
For SOCK_SEQPACKET socket it must fail, as this type of socket has
message size limit.

Signed-off-by: Arseniy Krasnov <redacted>
---
tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)

diff --git a/tools/testing/vsock/vsock_test.c

b/tools/testing/vsock/vsock_test.c

quoted

quoted

quoted

quoted

index 107c11165887..bb4e8657f1d6 100644

--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c

@@ -560,6 +560,63 @@ static void test_seqpacket_timeout_server(const

struct test_opts *opts)

quoted

quoted

quoted

quoted

    close(fd);
}

+static void test_seqpacket_bigmsg_client(const struct test_opts

*opts)

quoted

quoted

quoted

quoted

+{
+    unsigned long sock_buf_size;
+    ssize_t send_size;
+    socklen_t len;
+    void *data;
+    int fd;
+
+    len = sizeof(sock_buf_size);
+
+    fd = vsock_seqpacket_connect(opts->peer_cid, 1234);

Not for this patch, but someday we should add a macro for this port

and maybe even make it configurable :-)

quoted

quoted

quoted

quoted

+    if (fd < 0) {
+        perror("connect");
+        exit(EXIT_FAILURE);
+    }
+
+    if (getsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+               &sock_buf_size, &len)) {
+        perror("getsockopt");
+        exit(EXIT_FAILURE);
+    }
+
+    sock_buf_size++;
+
+    data = malloc(sock_buf_size);
+    if (!data) {
+        perror("malloc");
+        exit(EXIT_FAILURE);
+    }
+
+    send_size = send(fd, data, sock_buf_size, 0);
+    if (send_size != -1) {

Can we check also `errno`?
IIUC it should contains EMSGSIZE.

Hm, seems current implementation is a little bit broken and returns

ENOMEM, because any negative value, returned by

quoted

transport callback is always replaced to ENOMEM. I think i need this

patch from Bobby:

quoted

https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/ (local)

quoted

May be i can include it to this patchset also fixing review comments(of

course keeping Bobby as author). Or more

quoted

simple way is to check ENOMEM instead of EMSGSIZE in this test(simple,

but a little bit dumb i think).

Maybe in this patch you can start checking ENOMEM (with a TODO comment),
and then Bobby can change it when sending his patch.

Or you can repost it (I'm not sure if we should keep the legacy behavior
for other transports or it was an error, but better to discuss it on
that patch). However, I think we should merge that patch.

@Bobby, what do you think?

Thanks,
Stefano

This sounds good to me. I removed it from the last rev because I decided not
to complicate the patch by also including SO_SNDBUF support, so had no
need. I think it makes sense overall though.

Ok! So I'll use Your patch(both af_vsock.c and transports related things - seems i'll
split it to several patches, I think for transport patches, it is better to ask Vmware
/Microsoft guys also). I'm going to send next version of my tests on this week.

Thank You

Also, sorry for the delay (I promised last week to send out new rev). I was
planning on sending out v4 with additional data for the non-nested virt
case,
but I've been having some IT troubles with the new phys box.

No problem, im currently rebasing my patches for zerocopy on v3.

Best,
Bobby

PS. sorry if this email format comes out wacky. I'm not at my dev machine
so just using Gmail's web app directly... Hopefully there is no HTML or
anything weird.