[PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[Peter Xu]

error-desc should present on dest QEMU after migration failed on dest when
exit-on-error is set to FALSE.  Check the error message.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration/precopy-tests.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index 57ca623de5..5f02e35324 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -759,6 +759,14 @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to,
     wait_for_migration_complete(to);
 }
 
+static void assert_migration_error(QTestState *vm)
+{
+    QDict *rep = migrate_query(vm);
+
+    g_assert(qdict_get_str(rep, "error-desc"));
+    qobject_unref(rep);
+}
+
 static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
                                            const char *uri, const char *phase)
 {
@@ -784,6 +792,7 @@ static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
 
     wait_for_migration_status(to, "failed",
                               (const char * []) { "completed", NULL });
+    assert_migration_error(to);
 }
 
 static void test_cancel_src_after_status(void *opaque)
-- 
2.50.1

Re: [PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[Juraj Marcin]

Hi Peter,

On 2025-10-31 12:49, Peter Xu wrote:
> error-desc should present on dest QEMU after migration failed on dest when
> exit-on-error is set to FALSE.  Check the error message.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/qtest/migration/precopy-tests.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index 57ca623de5..5f02e35324 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -759,6 +759,14 @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to,
>      wait_for_migration_complete(to);
>  }
>  
> +static void assert_migration_error(QTestState *vm)
> +{
> +    QDict *rep = migrate_query(vm);
> +
> +    g_assert(qdict_get_str(rep, "error-desc"));

I think it would be beneficial to also check if there even is
"error-desc". That way if the "error-desc" is missing, it fails on
assert with SIGABRT instead of SIGSEGV inside qdict_get_str().

With this change you can add my:

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>


diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
index 5f02e35324..87e33b8965 100644
--- a/tests/qtest/migration/precopy-tests.c
+++ b/tests/qtest/migration/precopy-tests.c
@@ -763,6 +763,7 @@ static void assert_migration_error(QTestState *vm)
 {
     QDict *rep = migrate_query(vm);

+    g_assert(qdict_get(rep, "error-desc"));
     g_assert(qdict_get_str(rep, "error-desc"));
     qobject_unref(rep);
 }


> +    qobject_unref(rep);
> +}
> +
>  static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
>                                             const char *uri, const char *phase)
>  {
> @@ -784,6 +792,7 @@ static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
>  
>      wait_for_migration_status(to, "failed",
>                                (const char * []) { "completed", NULL });
> +    assert_migration_error(to);
>  }
>  
>  static void test_cancel_src_after_status(void *opaque)
> -- 
> 2.50.1
> 

Re: [PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[Peter Xu]

On Tue, Nov 04, 2025 at 01:35:53PM +0100, Juraj Marcin wrote:
> Hi Peter,
> 
> On 2025-10-31 12:49, Peter Xu wrote:
> > error-desc should present on dest QEMU after migration failed on dest when
> > exit-on-error is set to FALSE.  Check the error message.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  tests/qtest/migration/precopy-tests.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> > index 57ca623de5..5f02e35324 100644
> > --- a/tests/qtest/migration/precopy-tests.c
> > +++ b/tests/qtest/migration/precopy-tests.c
> > @@ -759,6 +759,14 @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to,
> >      wait_for_migration_complete(to);
> >  }
> >  
> > +static void assert_migration_error(QTestState *vm)
> > +{
> > +    QDict *rep = migrate_query(vm);
> > +
> > +    g_assert(qdict_get_str(rep, "error-desc"));
> 
> I think it would be beneficial to also check if there even is
> "error-desc". That way if the "error-desc" is missing, it fails on
> assert with SIGABRT instead of SIGSEGV inside qdict_get_str().

IMHO it doesn't matter on how the test would crash.

> 
> With this change you can add my:
> 
> Reviewed-by: Juraj Marcin <jmarcin@redhat.com>

I would go ahead and merge a test patch if it had both lines, definitely
not a huge deal.

However strictly speaking, qdict_get_str() is actually pretty efficient to
make sure both that exists && is_string when used in testings. Would you
agree?

I definitely still want your R-b one way or another!

> 
> 
> diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> index 5f02e35324..87e33b8965 100644
> --- a/tests/qtest/migration/precopy-tests.c
> +++ b/tests/qtest/migration/precopy-tests.c
> @@ -763,6 +763,7 @@ static void assert_migration_error(QTestState *vm)
>  {
>      QDict *rep = migrate_query(vm);
> 
> +    g_assert(qdict_get(rep, "error-desc"));
>      g_assert(qdict_get_str(rep, "error-desc"));
>      qobject_unref(rep);
>  }
> 
> 
> > +    qobject_unref(rep);
> > +}
> > +
> >  static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> >                                             const char *uri, const char *phase)
> >  {
> > @@ -784,6 +792,7 @@ static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> >  
> >      wait_for_migration_status(to, "failed",
> >                                (const char * []) { "completed", NULL });
> > +    assert_migration_error(to);
> >  }
> >  
> >  static void test_cancel_src_after_status(void *opaque)
> > -- 
> > 2.50.1
> > 
> 

-- 
Peter Xu

