[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

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

[Peter Xu]

On Fri, Oct 31, 2025 at 12:49:56PM -0400, 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"));
> +    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);

While I was running more tests I found this assertion might still trigger
but only randomly.  I think it might be caused by some migration failure
path not setting the error string even if it'll fail the migration.  I'll
unqueuing this one for now and put this into backlog as of now..

-- 
Peter Xu