Re: [PATCH 4/6] migration: Add ram-only capability

[Vladimir Sementsov-Ogievskiy]

14.01.2022 18:55, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Fri, Jan 14, 2022 at 12:22:13PM +0100, Markus Armbruster wrote:
> > > Nikita Lapshin <nikita.lapshin@virtuozzo.com> writes:
> > >
> > > > If this capability is enabled migration stream
> > > > will have RAM section only.
> > > >
> > > > Signed-off-by: Nikita Lapshin <nikita.lapshin@virtuozzo.com>
> > >
> > > [...]
> > >
> > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > index d53956852c..626fc59d14 100644
> > > > --- a/qapi/migration.json
> > > > +++ b/qapi/migration.json
> > > > @@ -454,6 +454,8 @@
> > > >   #
> > > >   # @no-ram: If enabled, migration stream won't contain any ram in it. (since 
> > > > 7.0)
> > > >   #
> > > > +# @ram-only: If enabled, only RAM sections will be sent. (since 7.0)
> > > > +#
> > >
> > > What happens when I ask for 'no-ram': true, 'ram-only': true?
> >
> > So IIUC
> >
> >    no-ram=false, ram-only=false =>  RAM + vmstate
> >    no-ram=true, ram-only=false => vmstate
> >    no-ram=false, ram-only=true =>  RAM
> >    no-ram=true, ram-only=true => nothing to send ?
> >
> > I find that the fact that one flag is a negative request and
> > the other flag is a positive request to be confusing.
>
> Me too.
>
> > If we must have two flags then could we at least use the same
> > style for both. ie either
> >
> >    @no-ram
> >    @no-vmstate
> >
> > Or
> >
> >    @ram-only
> >    @vmstate-only
>
> I strongly prefer "positive" names for booleans, avoiding double
> negation.
>
> > Since the code enforces these flags are mutually exclusive
> > though, it might point towards being handled by a enum
> >
> >    { 'enum': 'MigrationStreamContent',
> >      'data': ['both', 'ram', 'vmstate'] }
>
> Enumerating the combinations that are actually valid is often nicer than
> a set of flags that look independent, but aren't.
>
> MigrationCapability can only do flags.  For an enum, we'd have to use
> MigrationParameters & friends.  For an example, check out
> @multifd-compression there.

Remember, that we also have block-dirty-bitmaps in migration stream. And we 
also may have block devices data (should it be deprecated?).

So, what about an optional migration parameter stream-content, which would be a 
*list* of MigrationStreamContent enum values?

Than we can deprecate dirty-bitmaps and block capabilities in favor of new 
migration parameter.

{ 'enum': 'MigrationStreamContent', 'data': ['block-dirty-bitmpas', 'block', 
'ram', 'vmstate'] }

...

Migration parameters:

...
'stream-content': ['MigrationStreamContent']


--
Best regards,
Vladimir