Re: [PULL 11/38] tests/qtest: migration-test: Add tests for file-based migration

[Juan Quintela <quintela@redhat.com>]

Fabiano Rosas <farosas@suse.de> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> wrote:
>> D> Juan Quintela <quintela@redhat.com> writes:
>>>
>>>> From: Fabiano Rosas <farosas@suse.de>
>>>>
>>>> Add basic tests for file-based migration.
>>>>
>>>> Note that we cannot use test_precopy_common because that routine
>>>> expects it to be possible to run the migration live. With the file
>>>> transport there is no live migration because we must wait for the
>>>> source to finish writing the migration data to the file before the
>>>> destination can start reading. Add a new migration function
>>>> specifically to handle the file migration.
>>>>
>>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> Message-ID: <20230712190742.22294-7-farosas@suse.de>
>>
>>>> +static void file_offset_finish_hook(QTestState *from, QTestState *to,
>>>> +                                    void *opaque)
>>>> +{
>>>> +#if defined(__linux__)
>>>> +    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_SHARED, fd, 0);
>>>> +    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);
>>>
>>> This truncates to 32-bits, so it breaks on a BE host. We need this:
>>>
>>> -->8--
>>> From ea0c2d1c988add48d9754891a9fc7f6854a9718a Mon Sep 17 00:00:00 2001
>>> From: Fabiano Rosas <farosas@suse.de>
>>> Date: Mon, 16 Oct 2023 15:21:49 -0300
>>> Subject: [PATCH] fixup! tests/qtest: migration-test: Add tests for file-based
>>>  migration
>>>
>>> ---
>>>  tests/qtest/migration-test.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>>> index da02b6d692..e1c110537b 100644
>>> --- a/tests/qtest/migration-test.c
>>> +++ b/tests/qtest/migration-test.c
>>> @@ -1966,7 +1966,7 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to,
>>>          g_assert(*p == 0);
>>>          p++;
>>>      }
>>> -    g_assert_cmpint(cpu_to_be32(*p), ==, QEMU_VM_FILE_MAGIC);
>>> +    g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);
>>>  
>>>      munmap(addr, size);
>>>      close(fd);
>>
>> I am resubmitting with this change.
>>
>> But I think we need to change this:
>>
>>>> +    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;
>>
>> I think we should change the test so the file is 64 bits on every
>> architecture.
>> Then we can cast to void * or uintptr_t as needed.
>
> Hm, I don't get what you mean here. What needs to be 64 bits?

size_t is 32 bits on 32bits host, and 64 bits on 64 bits host.
uintprt_t is the same.

So using explicit sizes:

static void file_offset_finish_hook(QTestState *from, QTestState *to,
                                    void *opaque)
{
#if defined(__linux__)
    g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME);
    size_t size = FILE_TEST_OFFSET + sizeof(QEMU_VM_FILE_MAGIC);
    uint64_t *addr, *p;
    int fd;

    fd = open(path, O_RDONLY);
    g_assert(fd != -1);
    addr = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
    g_assert(addr != MAP_FAILED);

    /*
     * Ensure the skipped offset contains zeros and the migration
     * stream starts at the right place.
     */
    p = addr;
    while (p < (uintprt_t)addr + FILE_TEST_OFFSET) {
        g_assert(*p == 0);
        p++;
    }
    g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC);

    munmap(addr, size);
    close(fd);
#endif
}

This is completely untested, but it should make sure that we are reading
64bits integers in both 32 and 64 bits hosts, no?

And yes, for migration, in case of doubt, we use 64bits.  I know it is
unfair for 32 bits host architectures, but they basically don't exist
anymore.

Later, Juan.