thr3ads.net - Virtualization - [RFC PATCH v1 1/3] test/vsock: rework message bound test [Nov 2022]

If this information is useful, please help other people find it:
Share via:
Stefano Garzarella
2022-Nov-23 15:24 UTC
[RFC PATCH v1 1/3] test/vsock: rework message bound test

On Mon, Nov 21, 2022 at 04:49:23PM +0000, Arseniy Krasnov
wrote:>On 21.11.2022 17:46, Stefano Garzarella wrote:
>> On Tue, Nov 15, 2022 at 08:50:36PM +0000, Arseniy Krasnov wrote:
>>> This updates message bound test making it more complex. Instead of
>>> sending 1 bytes messages with one MSG_EOR bit, it sends messages of
>>> random length(one half of messages are smaller than page size,
second
>>> half are bigger) with random number of MSG_EOR bits set. Receiver
>>> also don't know total number of messages.
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov at sberdevices.ru>
>>> ---
>>> tools/testing/vsock/control.c??? |? 34 +++++++++
>>> tools/testing/vsock/control.h??? |?? 2 +
>>> tools/testing/vsock/util.c?????? |? 13 ++++
>>> tools/testing/vsock/util.h?????? |?? 1 +
>>> tools/testing/vsock/vsock_test.c | 115
+++++++++++++++++++++++++++----
>>> 5 files changed, 152 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tools/testing/vsock/control.c
b/tools/testing/vsock/control.c
>>> index 4874872fc5a3..bed1649bdf3d 100644
>>> --- a/tools/testing/vsock/control.c
>>> +++ b/tools/testing/vsock/control.c
>>> @@ -141,6 +141,40 @@ void control_writeln(const char *str)
>>> ????timeout_end();
>>> }
>>>
>>> +void control_writeulong(unsigned long value)
>>> +{
>>> +??? char str[32];
>>> +
>>> +??? if (snprintf(str, sizeof(str), "%lu", value) >=
sizeof(str)) {
>>> +??????? perror("snprintf");
>>> +??????? exit(EXIT_FAILURE);
>>> +??? }
>>> +
>>> +??? control_writeln(str);
>>> +}
>>> +
>>> +unsigned long control_readulong(bool *ok)
>>> +{
>>> +??? unsigned long value;
>>> +??? char *str;
>>> +
>>> +??? if (ok)
>>> +??????? *ok = false;
>>> +
>>> +??? str = control_readln();
>>> +
>>> +??? if (str == NULL)
>>
>> checkpatch suggests to use !str
>>
>>> +??????? return 0;
>>
>> Maybe we can just call exit(EXIT_FAILURE) here and remove the `ok`
>> parameter, since I'm not sure we can recover from this error.
>>
>>> +
>>> +??? value = strtoul(str, NULL, 10);
>>> +??? free(str);
>>> +
>>> +??? if (ok)
>>> +??????? *ok = true;
>>> +
>>> +??? return value;
>>> +}
>>> +
>>> /* Return the next line from the control socket (without the
trailing newline).
>>> ?*
>>> ?* The program terminates if a timeout occurs.
>>> diff --git a/tools/testing/vsock/control.h
b/tools/testing/vsock/control.h
>>> index 51814b4f9ac1..cdd922dfea68 100644
>>> --- a/tools/testing/vsock/control.h
>>> +++ b/tools/testing/vsock/control.h
>>> @@ -9,7 +9,9 @@ void control_init(const char *control_host, const
char *control_port,
>>> void control_cleanup(void);
>>> void control_writeln(const char *str);
>>> char *control_readln(void);
>>> +unsigned long control_readulong(bool *ok);
>>> void control_expectln(const char *str);
>>> bool control_cmpln(char *line, const char *str, bool fail);
>>> +void control_writeulong(unsigned long value);
>>>
>>> #endif /* CONTROL_H */
>>> diff --git a/tools/testing/vsock/util.c
b/tools/testing/vsock/util.c
>>> index 2acbb7703c6a..351903836774 100644
>>> --- a/tools/testing/vsock/util.c
>>> +++ b/tools/testing/vsock/util.c
>>> @@ -395,3 +395,16 @@ void skip_test(struct test_case *test_cases,
size_t test_cases_len,
>>>
>>> ????test_cases[test_id].skip = true;
>>> }
>>> +
>>> +unsigned long djb2(const void *data, size_t len)
>>
>> I would add hash_ as a prefix (or suffix).
>>
>>> +{
>>> +??? unsigned long hash = 5381;
>>> +??? int i = 0;
>>> +
>>> +??? while (i < len) {
>>> +??????? hash = ((hash << 5) + hash) + ((unsigned char
*)data)[i];
>>> +??????? i++;
>>> +??? }
>>> +
>>> +??? return hash;
>>> +}
>>> diff --git a/tools/testing/vsock/util.h
b/tools/testing/vsock/util.h
>>> index a3375ad2fb7f..988cc69a4642 100644
>>> --- a/tools/testing/vsock/util.h
>>> +++ b/tools/testing/vsock/util.h
>>> @@ -49,4 +49,5 @@ void run_tests(const struct test_case
*test_cases,
>>> void list_tests(const struct test_case *test_cases);
>>> void skip_test(struct test_case *test_cases, size_t test_cases_len,
>>> ?????????? const char *test_id_str);
>>> +unsigned long djb2(const void *data, size_t len);
>>> #endif /* UTIL_H */
>>> diff --git a/tools/testing/vsock/vsock_test.c
b/tools/testing/vsock/vsock_test.c
>>> index bb6d691cb30d..107c11165887 100644
>>> --- a/tools/testing/vsock/vsock_test.c
>>> +++ b/tools/testing/vsock/vsock_test.c
>>> @@ -284,10 +284,14 @@ static void test_stream_msg_peek_server(const
struct test_opts *opts)
>>> ????close(fd);
>>> }
>>>
>>> -#define MESSAGES_CNT 7
>>> -#define MSG_EOR_IDX (MESSAGES_CNT / 2)
>>> +#define SOCK_BUF_SIZE (2 * 1024 * 1024)
>>> +#define MAX_MSG_SIZE (32 * 1024)
>>> +
>>> static void test_seqpacket_msg_bounds_client(const struct test_opts
*opts)
>>> {
>>> +??? unsigned long curr_hash;
>>> +??? int page_size;
>>> +??? int msg_count;
>>> ????int fd;
>>>
>>> ????fd = vsock_seqpacket_connect(opts->peer_cid, 1234);
>>> @@ -296,18 +300,69 @@ static void
test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>>> ??????? exit(EXIT_FAILURE);
>>> ????}
>>>
>>> -??? /* Send several messages, one with MSG_EOR flag */
>>> -??? for (int i = 0; i < MESSAGES_CNT; i++)
>>> -??????? send_byte(fd, 1, (i == MSG_EOR_IDX) ? MSG_EOR : 0);
>>> +??? /* Wait, until receiver sets buffer size. */
>>> +??? control_expectln("SRVREADY");
>>> +
>>> +??? curr_hash = 0;
>>> +??? page_size = getpagesize();
>>> +??? msg_count = SOCK_BUF_SIZE / MAX_MSG_SIZE;
>>> +
>>> +??? for (int i = 0; i < msg_count; i++) {
>>> +??????? ssize_t send_size;
>>> +??????? size_t buf_size;
>>> +??????? int flags;
>>> +??????? void *buf;
>>> +
>>> +??????? /* Use "small" buffers and "big"
buffers. */
>>> +??????? if (i & 1)
>>> +??????????? buf_size = page_size +
>>> +??????????????????? (rand() % (MAX_MSG_SIZE - page_size));
>>> +??????? else
>>> +??????????? buf_size = 1 + (rand() % page_size);
>>> +
>>> +??????? buf = malloc(buf_size);
>>> +
>>> +??????? if (!buf) {
>>> +??????????? perror("malloc");
>>> +??????????? exit(EXIT_FAILURE);
>>> +??????? }
>>> +
>>> +??????? /* Set at least one MSG_EOR + some random. */
>>> +??????? if (i == (msg_count / 2) || (rand() & 1)) {
>>> +??????????? flags = MSG_EOR;
>>> +??????????? curr_hash++;
>>> +??????? } else {
>>> +??????????? flags = 0;
>>> +??????? }
>>> +
>>> +??????? send_size = send(fd, buf, buf_size, flags);
>>> +
>>> +??????? if (send_size < 0) {
>>> +??????????? perror("send");
>>> +??????????? exit(EXIT_FAILURE);
>>> +??????? }
>>> +
>>> +??????? if (send_size != buf_size) {
>>> +??????????? fprintf(stderr, "Invalid send size\n");
>>> +??????????? exit(EXIT_FAILURE);
>>> +??????? }
>>> +
>>> +??????? curr_hash += send_size;
>>> +??????? curr_hash = djb2(&curr_hash, sizeof(curr_hash));
>>> +??? }
>>>
>>> ????control_writeln("SENDDONE");
>>> +??? control_writeulong(curr_hash);
>>
>> Why do we need to hash the size?
>>
>> Maybe we can send it without making the hash, anyway even if it wraps,
>> it should wrap the same way in both the server and the client.
>> (Or maybe we can use uin32_t or uint64_t to make sure both were
>> using 4 or 8 bytes)
>Hello, thanks for review. I think if we will use sum of message size(IIUC),
in most
>paranoic case it won't guarantee message bounds control: single 4 bytes
message
>could be read as 4 x 1 byte message(IIUC of course). Idea of hashing is
simple:
>every iteration we do current_hash = hash(previous_hash + size of current
message);
>I think this is more reliable and protects from case described above.
Okay, now I understand what it is for and agree that using hash is 
better.
Please add a comment to explain it.
>
>All other comments - ack.
Thanks,
Stefano
Virtualization - Nov 2022 - [RFC PATCH v1 1/3] test/vsock: rework message bound test

[RFC PATCH v1 1/3] test/vsock: rework message bound test
