| Message ID | 20240426142042.14573-4-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/mapped-ram: Add direct-io support | expand |
On Fri, Apr 26, 2024 at 11:20:36AM -0300, Fabiano Rosas wrote: > When doing file migration, QEMU accepts an offset that should be > skipped when writing the migration stream to the file. The purpose of > the offset is to allow the management layer to put its own metadata at > the start of the file. > > We have tests for this in migration-test, but only testing that the > migration stream starts at the correct offset and not that it actually > leaves the data intact. Unsurprisingly, there's been a bug in that > area that the tests didn't catch. > > Fix the tests to write some data to the offset region and check that > it's actually there after the migration. > > Fixes: 3dc35470c8 ("tests/qtest: migration-test: Add tests for file-based migration") > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > tests/qtest/migration-test.c | 70 +++++++++++++++++++++++++++++++++--- > 1 file changed, 65 insertions(+), 5 deletions(-) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 5d6d8cd634..7b177686b4 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -2081,6 +2081,63 @@ static void test_precopy_file(void) > test_file_common(&args, true); > } > > +#ifndef _WIN32 > +static void file_dirty_offset_region(void) > +{ > +#if defined(__linux__) Hmm, what's the case to cover when !_WIN32 && __linux__? Can we remove one layer of ifdef? I'm also wondering why it can't work on win32? I thought win32 has all these stuff we used here, but I may miss something. > + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); > + size_t size = FILE_TEST_OFFSET; > + uintptr_t *addr, *p; > + int fd; > + > + fd = open(path, O_CREAT | O_RDWR, 0660); > + g_assert(fd != -1); > + > + g_assert(!ftruncate(fd, size)); > + > + addr = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0); > + g_assert(addr != MAP_FAILED); > + > + /* ensure the skipped offset contains some data */ > + p = addr; > + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { > + *p = (unsigned long) FILE_TEST_FILENAME; This is fine, but not as clear what is assigned.. I think here we assigned is the pointer pointing to the binary's RO section (rather than the chars). Maybe using some random numbers would be more straightforward, but no strong opinions. > + p++; > + } > + > + munmap(addr, size); > + fsync(fd); > + close(fd); > +#endif > +} > + > +static void *file_offset_start_hook(QTestState *from, QTestState *to) > +{ > + g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); > + int src_flags = O_WRONLY; > + int dst_flags = O_RDONLY; > + int fds[2]; > + > + file_dirty_offset_region(); > + > + fds[0] = open(file, src_flags, 0660); > + assert(fds[0] != -1); > + > + fds[1] = open(file, dst_flags, 0660); > + assert(fds[1] != -1); > + > + qtest_qmp_fds_assert_success(from, &fds[0], 1, "{'execute': 'add-fd', " > + "'arguments': {'fdset-id': 1}}"); > + > + qtest_qmp_fds_assert_success(to, &fds[1], 1, "{'execute': 'add-fd', " > + "'arguments': {'fdset-id': 1}}"); > + > + close(fds[0]); > + close(fds[1]); > + > + return NULL; > +} > + > static void file_offset_finish_hook(QTestState *from, QTestState *to, > void *opaque) > { > @@ -2096,12 +2153,12 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, > g_assert(addr != MAP_FAILED); > > /* > - * Ensure the skipped offset contains zeros and the migration > - * stream starts at the right place. > + * Ensure the skipped offset region's data has not been touched > + * and the migration stream starts at the right place. > */ > p = addr; > while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { > - g_assert(*p == 0); > + g_assert_cmpstr((char *) *p, ==, FILE_TEST_FILENAME); > p++; > } > g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC); > @@ -2113,17 +2170,18 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, > > static void test_precopy_file_offset(void) > { > - g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs, > - FILE_TEST_FILENAME, > + g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", > FILE_TEST_OFFSET); Do we want to keep both tests to cover both normal file and fdsets? > MigrateCommon args = { > .connect_uri = uri, > .listen_uri = "defer", > + .start_hook = file_offset_start_hook, > .finish_hook = file_offset_finish_hook, > }; > > test_file_common(&args, false); > } > +#endif > > static void test_precopy_file_offset_bad(void) > { > @@ -3636,8 +3694,10 @@ int main(int argc, char **argv) > > migration_test_add("/migration/precopy/file", > test_precopy_file); > +#ifndef _WIN32 > migration_test_add("/migration/precopy/file/offset", > test_precopy_file_offset); > +#endif > migration_test_add("/migration/precopy/file/offset/bad", > test_precopy_file_offset_bad); > > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Fri, Apr 26, 2024 at 11:20:36AM -0300, Fabiano Rosas wrote: >> When doing file migration, QEMU accepts an offset that should be >> skipped when writing the migration stream to the file. The purpose of >> the offset is to allow the management layer to put its own metadata at >> the start of the file. >> >> We have tests for this in migration-test, but only testing that the >> migration stream starts at the correct offset and not that it actually >> leaves the data intact. Unsurprisingly, there's been a bug in that >> area that the tests didn't catch. >> >> Fix the tests to write some data to the offset region and check that >> it's actually there after the migration. >> >> Fixes: 3dc35470c8 ("tests/qtest: migration-test: Add tests for file-based migration") >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> tests/qtest/migration-test.c | 70 +++++++++++++++++++++++++++++++++--- >> 1 file changed, 65 insertions(+), 5 deletions(-) >> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c >> index 5d6d8cd634..7b177686b4 100644 >> --- a/tests/qtest/migration-test.c >> +++ b/tests/qtest/migration-test.c >> @@ -2081,6 +2081,63 @@ static void test_precopy_file(void) >> test_file_common(&args, true); >> } >> >> +#ifndef _WIN32 >> +static void file_dirty_offset_region(void) >> +{ >> +#if defined(__linux__) > > Hmm, what's the case to cover when !_WIN32 && __linux__? Can we remove one > layer of ifdef? > > I'm also wondering why it can't work on win32? I thought win32 has all > these stuff we used here, but I may miss something. > __linux__ is because of mmap, !_WIN32 is because of the passing of fds. We might be able to keep !_WIN32 only, I'll check. >> + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); >> + size_t size = FILE_TEST_OFFSET; >> + uintptr_t *addr, *p; >> + int fd; >> + >> + fd = open(path, O_CREAT | O_RDWR, 0660); >> + g_assert(fd != -1); >> + >> + g_assert(!ftruncate(fd, size)); >> + >> + addr = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0); >> + g_assert(addr != MAP_FAILED); >> + >> + /* ensure the skipped offset contains some data */ >> + p = addr; >> + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { >> + *p = (unsigned long) FILE_TEST_FILENAME; > > This is fine, but not as clear what is assigned.. I think here we assigned > is the pointer pointing to the binary's RO section (rather than the chars). Haha you're right, I was assigning the FILE_TEST_OFFSET previously and just switched to the FILENAME without thinking. I'll fix it up. > Maybe using some random numbers would be more straightforward, but no > strong opinions. > >> + p++; >> + } >> + >> + munmap(addr, size); >> + fsync(fd); >> + close(fd); >> +#endif >> +} >> + >> +static void *file_offset_start_hook(QTestState *from, QTestState *to) >> +{ >> + g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); >> + int src_flags = O_WRONLY; >> + int dst_flags = O_RDONLY; >> + int fds[2]; >> + >> + file_dirty_offset_region(); >> + >> + fds[0] = open(file, src_flags, 0660); >> + assert(fds[0] != -1); >> + >> + fds[1] = open(file, dst_flags, 0660); >> + assert(fds[1] != -1); >> + >> + qtest_qmp_fds_assert_success(from, &fds[0], 1, "{'execute': 'add-fd', " >> + "'arguments': {'fdset-id': 1}}"); >> + >> + qtest_qmp_fds_assert_success(to, &fds[1], 1, "{'execute': 'add-fd', " >> + "'arguments': {'fdset-id': 1}}"); >> + >> + close(fds[0]); >> + close(fds[1]); >> + >> + return NULL; >> +} >> + >> static void file_offset_finish_hook(QTestState *from, QTestState *to, >> void *opaque) >> { >> @@ -2096,12 +2153,12 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, >> g_assert(addr != MAP_FAILED); >> >> /* >> - * Ensure the skipped offset contains zeros and the migration >> - * stream starts at the right place. >> + * Ensure the skipped offset region's data has not been touched >> + * and the migration stream starts at the right place. >> */ >> p = addr; >> while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { >> - g_assert(*p == 0); >> + g_assert_cmpstr((char *) *p, ==, FILE_TEST_FILENAME); >> p++; >> } >> g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC); >> @@ -2113,17 +2170,18 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, >> >> static void test_precopy_file_offset(void) >> { >> - g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs, >> - FILE_TEST_FILENAME, >> + g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", >> FILE_TEST_OFFSET); > > Do we want to keep both tests to cover both normal file and fdsets? > I think the fdset + offset is the most complex in terms of requirements, so I don't think we need to test the other one. I'm actually already a bit concerned about the amount of tests we have. I was even thinking of starting playing with some code coverage tools and prune some of the tests if possible. >> MigrateCommon args = { >> .connect_uri = uri, >> .listen_uri = "defer", >> + .start_hook = file_offset_start_hook, >> .finish_hook = file_offset_finish_hook, >> }; >> >> test_file_common(&args, false); >> } >> +#endif >> >> static void test_precopy_file_offset_bad(void) >> { >> @@ -3636,8 +3694,10 @@ int main(int argc, char **argv) >> >> migration_test_add("/migration/precopy/file", >> test_precopy_file); >> +#ifndef _WIN32 >> migration_test_add("/migration/precopy/file/offset", >> test_precopy_file_offset); >> +#endif >> migration_test_add("/migration/precopy/file/offset/bad", >> test_precopy_file_offset_bad); >> >> -- >> 2.35.3 >>
On Fri, May 03, 2024 at 05:36:59PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Apr 26, 2024 at 11:20:36AM -0300, Fabiano Rosas wrote: > >> When doing file migration, QEMU accepts an offset that should be > >> skipped when writing the migration stream to the file. The purpose of > >> the offset is to allow the management layer to put its own metadata at > >> the start of the file. > >> > >> We have tests for this in migration-test, but only testing that the > >> migration stream starts at the correct offset and not that it actually > >> leaves the data intact. Unsurprisingly, there's been a bug in that > >> area that the tests didn't catch. > >> > >> Fix the tests to write some data to the offset region and check that > >> it's actually there after the migration. > >> > >> Fixes: 3dc35470c8 ("tests/qtest: migration-test: Add tests for file-based migration") > >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> --- > >> tests/qtest/migration-test.c | 70 +++++++++++++++++++++++++++++++++--- > >> 1 file changed, 65 insertions(+), 5 deletions(-) > >> > >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > >> index 5d6d8cd634..7b177686b4 100644 > >> --- a/tests/qtest/migration-test.c > >> +++ b/tests/qtest/migration-test.c > >> @@ -2081,6 +2081,63 @@ static void test_precopy_file(void) > >> test_file_common(&args, true); > >> } > >> > >> +#ifndef _WIN32 > >> +static void file_dirty_offset_region(void) > >> +{ > >> +#if defined(__linux__) > > > > Hmm, what's the case to cover when !_WIN32 && __linux__? Can we remove one > > layer of ifdef? > > > > I'm also wondering why it can't work on win32? I thought win32 has all > > these stuff we used here, but I may miss something. > > > > __linux__ is because of mmap, !_WIN32 is because of the passing of > fds. We might be able to keep !_WIN32 only, I'll check. Thanks, or simply use __linux__; we don't lose that much if test less on very special hosts. Just feel a bit over-engineer to use two ifdefs for one such test. > > >> + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); > >> + size_t size = FILE_TEST_OFFSET; > >> + uintptr_t *addr, *p; > >> + int fd; > >> + > >> + fd = open(path, O_CREAT | O_RDWR, 0660); > >> + g_assert(fd != -1); > >> + > >> + g_assert(!ftruncate(fd, size)); > >> + > >> + addr = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0); > >> + g_assert(addr != MAP_FAILED); > >> + > >> + /* ensure the skipped offset contains some data */ > >> + p = addr; > >> + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { > >> + *p = (unsigned long) FILE_TEST_FILENAME; > > > > This is fine, but not as clear what is assigned.. I think here we assigned > > is the pointer pointing to the binary's RO section (rather than the chars). > > Haha you're right, I was assigning the FILE_TEST_OFFSET previously and > just switched to the FILENAME without thinking. I'll fix it up. :) > > > Maybe using some random numbers would be more straightforward, but no > > strong opinions. > > > >> + p++; > >> + } > >> + > >> + munmap(addr, size); > >> + fsync(fd); > >> + close(fd); > >> +#endif > >> +} > >> + > >> +static void *file_offset_start_hook(QTestState *from, QTestState *to) > >> +{ > >> + g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); > >> + int src_flags = O_WRONLY; > >> + int dst_flags = O_RDONLY; > >> + int fds[2]; > >> + > >> + file_dirty_offset_region(); > >> + > >> + fds[0] = open(file, src_flags, 0660); > >> + assert(fds[0] != -1); > >> + > >> + fds[1] = open(file, dst_flags, 0660); > >> + assert(fds[1] != -1); > >> + > >> + qtest_qmp_fds_assert_success(from, &fds[0], 1, "{'execute': 'add-fd', " > >> + "'arguments': {'fdset-id': 1}}"); > >> + > >> + qtest_qmp_fds_assert_success(to, &fds[1], 1, "{'execute': 'add-fd', " > >> + "'arguments': {'fdset-id': 1}}"); > >> + > >> + close(fds[0]); > >> + close(fds[1]); > >> + > >> + return NULL; > >> +} > >> + > >> static void file_offset_finish_hook(QTestState *from, QTestState *to, > >> void *opaque) > >> { > >> @@ -2096,12 +2153,12 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, > >> g_assert(addr != MAP_FAILED); > >> > >> /* > >> - * Ensure the skipped offset contains zeros and the migration > >> - * stream starts at the right place. > >> + * Ensure the skipped offset region's data has not been touched > >> + * and the migration stream starts at the right place. > >> */ > >> p = addr; > >> while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { > >> - g_assert(*p == 0); > >> + g_assert_cmpstr((char *) *p, ==, FILE_TEST_FILENAME); > >> p++; > >> } > >> g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC); > >> @@ -2113,17 +2170,18 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, > >> > >> static void test_precopy_file_offset(void) > >> { > >> - g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs, > >> - FILE_TEST_FILENAME, > >> + g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", > >> FILE_TEST_OFFSET); > > > > Do we want to keep both tests to cover both normal file and fdsets? > > > > I think the fdset + offset is the most complex in terms of requirements, > so I don't think we need to test the other one. They will still cover different qemu code paths, right? Even if only slightly different. > > I'm actually already a bit concerned about the amount of tests we > have. I was even thinking of starting playing with some code coverage > tools and prune some of the tests if possible. IMHO we don't need to drop any test, but if / when we find it runs too slow, we either: - try to speed it up - I never tried, but I _feel_ like I can make it faster in some way, just like when Dan used to do with reducing migration-test runtimes, perhaps from different angles, or - mark more tests optional to run by default, then we use getenv() to select those. Said that, what you're exploring sounds interesting irrelevant. > > >> MigrateCommon args = { > >> .connect_uri = uri, > >> .listen_uri = "defer", > >> + .start_hook = file_offset_start_hook, > >> .finish_hook = file_offset_finish_hook, > >> }; > >> > >> test_file_common(&args, false); > >> } > >> +#endif > >> > >> static void test_precopy_file_offset_bad(void) > >> { > >> @@ -3636,8 +3694,10 @@ int main(int argc, char **argv) > >> > >> migration_test_add("/migration/precopy/file", > >> test_precopy_file); > >> +#ifndef _WIN32 > >> migration_test_add("/migration/precopy/file/offset", > >> test_precopy_file_offset); > >> +#endif > >> migration_test_add("/migration/precopy/file/offset/bad", > >> test_precopy_file_offset_bad); > >> > >> -- > >> 2.35.3 > >> >
On Fri, May 03, 2024 at 05:36:59PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Apr 26, 2024 at 11:20:36AM -0300, Fabiano Rosas wrote: > >> When doing file migration, QEMU accepts an offset that should be > >> skipped when writing the migration stream to the file. The purpose of > >> the offset is to allow the management layer to put its own metadata at > >> the start of the file. > >> > >> We have tests for this in migration-test, but only testing that the > >> migration stream starts at the correct offset and not that it actually > >> leaves the data intact. Unsurprisingly, there's been a bug in that > >> area that the tests didn't catch. > >> > >> Fix the tests to write some data to the offset region and check that > >> it's actually there after the migration. > >> > >> Fixes: 3dc35470c8 ("tests/qtest: migration-test: Add tests for file-based migration") > >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> --- > >> tests/qtest/migration-test.c | 70 +++++++++++++++++++++++++++++++++--- > >> 1 file changed, 65 insertions(+), 5 deletions(-) > >> > >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > >> index 5d6d8cd634..7b177686b4 100644 > >> --- a/tests/qtest/migration-test.c > >> +++ b/tests/qtest/migration-test.c > >> @@ -2081,6 +2081,63 @@ static void test_precopy_file(void) > >> test_file_common(&args, true); > >> } > >> > >> +#ifndef _WIN32 > >> +static void file_dirty_offset_region(void) > >> +{ > >> +#if defined(__linux__) > > > > Hmm, what's the case to cover when !_WIN32 && __linux__? Can we remove one > > layer of ifdef? > > > > I'm also wondering why it can't work on win32? I thought win32 has all > > these stuff we used here, but I may miss something. > > > > __linux__ is because of mmap, !_WIN32 is because of the passing of > fds. We might be able to keep !_WIN32 only, I'll check. > > >> + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); > >> + size_t size = FILE_TEST_OFFSET; > >> + uintptr_t *addr, *p; > >> + int fd; > >> + > >> + fd = open(path, O_CREAT | O_RDWR, 0660); > >> + g_assert(fd != -1); > >> + > >> + g_assert(!ftruncate(fd, size)); > >> + > >> + addr = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0); > >> + g_assert(addr != MAP_FAILED); > >> + > >> + /* ensure the skipped offset contains some data */ > >> + p = addr; > >> + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { > >> + *p = (unsigned long) FILE_TEST_FILENAME; > > > > This is fine, but not as clear what is assigned.. I think here we assigned > > is the pointer pointing to the binary's RO section (rather than the chars). > > Haha you're right, I was assigning the FILE_TEST_OFFSET previously and > just switched to the FILENAME without thinking. I'll fix it up. > > > Maybe using some random numbers would be more straightforward, but no > > strong opinions. > > > >> + p++; > >> + } > >> + > >> + munmap(addr, size); > >> + fsync(fd); > >> + close(fd); > >> +#endif > >> +} Use of mmap and this loop looks like overkill to me, when we can do it in a fully portable manner with: g_autofree char *data = g_new0(char *, offset); memset(data, 0x44, offset); g_file_set_contents(path, data, offset, NULL); and I checked that g_file_set_contents' impl also takes care of fsync. With regards, Daniel
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 5d6d8cd634..7b177686b4 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -2081,6 +2081,63 @@ static void test_precopy_file(void) test_file_common(&args, true); } +#ifndef _WIN32 +static void file_dirty_offset_region(void) +{ +#if defined(__linux__) + g_autofree char *path = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); + size_t size = FILE_TEST_OFFSET; + uintptr_t *addr, *p; + int fd; + + fd = open(path, O_CREAT | O_RDWR, 0660); + g_assert(fd != -1); + + g_assert(!ftruncate(fd, size)); + + addr = mmap(NULL, size, PROT_WRITE, MAP_SHARED, fd, 0); + g_assert(addr != MAP_FAILED); + + /* ensure the skipped offset contains some data */ + p = addr; + while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { + *p = (unsigned long) FILE_TEST_FILENAME; + p++; + } + + munmap(addr, size); + fsync(fd); + close(fd); +#endif +} + +static void *file_offset_start_hook(QTestState *from, QTestState *to) +{ + g_autofree char *file = g_strdup_printf("%s/%s", tmpfs, FILE_TEST_FILENAME); + int src_flags = O_WRONLY; + int dst_flags = O_RDONLY; + int fds[2]; + + file_dirty_offset_region(); + + fds[0] = open(file, src_flags, 0660); + assert(fds[0] != -1); + + fds[1] = open(file, dst_flags, 0660); + assert(fds[1] != -1); + + qtest_qmp_fds_assert_success(from, &fds[0], 1, "{'execute': 'add-fd', " + "'arguments': {'fdset-id': 1}}"); + + qtest_qmp_fds_assert_success(to, &fds[1], 1, "{'execute': 'add-fd', " + "'arguments': {'fdset-id': 1}}"); + + close(fds[0]); + close(fds[1]); + + return NULL; +} + static void file_offset_finish_hook(QTestState *from, QTestState *to, void *opaque) { @@ -2096,12 +2153,12 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, g_assert(addr != MAP_FAILED); /* - * Ensure the skipped offset contains zeros and the migration - * stream starts at the right place. + * Ensure the skipped offset region's data has not been touched + * and the migration stream starts at the right place. */ p = addr; while (p < addr + FILE_TEST_OFFSET / sizeof(uintptr_t)) { - g_assert(*p == 0); + g_assert_cmpstr((char *) *p, ==, FILE_TEST_FILENAME); p++; } g_assert_cmpint(cpu_to_be64(*p) >> 32, ==, QEMU_VM_FILE_MAGIC); @@ -2113,17 +2170,18 @@ static void file_offset_finish_hook(QTestState *from, QTestState *to, static void test_precopy_file_offset(void) { - g_autofree char *uri = g_strdup_printf("file:%s/%s,offset=%d", tmpfs, - FILE_TEST_FILENAME, + g_autofree char *uri = g_strdup_printf("file:/dev/fdset/1,offset=%d", FILE_TEST_OFFSET); MigrateCommon args = { .connect_uri = uri, .listen_uri = "defer", + .start_hook = file_offset_start_hook, .finish_hook = file_offset_finish_hook, }; test_file_common(&args, false); } +#endif static void test_precopy_file_offset_bad(void) { @@ -3636,8 +3694,10 @@ int main(int argc, char **argv) migration_test_add("/migration/precopy/file", test_precopy_file); +#ifndef _WIN32 migration_test_add("/migration/precopy/file/offset", test_precopy_file_offset); +#endif migration_test_add("/migration/precopy/file/offset/bad", test_precopy_file_offset_bad);
When doing file migration, QEMU accepts an offset that should be skipped when writing the migration stream to the file. The purpose of the offset is to allow the management layer to put its own metadata at the start of the file. We have tests for this in migration-test, but only testing that the migration stream starts at the correct offset and not that it actually leaves the data intact. Unsurprisingly, there's been a bug in that area that the tests didn't catch. Fix the tests to write some data to the offset region and check that it's actually there after the migration. Fixes: 3dc35470c8 ("tests/qtest: migration-test: Add tests for file-based migration") Signed-off-by: Fabiano Rosas <farosas@suse.de> --- tests/qtest/migration-test.c | 70 +++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 5 deletions(-)