Currently the code that adds a migration blocker does not check if the same
blocker already exists. Return an EEXIST error code if there is an attempt to
add the same migration blocker again. This way the same migration blocker will
not get added twice.

Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index a5b0465ed3..1eb75fb7fb 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
         if (modes & BIT(mode)) {
+            if (g_slist_index(migration_blockers[mode],
+                                 *reasonp) >= 0) {
+                return -EEXIST;
+            }
             migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
                                                        *reasonp);
         }
-- 
2.42.0

Hi, Ani,

On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
> Currently the code that adds a migration blocker does not check if the same
> blocker already exists. Return an EEXIST error code if there is an attempt to
> add the same migration blocker again. This way the same migration blocker will
> not get added twice.

Could you help explain why it will inject two identical errors in the first
place, and why the caller cannot make sure it won't be injected twice?

Thanks,

> 
> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a5b0465ed3..1eb75fb7fb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>  {
>      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>          if (modes & BIT(mode)) {
> +            if (g_slist_index(migration_blockers[mode],
> +                                 *reasonp) >= 0) {
> +                return -EEXIST;
> +            }
>              migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>                                                         *reasonp);
>          }
> -- 
> 2.42.0
> 

-- 
Peter Xu

> On 25 Feb 2026, at 10:59 PM, Peter Xu <peterx@redhat.com> wrote:
> 
> Hi, Ani,
> 
> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>> Currently the code that adds a migration blocker does not check if the same
>> blocker already exists. Return an EEXIST error code if there is an attempt to
>> add the same migration blocker again. This way the same migration blocker will
>> not get added twice.
> 
> Could you help explain why it will inject two identical errors in the first
> place, and why the caller cannot make sure it won't be injected twice?

Likely due to a bug in the code. For example if the init function that adds the blocker is called again and the caller does not handle the second init call properly.
This came up as a part of the coco reset work where migration blockers are added in init methods. They need not be added again when init methods are again called during the reset process. The caller can handle it of course but adding a check further down the call stack makes things more robust.


> 
> Thanks,
> 
>> 
>> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> migration/migration.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index a5b0465ed3..1eb75fb7fb 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>> {
>>     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>         if (modes & BIT(mode)) {
>> +            if (g_slist_index(migration_blockers[mode],
>> +                                 *reasonp) >= 0) {
>> +                return -EEXIST;
>> +            }
>>             migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>>                                                        *reasonp);
>>         }
>> -- 
>> 2.42.0
>> 
> 
> -- 
> Peter Xu

On 2/26/26 04:46, Ani Sinha wrote:
> 
> 
>> On 25 Feb 2026, at 10:59 PM, Peter Xu <peterx@redhat.com> wrote:
>> 
>> Hi, Ani,
>> 
>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>>> Currently the code that adds a migration blocker does not check
>>> if the same blocker already exists. Return an EEXIST error code
>>> if there is an attempt to add the same migration blocker again.
>>> This way the same migration blocker will not get added twice.
>> 
>> Could you help explain why it will inject two identical errors in
>> the first place, and why the caller cannot make sure it won't be
>> injected twice?
> 
> Likely due to a bug in the code. For example if the init function
> that adds the blocker is called again and the caller does not handle
> the second init call properly. This came up as a part of the coco
> reset work where migration blockers are added in init methods. They
> need not be added again when init methods are again called during
> the reset process. The caller can handle it of course but adding a
> check further down the call stack makes things more robust.
Since this is the last patch, is it okay to remove it at least for now? 
Can the situation actually happen?

If not and it's a programming error, even an abort() is okay.

Paolo

> On 26 Feb 2026, at 10:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 2/26/26 04:46, Ani Sinha wrote:
>>> On 25 Feb 2026, at 10:59 PM, Peter Xu <peterx@redhat.com> wrote:
>>> Hi, Ani,
>>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>>>> Currently the code that adds a migration blocker does not check
>>>> if the same blocker already exists. Return an EEXIST error code
>>>> if there is an attempt to add the same migration blocker again.
>>>> This way the same migration blocker will not get added twice.
>>> Could you help explain why it will inject two identical errors in
>>> the first place, and why the caller cannot make sure it won't be
>>> injected twice?
>> Likely due to a bug in the code. For example if the init function
>> that adds the blocker is called again and the caller does not handle
>> the second init call properly. This came up as a part of the coco
>> reset work where migration blockers are added in init methods. They
>> need not be added again when init methods are again called during
>> the reset process. The caller can handle it of course but adding a
>> check further down the call stack makes things more robust.
> Since this is the last patch, is it okay to remove it at least for now? Can the situation actually happen?
> 

