Re: [PATCH v2 3/6] migration: Remove 'blk/-b' option from migrate commands

[Peter Xu]

On Mon, Apr 29, 2024 at 03:35:02PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Apr 29, 2024 at 02:18:57PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Fri, Apr 26, 2024 at 10:14:05AM -0300, Fabiano Rosas wrote:
> >> >> @@ -2003,21 +1997,7 @@ static bool migrate_prepare(MigrationState *s, 
> >> >> bool blk, bool resume,
> >> >>          }
> >> >>      }
> >> >>  
> >> >> -    if (blk) {

[1]

> >> >> -        if (migrate_colo()) {
> >> >> -            error_setg(errp, "No disk migration is required in COLO 
> >> >> mode");
> >> >> -            return false;
> >> >> -        }
> >> >> -        if (migrate_block()) {
> >> >> -            error_setg(errp, "Command options are incompatible with "
> >> >> -                       "current migration capabilities");
> >> >> -            return false;
> >> >> -        }
> >> >> -        if (!migrate_cap_set(MIGRATION_CAPABILITY_BLOCK, true, errp)) {
> >> >> -            return false;
> >> >> -        }
> >> >> -        s->must_remove_block_options = true;
> >> >> -    }
> >> >> +    s->must_remove_block_options = true;
> >> >
> >> > Can we drop this var too?  Perhaps with block_cleanup_parameters()?
> >> 
> >> Yes, Markus mentioned it in v1 already. Take a look there. There's
> >> several other declarations I missed. v3 is coming soon.
> >
> > Right, noticed that it's removed actually in the next patch.
> >
> > But iiuc it can already been removed in this patch.  If we want to remove
> > it in the next, logically we should set must_remove_block_options=false
> > here, though..  So maybe easier to just drop it here.
> 
> Ah I see what you mean. I thought you're just asking for the removal
> overall.
> 
> But block_cleanup_parameters sets the block capability to false and the
> whole block migration only goes away in the next patch. I think we need
> to keep this as true to preserve behavior. In theory, after this patch
> people could still use the block migration just fine by setting the cap.

I'm reading this patch the same as "blk==false" always above [1].  In that
case, only if we keep must_remove_block_options=false could we maintain the
behavior?  Otherwise at the end of migration (or cancellations) QEMU can
wrongly clear MIGRATION_CAPABILITY_BLOCK bit if the user set it manually?

(But hey, we're discussing whoever applies this patch only without the
 rest..  definitely not a huge deal :)

-- 
Peter Xu