Peter Xu <pet...@redhat.com> writes:
> On Wed, Jun 28, 2023 at 01:55:42PM -0300, Fabiano Rosas wrote:
>> Add basic tests for file-based migration.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> tests/qtest/migration-test.c | 104 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 104 insertions(+)
>>
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index acb778a8cd..b3019f54de 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -52,6 +52,10 @@ static bool got_dst_resume;
>> */
>> #define DIRTYLIMIT_TOLERANCE_RANGE 25 /* MB/s */
>>
>> +#define QEMU_VM_FILE_MAGIC 0x5145564d
>> +#define FILE_TEST_FILENAME "migfile"
>> +#define FILE_TEST_OFFSET 0x1000
>> +
>> #if defined(__linux__)
>> #include <sys/syscall.h>
>> #include <sys/vfs.h>
>> @@ -763,6 +767,7 @@ static void test_migrate_end(QTestState *from,
>> QTestState *to, bool test_dest)
>> cleanup("migsocket");
>> cleanup("src_serial");
>> cleanup("dest_serial");
>> + cleanup(FILE_TEST_FILENAME);
>> }
>>
>> #ifdef CONFIG_GNUTLS
>> @@ -1460,11 +1465,28 @@ static void test_precopy_common(MigrateCommon *args)
>> */
>> wait_for_migration_complete(from);
>>
>> + /*
>> + * For file based migration the target must begin its
>> + * migration after the source has finished.
>> + */
>> + if (strstr(connect_uri, "file:")) {
>> + migrate_incoming_qmp(to, connect_uri, "{}");
>> + }
>> +
>> if (!got_src_stop) {
>> qtest_qmp_eventwait(from, "STOP");
>> }
>> } else {
>> wait_for_migration_complete(from);
>> +
>> + /*
>> + * For file based migration the target must begin its
>> + * migration after the source has finished.
>> + */
>> + if (strstr(connect_uri, "file:")) {
>> + migrate_incoming_qmp(to, connect_uri, "{}");
>> + }
>> +
>> /*
>> * Must wait for dst to finish reading all incoming
>> * data on the socket before issuing 'cont' otherwise
>> @@ -1682,6 +1704,78 @@ static void test_precopy_unix_compress_nowait(void)
>> test_precopy_common(&args);
>> }
>>
>> +static void test_precopy_file(void)
>> +{
>> + g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
>> + FILE_TEST_FILENAME);
>> + MigrateCommon args = {
>> + .connect_uri = uri,
>> + .listen_uri = "defer",
>> + };
>> +
>> + test_precopy_common(&args);
>> +}
>> +
>> +#if defined(__linux__)
>> +static void file_offset_finish_hook(QTestState *from, QTestState *to, void
>> *opaque)
>> +{
>> + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs,
>> FILE_TEST_FILENAME);
>> + size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
>> + uintptr_t *addr, *p;
>> + int fd;
>> +
>> + fd = open(path, O_RDONLY);
>> + g_assert(fd != -1);
>> + addr = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
>
> Not something that matters a lot, but RO mapping a file with private is a
> bit weird. Maybe just use MAP_SHARED?
>
Yep
>> + g_assert(addr != MAP_FAILED);
>> +
>> + /*
>> + * Ensure the skipped offset contains zeros and the migration
>> + * stream starts at the right place.
>> + */
>> + p = addr;
>> + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) {
>> + g_assert(*p == 0);
>> + p++;
>> + }
>> + g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>> +
>> + munmap(addr, size);
>> + close(fd);
>> +}
>> +
>> +static void test_precopy_file_offset(void)
>> +{
>> + g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs,
>> + FILE_TEST_FILENAME,
>> + FILE_TEST_OFFSET);
>
> Is it intended to also only run this test with linux? IIUC it should also
> work for others. Maybe only file_offset_finish_hook() is optional? Or am i
> wrong?
>
Yes, only the hook is linux-specific. I'll change it.
>> + MigrateCommon args = {
>> + .connect_uri = uri,
>> + .listen_uri = "defer",
>> + .finish_hook = file_offset_finish_hook,
>> + };
>> +
>> + test_precopy_common(&args);
>> +}
>> +#endif
>> +
>> +static void test_precopy_file_offset_bad(void)
>> +{
>> + /* using a value not supported by qemu_strtosz() */
>> + g_autofree char *uri = g_strdup_printf("file:%s/migfile,offset=0x20M",
>> + tmpfs);
>> + MigrateCommon args = {
>> + .connect_uri = uri,
>> + .listen_uri = "defer",
>> + .error_str = g_strdup(
>> + "file URI has bad offset 0x20M: Unknown error -22"),
>
> "Unknown error" may imply that in Steve's patch the errno is inverted..
>
> Shall we not rely on the string in the test? It might be too strict, I
> worry, because error strings should be defined for human readers, and we
> may not want some e.g. grammar / trivial change to break a test.
>
Well, you just caught an issue with the errno by looking at the string,
so maybe testing it is a good thing?
I'd expect anyone changing the string to run the test and catch the
mismatch before sending a patch anyway.
I don't have a strong opinion about it, though. I can remove the
error_str.
