* Juan Quintela (quint...@redhat.com) wrote:
> Eric Blake <ebl...@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:-)
>
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 / dgilb...@redhat.com / Manchester, UK