Yes you can drop this patch for now. When we get consensus if/what needs to be done, I can send another fix. 

> If not and it's a programming error, even an abort() is okay.

On Thu, Feb 26, 2026 at 09:16:43AM +0530, Ani Sinha wrote:
> 
> 
> > On 25 Feb 2026, at 10:59 PM, Peter Xu <peterx@redhat.com> wrote:
> > 
> > Hi, Ani,
> > 
> > On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
> >> Currently the code that adds a migration blocker does not check if the same
> >> blocker already exists. Return an EEXIST error code if there is an attempt to
> >> add the same migration blocker again. This way the same migration blocker will
> >> not get added twice.
> > 
> > Could you help explain why it will inject two identical errors in the first
> > place, and why the caller cannot make sure it won't be injected twice?
> 
> Likely due to a bug in the code. For example if the init function that
> adds the blocker is called again and the caller does not handle the
> second init call properly.  This came up as a part of the coco reset work
> where migration blockers are added in init methods. They need not be
> added again when init methods are again called during the reset
> process. The caller can handle it of course but adding a check further
> down the call stack makes things more robust.

IMHO if we want to make it more robust, we shouldn't return an error
because the caller may not always check for errors.

Would assertion suites more here?  Because migration blockers are not
something the user can manipulate, so it's a solid bug to fix when
triggered.

Thanks,

-- 
Peter Xu

> On 26 Feb 2026, at 6:38 PM, Peter Xu <peterx@redhat.com> wrote:
> 
> On Thu, Feb 26, 2026 at 09:16:43AM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 25 Feb 2026, at 10:59 PM, Peter Xu <peterx@redhat.com> wrote:
>>> 
>>> Hi, Ani,
>>> 
>>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>>>> Currently the code that adds a migration blocker does not check if the same
>>>> blocker already exists. Return an EEXIST error code if there is an attempt to
>>>> add the same migration blocker again. This way the same migration blocker will
>>>> not get added twice.
>>> 
>>> Could you help explain why it will inject two identical errors in the first
>>> place, and why the caller cannot make sure it won't be injected twice?
>> 
>> Likely due to a bug in the code. For example if the init function that
>> adds the blocker is called again and the caller does not handle the
>> second init call properly.  This came up as a part of the coco reset work
>> where migration blockers are added in init methods. They need not be
>> added again when init methods are again called during the reset
>> process. The caller can handle it of course but adding a check further
>> down the call stack makes things more robust.
> 
> IMHO if we want to make it more robust, we shouldn't return an error
> because the caller may not always check for errors.
> 
> Would assertion suites more here?  Because migration blockers are not
> something the user can manipulate, so it's a solid bug to fix when
> triggered.

If Prasad agrees, I will send something.

> 
> Thanks,
> 
> -- 
> Peter Xu

Hello all,

On Thu, 26 Feb 2026 at 21:46, Ani Sinha <anisinha@redhat.com> wrote:
> >>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
> >>>> Currently the code that adds a migration blocker does not check if the same
> >>>> blocker already exists. Return an EEXIST error code if there is an attempt to
> >>>> add the same migration blocker again. This way the same migration blocker will
> >>>> not get added twice.
> >>>
> >>> Could you help explain why it will inject two identical errors in the first
> >>> place, and why the caller cannot make sure it won't be injected twice?
> >>
> >> Likely due to a bug in the code. For example if the init function that
> >> adds the blocker is called again and the caller does not handle the
> >> second init call properly.  This came up as a part of the coco reset work
> >> where migration blockers are added in init methods. They need not be
> >> added again when init methods are again called during the reset
> >> process. The caller can handle it of course but adding a check further
> >> down the call stack makes things more robust.
> >
> > IMHO if we want to make it more robust, we shouldn't return an error
> > because the caller may not always check for errors.
> >
> > Would assertion suites more here?  Because migration blockers are not
> > something the user can manipulate, so it's a solid bug to fix when
> > triggered.
>
> If Prasad agrees, I will send something.

