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

[Juan Quintela <quintela@redhat.com>]

Eric Blake <eblake@redhat.com> 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:-)

Later, Juan.