migrate_add_blocker() can fail (e.g. if migration is already in
progress), in which case it returns a negative value and populates
its errp argument with the reason.

The previous code ignored the return value, leaving state
inconsistent on failure. Fix this by:
- Passing a local Error ** so failure details can be reported.
- Checking the return value; on failure, report the error, free the
  migration_blocker Error object, and return NULL to the caller.

Resolves: CID 1645470

Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
---
 plugins/api-system.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/plugins/api-system.c b/plugins/api-system.c
index 9a70b9caa6..f0d6cefe74 100644
--- a/plugins/api-system.c
+++ b/plugins/api-system.c
@@ -105,10 +105,16 @@ static Error *migration_blocker;
 const void *qemu_plugin_request_time_control(void)
 {
     if (!has_control) {
+        Error *local_err = NULL;
         has_control = true;
         error_setg(&migration_blocker,
                    "TCG plugin time control does not support migration");
-        migrate_add_blocker(&migration_blocker, NULL);
+        if (migrate_add_blocker(&migration_blocker, &local_err) < 0) {
+            error_report_err(local_err);
+            error_free(migration_blocker);
+            migration_blocker = NULL;
+            return NULL;
+        }
         return &has_control;
     }
     return NULL;
-- 
2.43.0

On Thu, 12 Mar 2026 at 12:17, Trieu Huynh <vikingtc4@gmail.com> wrote:
>
> migrate_add_blocker() can fail (e.g. if migration is already in
> progress), in which case it returns a negative value and populates
> its errp argument with the reason.
>
> The previous code ignored the return value, leaving state
> inconsistent on failure. Fix this by:
> - Passing a local Error ** so failure details can be reported.
> - Checking the return value; on failure, report the error, free the
>   migration_blocker Error object, and return NULL to the caller.
>
> Resolves: CID 1645470
>
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
>  plugins/api-system.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/api-system.c b/plugins/api-system.c
> index 9a70b9caa6..f0d6cefe74 100644
> --- a/plugins/api-system.c
> +++ b/plugins/api-system.c
> @@ -105,10 +105,16 @@ static Error *migration_blocker;
>  const void *qemu_plugin_request_time_control(void)
>  {
>      if (!has_control) {
> +        Error *local_err = NULL;
>          has_control = true;
>          error_setg(&migration_blocker,
>                     "TCG plugin time control does not support migration");
> -        migrate_add_blocker(&migration_blocker, NULL);
> +        if (migrate_add_blocker(&migration_blocker, &local_err) < 0) {
> +            error_report_err(local_err);
> +            error_free(migration_blocker);
> +            migration_blocker = NULL;

You don't need to free the migration_blocker Error if
migrate_add_blocker() fails -- the function documents that it
will do that for you and set the pointer to NULL.

> +            migration_blocker = NULL;
> +            return NULL;
> +        }
>          return &has_control;

thanks
-- PMM

On 3/12/26 5:16 AM, Trieu Huynh wrote:
> migrate_add_blocker() can fail (e.g. if migration is already in
> progress), in which case it returns a negative value and populates
> its errp argument with the reason.
> 
> The previous code ignored the return value, leaving state
> inconsistent on failure. Fix this by:
> - Passing a local Error ** so failure details can be reported.
> - Checking the return value; on failure, report the error, free the
>    migration_blocker Error object, and return NULL to the caller.
> 
> Resolves: CID 1645470
> 
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
>   plugins/api-system.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 

While the description makes sense, how would it happen in real world?

Plugins usually will request time control on startup (so no migration is 
happening). In all cases, plugin who want to control time will have no 
other choice than abort.
Maybe we could simply assert that migrate_add_blocker succeeded in this 
context.

Regards,
Pierrick

On Thu, 12 Mar 2026 at 18:02, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 3/12/26 5:16 AM, Trieu Huynh wrote:
> > migrate_add_blocker() can fail (e.g. if migration is already in
> > progress), in which case it returns a negative value and populates
> > its errp argument with the reason.
> >
> > The previous code ignored the return value, leaving state
> > inconsistent on failure. Fix this by:
> > - Passing a local Error ** so failure details can be reported.
> > - Checking the return value; on failure, report the error, free the
> >    migration_blocker Error object, and return NULL to the caller.
> >
> > Resolves: CID 1645470
> >
> > Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> > ---
> >   plugins/api-system.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
>
> While the description makes sense, how would it happen in real world?
>
> Plugins usually will request time control on startup (so no migration is
> happening). In all cases, plugin who want to control time will have no
> other choice than abort.
> Maybe we could simply assert that migrate_add_blocker succeeded in this
> context.

If the user starts QEMU with the --only-migratable option then
all attempts to install migration-blockers will fail. In this
case we need to report to the user that the thing they tried
to do (use a time-control plugin and also migrate) isn't possible,
i.e. exit cleanly with an error rather than falling over.

thanks
-- PMM

On 3/12/26 11:08 AM, Peter Maydell wrote:
> On Thu, 12 Mar 2026 at 18:02, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 3/12/26 5:16 AM, Trieu Huynh wrote:
>>> migrate_add_blocker() can fail (e.g. if migration is already in
>>> progress), in which case it returns a negative value and populates
>>> its errp argument with the reason.
>>>
>>> The previous code ignored the return value, leaving state
>>> inconsistent on failure. Fix this by:
>>> - Passing a local Error ** so failure details can be reported.
>>> - Checking the return value; on failure, report the error, free the
>>>     migration_blocker Error object, and return NULL to the caller.
>>>
>>> Resolves: CID 1645470
>>>
>>> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
>>> ---
>>>    plugins/api-system.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>
>> While the description makes sense, how would it happen in real world?
>>
>> Plugins usually will request time control on startup (so no migration is
>> happening). In all cases, plugin who want to control time will have no
>> other choice than abort.
>> Maybe we could simply assert that migrate_add_blocker succeeded in this
>> context.
> 
> If the user starts QEMU with the --only-migratable option then
> all attempts to install migration-blockers will fail. In this
> case we need to report to the user that the thing they tried
> to do (use a time-control plugin and also migrate) isn't possible,
> i.e. exit cleanly with an error rather than falling over.
>

Reporting errors is useful when there is a possibility to recover from 
it, but in this case it seems like we can only stop when user try to 
request time control, so I'm not sure we should return anything from 
this function at all instead of crashing.

> thanks
> -- PMM

On Thu, 12 Mar 2026 at 18:12, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 3/12/26 11:08 AM, Peter Maydell wrote:
> > On Thu, 12 Mar 2026 at 18:02, Pierrick Bouvier
> > <pierrick.bouvier@linaro.org> wrote:
> >>
> >> On 3/12/26 5:16 AM, Trieu Huynh wrote:
> >>> migrate_add_blocker() can fail (e.g. if migration is already in
> >>> progress), in which case it returns a negative value and populates
> >>> its errp argument with the reason.
> >>>
> >>> The previous code ignored the return value, leaving state
> >>> inconsistent on failure. Fix this by:
> >>> - Passing a local Error ** so failure details can be reported.
> >>> - Checking the return value; on failure, report the error, free the
> >>>     migration_blocker Error object, and return NULL to the caller.
> >>>
> >>> Resolves: CID 1645470
> >>>
> >>> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> >>> ---
> >>>    plugins/api-system.c | 8 +++++++-
> >>>    1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>
> >> While the description makes sense, how would it happen in real world?
> >>
> >> Plugins usually will request time control on startup (so no migration is
> >> happening). In all cases, plugin who want to control time will have no
> >> other choice than abort.
> >> Maybe we could simply assert that migrate_add_blocker succeeded in this
> >> context.
> >
> > If the user starts QEMU with the --only-migratable option then
> > all attempts to install migration-blockers will fail. In this
> > case we need to report to the user that the thing they tried
> > to do (use a time-control plugin and also migrate) isn't possible,
> > i.e. exit cleanly with an error rather than falling over.
> >
>
> Reporting errors is useful when there is a possibility to recover from
> it, but in this case it seems like we can only stop when user try to
> request time control, so I'm not sure we should return anything from
> this function at all instead of crashing.

When the user does something that is misguided QEMU should (as an ideal)
never crash -- a crash indicates a bug in QEMU. User mistakes should
cause QEMU to produce (hopefully helpful) error messages.

If we want to say "there is no sensible way for the plugin to recover
from this failure, we should just print the error message and exit QEMU"
then that is what error_fatal is for.

thanks
-- PMM

On 3/12/26 11:15 AM, Peter Maydell wrote:
> On Thu, 12 Mar 2026 at 18:12, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 3/12/26 11:08 AM, Peter Maydell wrote:
>>> On Thu, 12 Mar 2026 at 18:02, Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> On 3/12/26 5:16 AM, Trieu Huynh wrote:
>>>>> migrate_add_blocker() can fail (e.g. if migration is already in
>>>>> progress), in which case it returns a negative value and populates
>>>>> its errp argument with the reason.
>>>>>
>>>>> The previous code ignored the return value, leaving state
>>>>> inconsistent on failure. Fix this by:
>>>>> - Passing a local Error ** so failure details can be reported.
>>>>> - Checking the return value; on failure, report the error, free the
>>>>>      migration_blocker Error object, and return NULL to the caller.
>>>>>
>>>>> Resolves: CID 1645470
>>>>>
>>>>> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
>>>>> ---
>>>>>     plugins/api-system.c | 8 +++++++-
>>>>>     1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> While the description makes sense, how would it happen in real world?
>>>>
>>>> Plugins usually will request time control on startup (so no migration is
>>>> happening). In all cases, plugin who want to control time will have no
>>>> other choice than abort.
>>>> Maybe we could simply assert that migrate_add_blocker succeeded in this
>>>> context.
>>>
>>> If the user starts QEMU with the --only-migratable option then
>>> all attempts to install migration-blockers will fail. In this
>>> case we need to report to the user that the thing they tried
>>> to do (use a time-control plugin and also migrate) isn't possible,
>>> i.e. exit cleanly with an error rather than falling over.
>>>
>>
>> Reporting errors is useful when there is a possibility to recover from
>> it, but in this case it seems like we can only stop when user try to
>> request time control, so I'm not sure we should return anything from
>> this function at all instead of crashing.
> 
> When the user does something that is misguided QEMU should (as an ideal)
> never crash -- a crash indicates a bug in QEMU. User mistakes should
> cause QEMU to produce (hopefully helpful) error messages.
> 
> If we want to say "there is no sensible way for the plugin to recover
> from this failure, we should just print the error message and exit QEMU"
> then that is what error_fatal is for.
>

So yes, error_fatal is that we would like here.
My point was not how to stop execution (error_fatal, assert, or any 
other means) but to avoid returning a NULL handle on this case.

> thanks
> -- PMM

Regards,
Pierrick

migrate_add_blocker() can fail (e.g. if migration is already in
progress), in which case it returns a negative value and populates
its errp argument with the reason.

The previous code ignored the return value. Pass &error_fatal so
that on failure QEMU exits cleanly with an informative error message
rather than continuing in an inconsistent state.

Resolves: CID 1645470
Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
---
v2:
- Pass &error_fatal instead of returning NULL: the plugin has no
  recovery path, QEMU should exit cleanly with an error message.
  (Peter Maydell, Pierrick Bouvier)
- Remove manual error_free(migration_blocker) + migration_blocker = NULL:
  migrate_add_blocker() already does this on failure. (Peter Maydell)
---
 plugins/api-system.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/plugins/api-system.c b/plugins/api-system.c
index 9a70b9caa6..fbf905cd63 100644
--- a/plugins/api-system.c
+++ b/plugins/api-system.c
@@ -108,7 +108,7 @@ const void *qemu_plugin_request_time_control(void)
         has_control = true;
         error_setg(&migration_blocker,
                    "TCG plugin time control does not support migration");
-        migrate_add_blocker(&migration_blocker, NULL);
+        migrate_add_blocker(&migration_blocker, &error_fatal);
         return &has_control;
     }
     return NULL;
-- 
2.43.0

On 3/17/26 6:47 AM, Trieu Huynh wrote:
> migrate_add_blocker() can fail (e.g. if migration is already in
> progress), in which case it returns a negative value and populates
> its errp argument with the reason.
> 
> The previous code ignored the return value. Pass &error_fatal so
> that on failure QEMU exits cleanly with an informative error message
> rather than continuing in an inconsistent state.
> 
> Resolves: CID 1645470
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
> v2:
> - Pass &error_fatal instead of returning NULL: the plugin has no
>    recovery path, QEMU should exit cleanly with an error message.
>    (Peter Maydell, Pierrick Bouvier)
> - Remove manual error_free(migration_blocker) + migration_blocker = NULL:
>    migrate_add_blocker() already does this on failure. (Peter Maydell)
> ---
>   plugins/api-system.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/plugins/api-system.c b/plugins/api-system.c
> index 9a70b9caa6..fbf905cd63 100644
> --- a/plugins/api-system.c
> +++ b/plugins/api-system.c
> @@ -108,7 +108,7 @@ const void *qemu_plugin_request_time_control(void)
>           has_control = true;
>           error_setg(&migration_blocker,
>                      "TCG plugin time control does not support migration");
> -        migrate_add_blocker(&migration_blocker, NULL);
> +        migrate_add_blocker(&migration_blocker, &error_fatal);
>           return &has_control;
>       }
>       return NULL;

This was merged into master (aed38fe5206d848a8366181bfab96835fcd5643b).
Thank you for your contribution!

Regards,
Pierrick

On 3/17/26 6:47 AM, Trieu Huynh wrote:
> migrate_add_blocker() can fail (e.g. if migration is already in
> progress), in which case it returns a negative value and populates
> its errp argument with the reason.
> 
> The previous code ignored the return value. Pass &error_fatal so
> that on failure QEMU exits cleanly with an informative error message
> rather than continuing in an inconsistent state.
> 
> Resolves: CID 1645470
> Signed-off-by: Trieu Huynh <vikingtc4@gmail.com>
> ---
> v2:
> - Pass &error_fatal instead of returning NULL: the plugin has no
>    recovery path, QEMU should exit cleanly with an error message.
>    (Peter Maydell, Pierrick Bouvier)
> - Remove manual error_free(migration_blocker) + migration_blocker = NULL:
>    migrate_add_blocker() already does this on failure. (Peter Maydell)
> ---
>   plugins/api-system.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/plugins/api-system.c b/plugins/api-system.c
> index 9a70b9caa6..fbf905cd63 100644
> --- a/plugins/api-system.c
> +++ b/plugins/api-system.c
> @@ -108,7 +108,7 @@ const void *qemu_plugin_request_time_control(void)
>           has_control = true;
>           error_setg(&migration_blocker,
>                      "TCG plugin time control does not support migration");
> -        migrate_add_blocker(&migration_blocker, NULL);
> +        migrate_add_blocker(&migration_blocker, &error_fatal);
>           return &has_control;
>       }
>       return NULL;

This looks better.
Just a small process nit, new versions should be posted as a new thread, 
and not as a reply to your previous version. No need to repost though.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

On error, correctly prints:
qemu-system-aarch64: disallowing migration blocker (--only-migratable) 
for: TCG plugin time control does not support migration

Regards,
Pierrick