On Wed, Feb 25, 2026 at 10:32:51AM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <[email protected]> writes: > > > On Wed, Feb 25, 2026 at 11:35:15AM +0530, Prasad Pandit wrote: > >> On Wed, 25 Feb 2026 at 09:23, Ani Sinha <[email protected]> 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 <[email protected]> > >> > Signed-off-by: Ani Sinha <[email protected]> > >> > --- > >> > 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 :|
