1. Patchew
  2. QEMU
  3. [PATCH 00/42] Migration test refactoring
  4. View patch

[PATCH 13/42] migration-test: Check for shared memory like for everything else

Juan Quintela posted 42 patches 2 years, 8 months ago
[PATCH 13/42] migration-test: Check for shared memory like for everything else
Posted by Juan Quintela 2 years, 8 months ago 
Makes things easier and cleaner.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/qtest/migration-test.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index daaf5cd71a..5837060138 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -645,13 +645,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *arch = qtest_get_arch();
     const char *memory_size;
 
-    if (args->use_shmem) {
-        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
-            g_test_skip("/dev/shm is not supported");
-            return -1;
-        }
-    }
-
     got_src_stop = false;
     got_dst_resume = false;
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
@@ -2639,6 +2632,15 @@ static bool kvm_dirty_ring_supported(void)
 #endif
 }
 
+static bool shm_supported(void)
+{
+    if (g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
+        return true;
+    }
+    g_test_message("Skipping test: shared memory not available");
+    return false;
+}
+
 int main(int argc, char **argv)
 {
     bool has_kvm, has_tcg;
@@ -2768,7 +2770,9 @@ int main(int argc, char **argv)
 #endif /* CONFIG_TASN1 */
 #endif /* CONFIG_GNUTLS */
 
-    qtest_add_func("/migration/ignore_shared", test_ignore_shared);
+    if (shm_supported()) {
+        qtest_add_func("/migration/ignore_shared", test_ignore_shared);
+    }
 #ifndef _WIN32
     qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
 #endif
-- 
2.40.1
Re: [PATCH 13/42] migration-test: Check for shared memory like for everything else
Posted by Peter Xu 2 years, 7 months ago 
On Fri, Jun 09, 2023 at 12:49:14AM +0200, Juan Quintela wrote:
> Makes things easier and cleaner.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  tests/qtest/migration-test.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index daaf5cd71a..5837060138 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -645,13 +645,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>      const char *arch = qtest_get_arch();
>      const char *memory_size;
>  
> -    if (args->use_shmem) {
> -        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> -            g_test_skip("/dev/shm is not supported");
> -            return -1;
> -        }
> -    }

Maybe assert on: "!args->use_shmem || shm_supported()" here?

Either way:

Reviewed-by: Peter Xu <peterx@redhat.com>

> -
>      got_src_stop = false;
>      got_dst_resume = false;
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> @@ -2639,6 +2632,15 @@ static bool kvm_dirty_ring_supported(void)
>  #endif
>  }
>  
> +static bool shm_supported(void)
> +{
> +    if (g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> +        return true;
> +    }
> +    g_test_message("Skipping test: shared memory not available");
> +    return false;
> +}
> +
>  int main(int argc, char **argv)
>  {
>      bool has_kvm, has_tcg;
> @@ -2768,7 +2770,9 @@ int main(int argc, char **argv)
>  #endif /* CONFIG_TASN1 */
>  #endif /* CONFIG_GNUTLS */
>  
> -    qtest_add_func("/migration/ignore_shared", test_ignore_shared);
> +    if (shm_supported()) {
> +        qtest_add_func("/migration/ignore_shared", test_ignore_shared);
> +    }
>  #ifndef _WIN32
>      qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
>  #endif
> -- 
> 2.40.1
> 

-- 
Peter Xu
Re: [PATCH 13/42] migration-test: Check for shared memory like for everything else
Posted by Juan Quintela 2 years, 7 months ago 
Peter Xu <peterx@redhat.com> wrote:
> On Fri, Jun 09, 2023 at 12:49:14AM +0200, Juan Quintela wrote:
>> Makes things easier and cleaner.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  tests/qtest/migration-test.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>> 
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index daaf5cd71a..5837060138 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -645,13 +645,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>      const char *arch = qtest_get_arch();
>>      const char *memory_size;
>>  
>> -    if (args->use_shmem) {
>> -        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
>> -            g_test_skip("/dev/shm is not supported");
>> -            return -1;
>> -        }
>> -    }
>
> Maybe assert on: "!args->use_shmem || shm_supported()" here?

Nope.

We are being extra defensive in some tests.

It is tested here

>> -    qtest_add_func("/migration/ignore_shared", test_ignore_shared);
>> +    if (shm_supported()) {
>> +        qtest_add_func("/migration/ignore_shared", test_ignore_shared);
>> +    }

Checking (in the same code path) once in the same file looks like enough
to me.

Thanks, Juan.
Re: [PATCH 13/42] migration-test: Check for shared memory like for everything else
Posted by Peter Xu 2 years, 7 months ago 
On Wed, Jun 21, 2023 at 12:07:20PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Fri, Jun 09, 2023 at 12:49:14AM +0200, Juan Quintela wrote:
> >> Makes things easier and cleaner.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  tests/qtest/migration-test.c | 20 ++++++++++++--------
> >>  1 file changed, 12 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> >> index daaf5cd71a..5837060138 100644
> >> --- a/tests/qtest/migration-test.c
> >> +++ b/tests/qtest/migration-test.c
> >> @@ -645,13 +645,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
> >>      const char *arch = qtest_get_arch();
> >>      const char *memory_size;
> >>  
> >> -    if (args->use_shmem) {
> >> -        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
> >> -            g_test_skip("/dev/shm is not supported");
> >> -            return -1;
> >> -        }
> >> -    }
> >
> > Maybe assert on: "!args->use_shmem || shm_supported()" here?
> 
> Nope.
> 
> We are being extra defensive in some tests.

This will protect a new test passing in use_shmem=true without checking
shm_supported().  It'll then fail at starting the VM I think otherwise.

> 
> It is tested here
> 
> >> -    qtest_add_func("/migration/ignore_shared", test_ignore_shared);
> >> +    if (shm_supported()) {
> >> +        qtest_add_func("/migration/ignore_shared", test_ignore_shared);
> >> +    }
> 
> Checking (in the same code path) once in the same file looks like enough
> to me.
> 
> Thanks, Juan.
> 

-- 
Peter Xu
Re: [PATCH 13/42] migration-test: Check for shared memory like for everything else
Posted by Juan Quintela 2 years, 7 months ago 
Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jun 21, 2023 at 12:07:20PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Fri, Jun 09, 2023 at 12:49:14AM +0200, Juan Quintela wrote:
>> >> Makes things easier and cleaner.
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  tests/qtest/migration-test.c | 20 ++++++++++++--------
>> >>  1 file changed, 12 insertions(+), 8 deletions(-)
>> >> 
>> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> >> index daaf5cd71a..5837060138 100644
>> >> --- a/tests/qtest/migration-test.c
>> >> +++ b/tests/qtest/migration-test.c
>> >> @@ -645,13 +645,6 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>> >>      const char *arch = qtest_get_arch();
>> >>      const char *memory_size;
>> >>  
>> >> -    if (args->use_shmem) {
>> >> -        if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
>> >> -            g_test_skip("/dev/shm is not supported");
>> >> -            return -1;
>> >> -        }
>> >> -    }
>> >
>> > Maybe assert on: "!args->use_shmem || shm_supported()" here?
>> 
>> Nope.
>> 
>> We are being extra defensive in some tests.
>
> This will protect a new test passing in use_shmem=true without checking
> shm_supported().  It'll then fail at starting the VM I think otherwise.

Hi


As it should.
Test is wrong and it aborts.
It is not that it has found an error, it is that it is badly written.

And anyways, args->use_shmem dissapears later in the series O:-)

Later, Juan.

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.