diff mbox series

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

Message ID f0510949-cc97-7a01-5fc8-f7e855b80515@sberdevices.ru
State New
Headers
Series test/vsock: update two tests and add new tool |

Commit Message

Arseniy Krasnov Nov. 15, 2022, 8:52 p.m. UTC 
  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 <AVKrasnov@sberdevices.ru>
---
 tools/testing/vsock/vsock_test.c | 62 ++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

-- 
2.25.1

Comments

Stefano Garzarella Nov. 21, 2022, 2:52 p.m. UTC | #1 
On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>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 <AVKrasnov@sberdevices.ru>
>---
> 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
>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)
> 	close(fd);
> }
>
>+static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>+{
>+	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 :-)

>+	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.

>+		fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>+			send_size);
>+	}
>+
>+	control_writeln("CLISENT");
>+
>+	free(data);
>+	close(fd);
>+}
>+
>+static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>+{
>+	int fd;
>+
>+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>+	if (fd < 0) {
>+		perror("accept");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	control_expectln("CLISENT");
>+
>+	close(fd);
>+}
>+
> #define BUF_PATTERN_1 'a'
> #define BUF_PATTERN_2 'b'
>
>@@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_timeout_client,
> 		.run_server = test_seqpacket_timeout_server,
> 	},
>+	{
>+		.name = "SOCK_SEQPACKET big message",
>+		.run_client = test_seqpacket_bigmsg_client,
>+		.run_server = test_seqpacket_bigmsg_server,
>+	},

I would add new tests always at the end, so if some CI uses --skip, we 
don't have to update the scripts to skip some tests.

