On Mon, Mar 2, 2026 at 3:01 PM Peter Xu <[email protected]> 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 <[email protected]> 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 >
