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

[Juan Quintela <quintela@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>> > * Juan Quintela (quintela@redhat.com) wrote:
>> >> Markus Armbruster <armbru@redhat.com> wrote:
>> >> > Juan Quintela <quintela@redhat.com> writes:
>> >> >
>> >> >> Eric Blake <eblake@redhat.com> wrote:
>> >> 
>> >> 
>> >> >>> 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.
>> >> >
>> >> > I'm not too familiar with this stuff, so please correct my
>> >> > misunderstandings.
>> >> >
>> >> > "Normal" migration configuration is global state, i.e. it applies to all
>> >> > future migrations.
>> >> >
>> >> > Except the "migrate" command's flags apply to just the migration kicked
>> >> > off by that command.
>> >> >
>> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i).
>> >> > !blk && inc makes no sense and is silently treated like !blk && !inc.
>> >> >
>> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP.
>> >> 
>> >> As qmp command is asynchronous, you can think that -d is *always* on in
>> >> QMP O:-)
>> >> 
>> >> > You'd like to deprecate these flags in favour of "normal" configuration.
>> >> > However, we need to maintain QMP backward compatibility at least for a
>> >> > while.  HMP backward compatibility is nice to have, but not required.
>> >> >
>> >> > First step is to design the new interface you want.  Second step is to
>> >> > figure out backward compatibility.
>> >> >
>> >> > The new interface adds a block migration tri-state (off,
>> >> > non-incremental, incremental) to global state, default off.  Whether
>> >> > it's done as two bools or an enum of three values doesn't matter here.
>> >> 
>> >> Tristates will complicate it.  I still think that:
>> >> 
>> >> - capability: block_migration
>> >> - parameter: block_shared
>> >> 
>> >> Makes more sense, no?
>> >
>> > I don't understand what making block_shared a parameter gives you as
>> > opposed to simply having two capabilities.
>> >
>> > (And how did we get 'shared'? We started off with block & incremental)
>> 
>> The variables on MigrationParams:
>> 
>> struct MigrationParams {
>>     bool blk;
>>     bool shared;
>> };
>> 
>> 
>> I can move to incremental.  I am not sure which one is clearer.
>> 
>> The advantage of having shared as a parameter is that we forget about
>> all this dependency bussiness.  Is the same than compression_threads
>> paramter, you setup to whichever value that you want.  But you don't get
>> compression_threads until you set the compress capability.
>> 
>> So, in this case we will have:
>> 
>> block capability: Are we using block migration or not
>> block-incremental parameter: If we are using block migration, are we
>>       using incremental copying of the block layer?
>
> If it's still a boolean why does having it as a parameter remove the
> dependency?

Forget -b/-i.

migration_set_parameter compression_threads 8

migrate <foo>

We don't use compression_threads at all

migrate_set_capability compress

migrate <foo>

Now, we use compression threads.

So, compression_threads parameter is a parameter that is only used when
compress capability is enabled.

Same for block migration.  Block_incremental parameter is used only when
block migration capability is setup.  No dependency check needed at all.

Or I am losing something obvious here?

Later, Juan.