Re: [PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[Juraj Marcin]

On 2025-11-05 14:52, Peter Xu wrote:
> On Tue, Nov 04, 2025 at 01:35:53PM +0100, Juraj Marcin wrote:
> > Hi Peter,
> > 
> > On 2025-10-31 12:49, Peter Xu wrote:
> > > error-desc should present on dest QEMU after migration failed on dest when
> > > exit-on-error is set to FALSE.  Check the error message.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  tests/qtest/migration/precopy-tests.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> > > index 57ca623de5..5f02e35324 100644
> > > --- a/tests/qtest/migration/precopy-tests.c
> > > +++ b/tests/qtest/migration/precopy-tests.c
> > > @@ -759,6 +759,14 @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to,
> > >      wait_for_migration_complete(to);
> > >  }
> > >  
> > > +static void assert_migration_error(QTestState *vm)
> > > +{
> > > +    QDict *rep = migrate_query(vm);
> > > +
> > > +    g_assert(qdict_get_str(rep, "error-desc"));
> > 
> > I think it would be beneficial to also check if there even is
> > "error-desc". That way if the "error-desc" is missing, it fails on
> > assert with SIGABRT instead of SIGSEGV inside qdict_get_str().
> 
> IMHO it doesn't matter on how the test would crash.
> 
> > 
> > With this change you can add my:
> > 
> > Reviewed-by: Juraj Marcin <jmarcin@redhat.com>
> 
> I would go ahead and merge a test patch if it had both lines, definitely
> not a huge deal.
> 
> However strictly speaking, qdict_get_str() is actually pretty efficient to
> make sure both that exists && is_string when used in testings. Would you
> agree?

It is an efficient way, I just thought the less efficient might be a
little bit easier to deduce why the test failed. But if nobody else
opposes, you can also keep it as proposed,

> 
> I definitely still want your R-b one way or another!

and also add my R-b.

> 
> > 
> > 
> > diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> > index 5f02e35324..87e33b8965 100644
> > --- a/tests/qtest/migration/precopy-tests.c
> > +++ b/tests/qtest/migration/precopy-tests.c
> > @@ -763,6 +763,7 @@ static void assert_migration_error(QTestState *vm)
> >  {
> >      QDict *rep = migrate_query(vm);
> > 
> > +    g_assert(qdict_get(rep, "error-desc"));
> >      g_assert(qdict_get_str(rep, "error-desc"));
> >      qobject_unref(rep);
> >  }
> > 
> > 
> > > +    qobject_unref(rep);
> > > +}
> > > +
> > >  static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> > >                                             const char *uri, const char *phase)
> > >  {
> > > @@ -784,6 +792,7 @@ static void test_cancel_src_pre_switchover(QTestState *from, QTestState *to,
> > >  
> > >      wait_for_migration_status(to, "failed",
> > >                                (const char * []) { "completed", NULL });
> > > +    assert_migration_error(to);
> > >  }
> > >  
> > >  static void test_cancel_src_after_status(void *opaque)
> > > -- 
> > > 2.50.1
> > > 
> > 
> 
> -- 
> Peter Xu
> 

Re: [PATCH] tests/migration-test: Check error-desc after pre-switch cancel tests

[Peter Xu]

On Thu, Nov 06, 2025 at 12:27:49PM +0100, Juraj Marcin wrote:
> On 2025-11-05 14:52, Peter Xu wrote:
> > On Tue, Nov 04, 2025 at 01:35:53PM +0100, Juraj Marcin wrote:
> > > Hi Peter,
> > > 
> > > On 2025-10-31 12:49, Peter Xu wrote:
> > > > error-desc should present on dest QEMU after migration failed on dest when
> > > > exit-on-error is set to FALSE.  Check the error message.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  tests/qtest/migration/precopy-tests.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/tests/qtest/migration/precopy-tests.c b/tests/qtest/migration/precopy-tests.c
> > > > index 57ca623de5..5f02e35324 100644
> > > > --- a/tests/qtest/migration/precopy-tests.c
> > > > +++ b/tests/qtest/migration/precopy-tests.c
> > > > @@ -759,6 +759,14 @@ static void test_cancel_src_after_none(QTestState *from, QTestState *to,
> > > >      wait_for_migration_complete(to);
> > > >  }
> > > >  
> > > > +static void assert_migration_error(QTestState *vm)
> > > > +{
> > > > +    QDict *rep = migrate_query(vm);
> > > > +
> > > > +    g_assert(qdict_get_str(rep, "error-desc"));
> > > 
> > > I think it would be beneficial to also check if there even is
> > > "error-desc". That way if the "error-desc" is missing, it fails on
> > > assert with SIGABRT instead of SIGSEGV inside qdict_get_str().
> > 
> > IMHO it doesn't matter on how the test would crash.
> > 
> > > 
> > > With this change you can add my:
> > > 
> > > Reviewed-by: Juraj Marcin <jmarcin@redhat.com>
> > 
> > I would go ahead and merge a test patch if it had both lines, definitely
> > not a huge deal.
> > 
> > However strictly speaking, qdict_get_str() is actually pretty efficient to
> > make sure both that exists && is_string when used in testings. Would you
> > agree?
> 
> It is an efficient way, I just thought the less efficient might be a
> little bit easier to deduce why the test failed. But if nobody else
> opposes, you can also keep it as proposed,
> 
> > 
> > I definitely still want your R-b one way or another!
> 
> and also add my R-b.

Thanks.  I wanted to have this coverage asap, so I collected it for -rc.

-- 
Peter Xu