Re: [PATCH v6 35/35] migration: return EEXIST when trying to add the same migration blocker

[Peter Xu <peterx@redhat.com>]

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