Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> Eric Blake <address@hidden> wrote:
> > On 05/12/2017 05:55 AM, Juan Quintela wrote:
> >>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, 
> >>>> bool blk,
> >>>>      }
> >>>>  
> >>>>      if (has_inc && inc) {
> >>>> +        migrate_set_block_enabled(s, true);
> >>>>          migrate_set_block_shared(s, true);
> >>>
> >>> [2]
> >>>
> >>> IIUC for [1] & [2] we are solving the same problem that "shared"
> >>> depends on "enabled" bit. Would it be good to unitfy this dependency
> >>> somewhere? E.g., by changing migrate_set_block_shared() into:
> >>>
> >>> void migrate_set_block_shared(MigrationState *s, bool value)
> >>> {
> >>>     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >>>     if (value) {
> >>>         migrate_set_block_enabled(s, true);
> >>>     }
> >>> }
> >> 
> >> ok with this.
> >
> > Or, as I commented on 1/3, maybe having a single property that is a
> > tri-state enum value, instead of 2 separate boolean properties, might be
> > nicer (but certainly a bit more complex to code up).
> 
> If you teach me how to do the qapi/qmp part, I will do the other bits.
> I don't really care if we do it one way or the other.
> 
> >> I will add once here that when we disable block enabled, we also disable
> >> shared, or just let it that way?
> >> 
> >>> Another thing to mention: after switching to the capability interface,
> >>> we'll cache the "enabled" and "shared" bits now while we don't cache
> >>> it before, right? IIUC it'll affect behavior of such sequence:
> >>>
> >>> - 1st migrate with enabled=1, shared=1, then
> >>> - 2nd migrate with enabled=0, shared=0
> >>>
> >>> Before the series, the 2nd migrate will use enabled=shared=0, but
> >>> after the series it should be using enabled=shared=1. Not sure whether
> >>> this would be a problem (or I missed anything?).
> >> 
> >> We can't be consistent with both old/new way.
> >> 
> >> Old way: we always setup the capabilities on command line (that should
> >> have been deprecated long, long ago)
> >
> > Well, the easy way out is to have the HMP migrate command (I assume
> > that's what you mean by "on command line") explicitly clear the
> > parameters if it is called without the -b/-i flag.  So the start of each
> > migration is what changes the properties, so long as you are still using
> > HMP to start the migration.  Or, on the QMP side, since 'migrate' has
> > optional 'blk' and 'inc' booleans, basically leave the settings alone if
> > the parameters were omitted, and explicitly update the property to the
> > value of those parameters if they were present.
> 
> We are going to have trouble whatever way that we do it, or we start
> doing lots of strange things.
> 
> Forget about qmp, we are going to assume that it is consistent with hmp.
> 
> migrate_set_capabilities block_enabled on
> migrate -b .....
> 
> Should migrate disable the block_enabled capability?  Give one
> warning/error?
> 
> And notice that this only matter if we do a migration, we cancel/get one
> error, and then we migrate again.
> 
> What I tried to do is assume that -b/-i arguments don't exist.  And if
> the user use them, we implement its behaviour with the minimal fuss
> possibly.
> 
> Only way that I can think of being consistent and bug compatible will be
> to store:
> - old block_shared/enanbeld capability value
> - if we set -b/-i on the command line
> 
> In migration cleanup do the right thing depending on this four
> variables.  I think that it is adding lots of complexity for very few
> improvement.
> 
> 
> > Or is the proposal that we are also going to simplify the QMP 'migrate'
> > command to get rid of crufty parameters?
> 
> I didn't read it that way, but I would not oppose O:-)
> 

Ewww this is messy; you end up with almost as much code as the old
flags you're trying to remove.
For HMP you could gently deprecate the flags over time and give
a warning telling people to use the capabilities; but doing that
in one go is a bit nasty, and you still have to do something
cleverer for the QMP which is similar.
 
I think I agree though that migrate, cancel, migrate should work
sensibly and it's hard to get it right.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK