Re: [PATCH v4 27/34] qemu-options: New -compat to set policy for deprecated interfaces

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 3/17/20 6:54 AM, Markus Armbruster wrote:
>> Policy is separate for input and output.
>>
>> Input policy can be "accept" (accept silently), or "reject" (reject
>> the request with an error).
>>
>> Output policy can be "accept" (pass on unchanged), or "hide" (filter
>> out the deprecated parts).
>>
>> Default is "accept".  Policies other than "accept" are implemented
>> later in this series.
>>
>> For now, -compat covers only syntactic aspects of QMP, i.e. stuff
>> tagged with feature 'deprecated'.  We may want to extend it to cover
>> semantic aspects, CLI, and experimental features.
>>
>> The option is experimental.
>
> On IRC, we decided that it's probably not worth shoe-horning this (and
> the rest of the series) into 5.0, given the experimental nature.

Yes.  I can't wait for having test suites try this, but releases and
impatience are not a good mix.

> Still, I'll go ahead and review, so we can settle on things early in
> 5.1.

Appreciated!

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>> +++ b/qapi/compat.json
>> @@ -0,0 +1,51 @@
>> +# -*- Mode: Python -*-
>> +
>> +##
>> +# = Compatibility policy
>> +##
>> +
>> +##
>> +# @CompatPolicyInput:
>> +#
>> +# Policy for handling "funny" input.
>> +#
>> +# @accept: Accept silently
>> +# @reject: Reject with an error
>> +#
>> +# Since: 5.0
>
> Of course, now that we're slipping this, you'll have to s/5.0/5.1/g
> over the remaining patches.  I won't point it out further.

Acknowledged.

>> +##
>> +# @CompatPolicy:
>> +#
>> +# Policy for handling deprecated management interfaces.
>> +#
>> +# This is intended for testing users of the management interfaces.
>> +#
>> +# Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
>> +# with feature 'deprecated'.  We may want to extend it to cover
>> +# semantic aspects, CLI, and experimental features.
>
> Hiding/rejecting x- interfaces is probably the easiest of these, but I
> agree that leaving this open-ended to add further coverage (or even
> additional modes) is still reasonable.

Apropos experimental: we may choose to move from the x- naming
convention to a special feature flag "experimental".  That way, we don't
have to rename when we declare the thing stable.

>> +#
>> +# @deprecated-input: how to handle deprecated input (default 'accept')
>> +# @deprecated-output: how to handle deprecated output (default 'accept')
>> +#
>> +# Since: 5.0
>> +##
>> +{ 'struct': 'CompatPolicy',
>> +  'data': { '*deprecated-input': 'CompatPolicyInput',
>> +            '*deprecated-output': 'CompatPolicyOutput' } }
>
> For example, adding
> '*experimental-input': 'CompatPolicyInput'
> would make it easy to hard-code failure on attempt to use x-* commands.

Exactly.

>> +++ b/include/qapi/compat-policy.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * Policy for handling "funny" management interfaces
>> + *
>> + * Copyright (C) 2019 Red Hat, Inc.
>
> You've had this in-tree for a while. I'll leave it up to you if you
> want to add 2020.

I want to.  Hope I remember when I respin.

>> + *
>> + * Authors:
>> + *  Markus Armbruster <address@hidden>,
>> + *
>
> Ending with a comma is odd.  Is the Authors: snippet even necessary,
> or are we better off relying on git history (which tends to be more
> accurate anyway)?

The comma is an accident.

The Authors section is custom, not necessity.

>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>
> Are we trying to use SPDX tags in more files?

No idea :)

>> +++ b/qemu-options.hx
>> @@ -3357,6 +3357,26 @@ DEFHEADING()
>>     DEFHEADING(Debug/Expert options:)
>>   +DEF("compat", HAS_ARG, QEMU_OPTION_compat,
>> +    "-compat 
>> [deprecated-input=accept|reject][,deprecated-output=accept|hide]\n"
>> +    "                Policy for handling deprecated management 
>> interfaces\n",
>> +    QEMU_ARCH_ALL)
>> +SRST
>> +``-compat 
>> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
>> +    Set policy for handling deprecated management interfaces (experimental):
>
> We'll eventually want to drop (experimental), especially if we get all
> the rest of this into 5.1.
>
> But for now this looks like a good start.

Thanks!