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

[Markus Armbruster]

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.

> none of these approaches are especially future proof if we ever
> need fine grained control over sending a sub-set of the non-RAM
> vmstate. Not sure if that matters in the end.
>
>
> Regards,
> Daniel