* The majority of the places I see constructs like:
===
        if (migrate_add_blocker(&g->migration_blocker, errp) < 0) {
OR
        ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err);
        if (ret < 0) {
            error_report_err(local_err);
===

* So setting **errp and returning a negative value is consistent with
that. Aborting (assert(3)) for trying to add a duplicate blocker
object seems like a harsh punishment, considering that'll happen at
run time and the user won't be able to do much then.

Thank you.
---
  - Prasad

On Mon, Mar 02, 2026 at 04:58:50PM +0530, Prasad Pandit wrote:
> Hello all,
> 
> On Thu, 26 Feb 2026 at 21:46, Ani Sinha <anisinha@redhat.com> wrote:
> > >>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
> > >>>> Currently the code that adds a migration blocker does not check if the same
> > >>>> blocker already exists. Return an EEXIST error code if there is an attempt to
> > >>>> add the same migration blocker again. This way the same migration blocker will
> > >>>> not get added twice.
> > >>>
> > >>> Could you help explain why it will inject two identical errors in the first
> > >>> place, and why the caller cannot make sure it won't be injected twice?
> > >>
> > >> Likely due to a bug in the code. For example if the init function that
> > >> adds the blocker is called again and the caller does not handle the
> > >> second init call properly.  This came up as a part of the coco reset work
> > >> where migration blockers are added in init methods. They need not be
> > >> added again when init methods are again called during the reset
> > >> process. The caller can handle it of course but adding a check further
> > >> down the call stack makes things more robust.
> > >
> > > IMHO if we want to make it more robust, we shouldn't return an error
> > > because the caller may not always check for errors.
> > >
> > > Would assertion suites more here?  Because migration blockers are not
> > > something the user can manipulate, so it's a solid bug to fix when
> > > triggered.
> >
> > If Prasad agrees, I will send something.
> 
> * The majority of the places I see constructs like:
> ===
>         if (migrate_add_blocker(&g->migration_blocker, errp) < 0) {
> OR
>         ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err);
>         if (ret < 0) {
>             error_report_err(local_err);
> ===
> 
> * So setting **errp and returning a negative value is consistent with
> that. Aborting (assert(3)) for trying to add a duplicate blocker
> object seems like a harsh punishment, considering that'll happen at
> run time and the user won't be able to do much then.

If it's a programming error, then it shouldn't happen at runtime.  The
current paths to fail this API was not for programming errors.

If this patch isn't required, IMHO we can at least drop it in this series
and make it separate.

Thanks,

-- 
Peter Xu

> On 3 Mar 2026, at 1:31 AM, Peter Xu <peterx@redhat.com> wrote:
> 
> On Mon, Mar 02, 2026 at 04:58:50PM +0530, Prasad Pandit wrote:
>> Hello all,
>> 
>> On Thu, 26 Feb 2026 at 21:46, Ani Sinha <anisinha@redhat.com> wrote:
>>>>>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
>>>>>>> Currently the code that adds a migration blocker does not check if the same
>>>>>>> blocker already exists. Return an EEXIST error code if there is an attempt to
>>>>>>> add the same migration blocker again. This way the same migration blocker will
>>>>>>> not get added twice.
>>>>>> 
>>>>>> Could you help explain why it will inject two identical errors in the first
>>>>>> place, and why the caller cannot make sure it won't be injected twice?
>>>>> 
>>>>> Likely due to a bug in the code. For example if the init function that
>>>>> adds the blocker is called again and the caller does not handle the
>>>>> second init call properly.  This came up as a part of the coco reset work
>>>>> where migration blockers are added in init methods. They need not be
>>>>> added again when init methods are again called during the reset
>>>>> process. The caller can handle it of course but adding a check further
>>>>> down the call stack makes things more robust.
>>>> 
>>>> IMHO if we want to make it more robust, we shouldn't return an error
>>>> because the caller may not always check for errors.
>>>> 
>>>> Would assertion suites more here?  Because migration blockers are not
>>>> something the user can manipulate, so it's a solid bug to fix when
>>>> triggered.
>>> 
>>> If Prasad agrees, I will send something.
>> 
>> * The majority of the places I see constructs like:
>> ===
>>        if (migrate_add_blocker(&g->migration_blocker, errp) < 0) {
>> OR
>>        ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err);
>>        if (ret < 0) {
>>            error_report_err(local_err);
>> ===
>> 
>> * So setting **errp and returning a negative value is consistent with
>> that. Aborting (assert(3)) for trying to add a duplicate blocker
>> object seems like a harsh punishment, considering that'll happen at
>> run time and the user won't be able to do much then.
> 
> If it's a programming error, then it shouldn't happen at runtime.  The
> current paths to fail this API was not for programming errors.

I have sent a patch with an assertion added
https://mail.gnu.org/archive/html/qemu-devel/2026-03/msg02602.html


> 
> If this patch isn't required, IMHO we can at least drop it in this series
> and make it separate.
> 
> Thanks,
> 
> -- 
> Peter Xu

On Mon, Mar 2, 2026 at 3:01 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Mar 02, 2026 at 04:58:50PM +0530, Prasad Pandit wrote:
> > Hello all,
> >
> > On Thu, 26 Feb 2026 at 21:46, Ani Sinha <anisinha@redhat.com> wrote:
> > > >>> On Wed, Feb 25, 2026 at 09:19:40AM +0530, Ani Sinha wrote:
> > > >>>> Currently the code that adds a migration blocker does not check if the same
> > > >>>> blocker already exists. Return an EEXIST error code if there is an attempt to
> > > >>>> add the same migration blocker again. This way the same migration blocker will
> > > >>>> not get added twice.
> > > >>>
> > > >>> Could you help explain why it will inject two identical errors in the first
> > > >>> place, and why the caller cannot make sure it won't be injected twice?
> > > >>
> > > >> Likely due to a bug in the code. For example if the init function that
> > > >> adds the blocker is called again and the caller does not handle the
> > > >> second init call properly.  This came up as a part of the coco reset work
> > > >> where migration blockers are added in init methods. They need not be
> > > >> added again when init methods are again called during the reset
> > > >> process. The caller can handle it of course but adding a check further
> > > >> down the call stack makes things more robust.
> > > >
> > > > IMHO if we want to make it more robust, we shouldn't return an error
> > > > because the caller may not always check for errors.
> > > >
> > > > Would assertion suites more here?  Because migration blockers are not
> > > > something the user can manipulate, so it's a solid bug to fix when
> > > > triggered.
> > >
> > > If Prasad agrees, I will send something.
> >
> > * The majority of the places I see constructs like:
> > ===
> >         if (migrate_add_blocker(&g->migration_blocker, errp) < 0) {
> > OR
> >         ret = migrate_add_blocker(&hv_no_nonarch_cs_mig_blocker, &local_err);
> >         if (ret < 0) {
> >             error_report_err(local_err);
> > ===
> >
> > * So setting **errp and returning a negative value is consistent with
> > that. Aborting (assert(3)) for trying to add a duplicate blocker
> > object seems like a harsh punishment, considering that'll happen at
> > run time and the user won't be able to do much then.
>
> If it's a programming error, then it shouldn't happen at runtime.  The
> current paths to fail this API was not for programming errors.
>
> If this patch isn't required, IMHO we can at least drop it in this series
> and make it separate.
>

Yes this patch is dropped.

> Thanks,
>
> --
> Peter Xu
>

On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
> Currently the code that adds a migration blocker does not check if the same
> blocker already exists. Return an EEXIST error code if there is an attempt to
> add the same migration blocker again. This way the same migration blocker will
> not get added twice.
>
> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index a5b0465ed3..1eb75fb7fb 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>  {
>      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>          if (modes & BIT(mode)) {
> +            if (g_slist_index(migration_blockers[mode],
> +                                 *reasonp) >= 0) {
> +                return -EEXIST;
> +            }
>              migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>                                                         *reasonp);
>          }

* Looks okay.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad

On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
> > Currently the code that adds a migration blocker does not check if the same
> > blocker already exists. Return an EEXIST error code if there is an attempt to
> > add the same migration blocker again. This way the same migration blocker will
> > not get added twice.
> >
> > Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> > ---
> >  migration/migration.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index a5b0465ed3..1eb75fb7fb 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)

This method has an  '**errp' parameter.....

> >  {
> >      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> >          if (modes & BIT(mode)) {
> > +            if (g_slist_index(migration_blockers[mode],
> > +                                 *reasonp) >= 0) {
> > +                return -EEXIST;

.... so using  -errno for return values is not appropriate - it must
set 'errp' and return -1.

> > +            }
> >              migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
> >                                                         *reasonp);
> >          }
> 
> * Looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> 
> Thank you.
> ---
>   - Prasad
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

> On 25 Feb 2026, at 2:37 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
>> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
>>> Currently the code that adds a migration blocker does not check if the same
>>> blocker already exists. Return an EEXIST error code if there is an attempt to
>>> add the same migration blocker again. This way the same migration blocker will
>>> not get added twice.
>>> 
>>> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>> ---
>>> migration/migration.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index a5b0465ed3..1eb75fb7fb 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
> 
> This method has an  '**errp' parameter.....
> 
>>> {
>>>     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>         if (modes & BIT(mode)) {
>>> +            if (g_slist_index(migration_blockers[mode],
>>> +                                 *reasonp) >= 0) {
>>> +                return -EEXIST;
> 
> .... so using  -errno for return values is not appropriate - it must
> set 'errp' and return -1.

I see functions migrate_add_blocker_modes() or migrate_add_blocker_internal() that do not do this. This is why I thought it was ok to just return -errno here.


> 
>>> +            }
>>>             migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>>>                                                        *reasonp);
>>>         }
>> 
>> * Looks okay.
>> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>> 
>> Thank you.
>> ---
>>  - Prasad
>> 
>> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

On Wed, Feb 25, 2026 at 03:04:27PM +0530, Ani Sinha wrote:
> 
> 
> > On 25 Feb 2026, at 2:37 PM, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 
> > On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
> >> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
> >>> Currently the code that adds a migration blocker does not check if the same
> >>> blocker already exists. Return an EEXIST error code if there is an attempt to
> >>> add the same migration blocker again. This way the same migration blocker will
> >>> not get added twice.
> >>> 
> >>> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> >>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >>> ---
> >>> migration/migration.c | 4 ++++
> >>> 1 file changed, 4 insertions(+)
> >>> 
> >>> diff --git a/migration/migration.c b/migration/migration.c
> >>> index a5b0465ed3..1eb75fb7fb 100644
> >>> --- a/migration/migration.c
> >>> +++ b/migration/migration.c
> >>> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
> > 
> > This method has an  '**errp' parameter.....
> > 
> >>> {
> >>>     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> >>>         if (modes & BIT(mode)) {
> >>> +            if (g_slist_index(migration_blockers[mode],
> >>> +                                 *reasonp) >= 0) {
> >>> +                return -EEXIST;
> > 
> > .... so using  -errno for return values is not appropriate - it must
> > set 'errp' and return -1.
> 
> I see functions migrate_add_blocker_modes() or migrate_add_blocker_internal()
> that do not do this. This is why I thought it was ok to just return -errno here.

Yes they all set 'errp', but indirectly:

    if (is_only_migratable(reasonp, modes, errp)) {
        return -EACCES;
    } else if (is_busy(reasonp, errp)) {
        return -EBUSY;
    }

In this case 'is_busy' and 'is_only_migratable' will be setting 'errp'.


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
>> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
>> > Currently the code that adds a migration blocker does not check if the same
>> > blocker already exists. Return an EEXIST error code if there is an attempt to
>> > add the same migration blocker again. This way the same migration blocker will
>> > not get added twice.
>> >
>> > Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
>> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> > ---
>> >  migration/migration.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index a5b0465ed3..1eb75fb7fb 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>
> This method has an  '**errp' parameter.....
>
>> >  {
>> >      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>> >          if (modes & BIT(mode)) {
>> > +            if (g_slist_index(migration_blockers[mode],
>> > +                                 *reasonp) >= 0) {
>> > +                return -EEXIST;
>
> .... so using  -errno for return values is not appropriate - it must
> set 'errp' and return -1.

Yes, it must set @errp on failure.  Returning -1 would be wrong,
because:

    int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
    {
        if (is_only_migratable(reasonp, modes, errp)) {
            return -EACCES;
        } else if (is_busy(reasonp, errp)) {
            return -EBUSY;
        }
        return add_blockers(reasonp, modes, errp);
    }

If callers don't actually care for error codes, the entire nest of
functions could be simplified to return 0/-1 or true/false.

>> > +            }
>> >              migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>> >                                                         *reasonp);
>> >          }
>> 
>> * Looks okay.
>> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>> 
>> Thank you.
>> ---
>>   - Prasad
>> 
>> 
>
> With regards,
> Daniel

On Wed, Feb 25, 2026 at 10:32:51AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
> >> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
> >> > Currently the code that adds a migration blocker does not check if the same
> >> > blocker already exists. Return an EEXIST error code if there is an attempt to
> >> > add the same migration blocker again. This way the same migration blocker will
> >> > not get added twice.
> >> >
> >> > Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
> >> > Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >> > ---
> >> >  migration/migration.c | 4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/migration/migration.c b/migration/migration.c
> >> > index a5b0465ed3..1eb75fb7fb 100644
> >> > --- a/migration/migration.c
> >> > +++ b/migration/migration.c
> >> > @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
> >
> > This method has an  '**errp' parameter.....
> >
> >> >  {
> >> >      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
> >> >          if (modes & BIT(mode)) {
> >> > +            if (g_slist_index(migration_blockers[mode],
> >> > +                                 *reasonp) >= 0) {
> >> > +                return -EEXIST;
> >
> > .... so using  -errno for return values is not appropriate - it must
> > set 'errp' and return -1.
> 
> Yes, it must set @errp on failure.  Returning -1 would be wrong,
> because:
> 
>     int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>     {
>         if (is_only_migratable(reasonp, modes, errp)) {
>             return -EACCES;
>         } else if (is_busy(reasonp, errp)) {
>             return -EBUSY;
>         }
>         return add_blockers(reasonp, modes, errp);
>     }
> 
> If callers don't actually care for error codes, the entire nest of
> functions could be simplified to return 0/-1 or true/false.

AFAICT there is only 1 caller that cares

spapr_mce_req_event tries to block migration while handling machine
check exceptions, and if seeing EBUSY it will report a warning
message.  I'm not convinced it should be checking for EBUSY at all.

If -only-migratable is set, it seems appropriate that it report
the same warning message, as it is unable to block the migration.


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

> On 25 Feb 2026, at 3:02 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote:
>>> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <anisinha@redhat.com> wrote:
>>>> Currently the code that adds a migration blocker does not check if the same
>>>> blocker already exists. Return an EEXIST error code if there is an attempt to
>>>> add the same migration blocker again. This way the same migration blocker will
>>>> not get added twice.
>>>> 
>>>> Suggested-by: Prasad Pandit <pjp@fedoraproject.org>
>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>> ---
>>>> migration/migration.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index a5b0465ed3..1eb75fb7fb 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -1702,6 +1702,10 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>> 
>> This method has an  '**errp' parameter.....
>> 
>>>> {
>>>>     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>>>         if (modes & BIT(mode)) {
>>>> +            if (g_slist_index(migration_blockers[mode],
>>>> +                                 *reasonp) >= 0) {
>>>> +                return -EEXIST;
>> 
>> .... so using  -errno for return values is not appropriate - it must
>> set 'errp' and return -1.
> 
> Yes, it must set @errp on failure.  Returning -1 would be wrong,

Ok so just

diff --git a/migration/migration.c b/migration/migration.c
index 1eb75fb7fb..dc75d2eeb6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1704,6 +1704,8 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
         if (modes & BIT(mode)) {
             if (g_slist_index(migration_blockers[mode],
                                  *reasonp) >= 0) {
+                error_propagate_prepend(errp, *reasonp,
+                                        "migration blocker already added ");
                 return -EEXIST;
             }
             migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],


> because:
> 
>    int migrate_add_blocker_modes(Error **reasonp, unsigned modes, Error **errp)
>    {
>        if (is_only_migratable(reasonp, modes, errp)) {
>            return -EACCES;
>        } else if (is_busy(reasonp, errp)) {
>            return -EBUSY;
>        }
>        return add_blockers(reasonp, modes, errp);
>    }
> 
> If callers don't actually care for error codes, the entire nest of
> functions could be simplified to return 0/-1 or true/false.
> 
>>>> +            }
>>>>             migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
>>>>                                                        *reasonp);
>>>>         }
>>> 
>>> * Looks okay.
>>> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>>> 
>>> Thank you.
>>> ---
>>>  - Prasad
>>> 
>>> 
>> 
>> With regards,
>> Daniel