> 	{
> 		.name = "SOCK_SEQPACKET invalid receive buffer",
> 		.run_client = test_seqpacket_invalid_rec_buffer_client,
>-- 
>2.25.1
Arseniy Krasnov Nov. 21, 2022, 4:50 p.m. UTC | #2 
On 21.11.2022 17:52, Stefano Garzarella wrote:
> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>> 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 <AVKrasnov@sberdevices.ru>
>> ---
>> 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
>> 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)
>>     close(fd);
>> }
>>
>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>> +{
>> +    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 :-)
> 
>> +    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.
> 
>> +        fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>> +            send_size);
>> +    }
>> +
>> +    control_writeln("CLISENT");
>> +
>> +    free(data);
>> +    close(fd);
>> +}
>> +
>> +static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>> +{
>> +    int fd;
>> +
>> +    fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>> +    if (fd < 0) {
>> +        perror("accept");
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>> +    control_expectln("CLISENT");
>> +
>> +    close(fd);
>> +}
>> +
>> #define BUF_PATTERN_1 'a'
>> #define BUF_PATTERN_2 'b'
>>
>> @@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
>>         .run_client = test_seqpacket_timeout_client,
>>         .run_server = test_seqpacket_timeout_server,
>>     },
>> +    {
>> +        .name = "SOCK_SEQPACKET big message",
>> +        .run_client = test_seqpacket_bigmsg_client,
>> +        .run_server = test_seqpacket_bigmsg_server,
>> +    },
> 
> I would add new tests always at the end, so if some CI uses --skip, we don't have to update the scripts to skip some tests.
Ack this and all above
> 
>>     {
>>         .name = "SOCK_SEQPACKET invalid receive buffer",
>>         .run_client = test_seqpacket_invalid_rec_buffer_client,
>> -- 
>> 2.25.1
>
Arseniy Krasnov Nov. 21, 2022, 9:40 p.m. UTC | #3 
On 21.11.2022 19:50, Arseniy Krasnov wrote:
> On 21.11.2022 17:52, Stefano Garzarella wrote:
>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>> 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 <AVKrasnov@sberdevices.ru>
>>> ---
>>> 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
>>> 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)
>>>     close(fd);
>>> }
>>>
>>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>>> +{
>>> +    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 :-)
>>
>>> +    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
transport callback is always replaced to ENOMEM. I think i need this patch from Bobby:
https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
May be i can include it to this patchset also fixing review comments(of course keeping Bobby as author). Or more
simple way is to check ENOMEM instead of EMSGSIZE in this test(simple, but a little bit dumb i think).
>>
>>> +        fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
>>> +            send_size);
>>> +    }
>>> +
>>> +    control_writeln("CLISENT");
>>> +
>>> +    free(data);
>>> +    close(fd);
>>> +}
>>> +
>>> +static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
>>> +{
>>> +    int fd;
>>> +
>>> +    fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
>>> +    if (fd < 0) {
>>> +        perror("accept");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>> +
>>> +    control_expectln("CLISENT");
>>> +
>>> +    close(fd);
>>> +}
>>> +
>>> #define BUF_PATTERN_1 'a'
>>> #define BUF_PATTERN_2 'b'
>>>
>>> @@ -832,6 +889,11 @@ static struct test_case test_cases[] = {
>>>         .run_client = test_seqpacket_timeout_client,
>>>         .run_server = test_seqpacket_timeout_server,
>>>     },
>>> +    {
>>> +        .name = "SOCK_SEQPACKET big message",
>>> +        .run_client = test_seqpacket_bigmsg_client,
>>> +        .run_server = test_seqpacket_bigmsg_server,
>>> +    },
>>
>> I would add new tests always at the end, so if some CI uses --skip, we don't have to update the scripts to skip some tests.
> Ack this and all above
>>
>>>     {
>>>         .name = "SOCK_SEQPACKET invalid receive buffer",
>>>         .run_client = test_seqpacket_invalid_rec_buffer_client,
>>> -- 
>>> 2.25.1
>>
>
Stefano Garzarella Nov. 23, 2022, 3:21 p.m. UTC | #4 
On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:
>On 21.11.2022 19:50, Arseniy Krasnov wrote:
>> On 21.11.2022 17:52, Stefano Garzarella wrote:
>>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>>> 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 <AVKrasnov@sberdevices.ru>
>>>> ---
>>>> 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
>>>> 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)
>>>>     close(fd);
>>>> }
>>>>
>>>> +static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>>>> +{
>>>> +    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 :-)
>>>
>>>> +    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
>transport callback is always replaced to ENOMEM. I think i need this patch from Bobby:
>https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
>May be i can include it to this patchset also fixing review comments(of course keeping Bobby as author). Or more
>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
Arseniy Krasnov Nov. 23, 2022, 4:28 p.m. UTC | #5 
On 23.11.2022 18:40, Bobby Eshleman wrote:
> On Wed, Nov 23, 2022 at 7:22 AM Stefano Garzarella <sgarzare@redhat.com>
> wrote:
> 
>> On Mon, Nov 21, 2022 at 09:40:39PM +0000, Arseniy Krasnov wrote:
>>> On 21.11.2022 19:50, Arseniy Krasnov wrote:
>>>> On 21.11.2022 17:52, Stefano Garzarella wrote:
>>>>> On Tue, Nov 15, 2022 at 08:52:35PM +0000, Arseniy Krasnov wrote:
>>>>>> 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 <AVKrasnov@sberdevices.ru>
>>>>>> ---
>>>>>> 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
>>>>>> 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)
>>>>>>     close(fd);
>>>>>> }
>>>>>>
>>>>>> +static void test_seqpacket_bigmsg_client(const struct test_opts
>> *opts)
>>>>>> +{
>>>>>> +    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 :-)
>>>>>
>>>>>> +    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
>>> transport callback is always replaced to ENOMEM. I think i need this
>> patch from Bobby:
>>>
>> https://lore.kernel.org/lkml/d81818b868216c774613dd03641fcfe63cc55a45.1660362668.git.bobby.eshleman@bytedance.com/
>>> May be i can include it to this patchset also fixing review comments(of
>> course keeping Bobby as author). Or more
>>> 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.
>
diff mbox series

Patch

diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
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)
 	close(fd);
 }
 
+static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
+{
+	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);
+	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) {
+		fprintf(stderr, "expected 'send(2)' failure, got %zi\n",
+			send_size);
+	}
+
+	control_writeln("CLISENT");
+
+	free(data);
+	close(fd);
+}
+
+static void test_seqpacket_bigmsg_server(const struct test_opts *opts)
+{
+	int fd;
+
+	fd = vsock_seqpacket_accept(VMADDR_CID_ANY, 1234, NULL);
+	if (fd < 0) {
+		perror("accept");
+		exit(EXIT_FAILURE);
+	}
+
+	control_expectln("CLISENT");
+
+	close(fd);
+}
+
 #define BUF_PATTERN_1 'a'
 #define BUF_PATTERN_2 'b'
 
@@ -832,6 +889,11 @@  static struct test_case test_cases[] = {
 		.run_client = test_seqpacket_timeout_client,
 		.run_server = test_seqpacket_timeout_server,
 	},
+	{
+		.name = "SOCK_SEQPACKET big message",
+		.run_client = test_seqpacket_bigmsg_client,
+		.run_server = test_seqpacket_bigmsg_server,
+	},
 	{
 		.name = "SOCK_SEQPACKET invalid receive buffer",
 		.run_client = test_seqpacket_invalid_rec_buffer_client,