Re: [Qemu-devel] [PATCH 1/4] add QemuSupportState

[John Snow]

On 10/31/2018 02:58 PM, Eduardo Habkost wrote:
> On Wed, Oct 31, 2018 at 02:37:36PM -0400, John Snow wrote:
>>
>>
>> On 10/31/2018 02:06 PM, Eduardo Habkost wrote:
>>> On Wed, Oct 31, 2018 at 12:04:16PM -0400, John Snow wrote:
>>>>
>>>>
>>>> On 10/30/2018 09:32 AM, Philippe Mathieu-Daudé wrote:
>>>>> Hi Gerd,
>>>>>
>>>>> On 30/10/18 12:13, Gerd Hoffmann wrote:
>>>>>> Indicates support state for somerhing (device, backend, subsystem, ...)
>>>>>
>>>>> "something"
>>>>>
>>>>>> in qemu.  Modeled roughly after the "S:" states we have in MAINTANERS.
>>>>>>
>>>>>> Signed-off-by: Gerd Hoffmann <address@hidden>
>>>>>> ---
>>>>>>   include/qemu/support-state.h | 17 +++++++++++++++++
>>>>>>   util/support-state.c         | 23 +++++++++++++++++++++++
>>>>>>   qapi/common.json             | 16 ++++++++++++++++
>>>>>>   util/Makefile.objs           |  1 +
>>>>>>   4 files changed, 57 insertions(+)
>>>>>>   create mode 100644 include/qemu/support-state.h
>>>>>>   create mode 100644 util/support-state.c
>>>>>>
>>>>>> diff --git a/include/qemu/support-state.h b/include/qemu/support-state.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..5fd3c83eee
>>>>>> --- /dev/null
>>>>>> +++ b/include/qemu/support-state.h
>>>>>> @@ -0,0 +1,17 @@
>>>>>> +#ifndef QEMU_SUPPORT_STATE_H
>>>>>> +#define QEMU_SUPPORT_STATE_H
>>>>>> +
>>>>>> +#include "qapi/qapi-types-common.h"
>>>>>> +
>>>>>> +typedef struct QemuSupportState {
>>>>>> +    SupportState state;
>>>>>> +    const char *reason;
>>>>>> +} QemuSupportState;
>>>>>> +
>>>>>> +void qemu_warn_support_state(const char *type, const char *name,
>>>>>> +                             QemuSupportState *state);
>>>>>> +
>>>>>> +bool qemu_is_deprecated(QemuSupportState *state);
>>>>>> +bool qemu_is_obsolete(QemuSupportState *state);
>>>>>> +
>>>>>> +#endif /* QEMU_SUPPORT_STATE_H */
>>>>>> diff --git a/util/support-state.c b/util/support-state.c
>>>>>> new file mode 100644
>>>>>> index 0000000000..7966fa0fc7
>>>>>> --- /dev/null
>>>>>> +++ b/util/support-state.c
>>>>>> @@ -0,0 +1,23 @@
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/error-report.h"
>>>>>> +#include "qemu/support-state.h"
>>>>>> +
>>>>>> +void qemu_warn_support_state(const char *type, const char *name,
>>>>>> +                             QemuSupportState *state)
>>>>>> +{
>>>>>> +    warn_report("%s %s is %s%s%s%s", type, name,
>>>>>> +                SupportState_str(state->state),
>>>>>> +                state->reason ? " ("          : "",
>>>>>> +                state->reason ? state->reason : "",
>>>>>> +                state->reason ? ")"           : "");
>>>>>> +}
>>>>>> +
>>>>>> +bool qemu_is_deprecated(QemuSupportState *state)
>>>>>> +{
>>>>>> +    return state->state == SUPPORT_STATE_DEPRECATED;
>>>>>> +}
>>>>>> +
>>>>>> +bool qemu_is_obsolete(QemuSupportState *state)
>>>>>> +{
>>>>>> +    return state->state == SUPPORT_STATE_OBSOLETE;
>>>>>> +}
>>>>>> diff --git a/qapi/common.json b/qapi/common.json
>>>>>> index 021174f04e..78176151af 100644
>>>>>> --- a/qapi/common.json
>>>>>> +++ b/qapi/common.json
>>>>>> @@ -151,3 +151,19 @@
>>>>>>                'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
>>>>>>                'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
>>>>>>                'x86_64', 'xtensa', 'xtensaeb' ] }
>>>>>> +
>>>>>> +##
>>>>>> +# @SupportState:
>>>>>> +#
>>>>>> +# Indicate Support level of qemu devices, backends, subsystems, ...
>>>>>> +#
>>>>>> +# Since: 3.2
>>>>>> +##
>>>>>> +{ 'enum': 'SupportState',
>>>>>> +  'data': [ 'unknown',
>>>>>
>>>>> 'unknown' is scary and should be fixed.
>>>>>
>>>>>> +            'supported',
>>>>>> +            'maintained',
>>>>>> +            'odd-fixes',
>>>>>
>>>>> All those fit in 'supported'
>>>>>
>>>>>> +            'orphan',
>>>>>> +            'obsolete',
>>>>>> +            'deprecated' ] }
>>>>>
>>>>> And all those should appear as 'deprecated' IMHO.
>>>>>
>>>>
>>>> You've covered it a bit below, but I think we need more than "supported"
>>>> and "deprecated" -- at *least* a third state "unsupported" is required, 
>>>> IMO:
>>>>
>>>> - Supported: We expect this to work. We test this component. We will
>>>> ship CVE fixes in a timely manner for this component. You are, as a
>>>> user, using something that is cared for and you may rely on us to care
>>>> for it.
>>>>
>>>> - Unsupported: We expect this to work, but nobody is tending to it
>>>> actively. It might be perfectly OK, but the tests and support may be
>>>> lacking. Try not to rely on this in an enterprise environment unless you
>>>> have resources to devote to caring for it personally. I'd count things
>>>> like Raspberry Pi boards in this category: they work, usually, but not
>>>> fully. Some people are working on them, but they're not ready for prime
>>>> time usage.
>>>
>>> I wonder: do we need a distinction between code that's
>>> unsupported and expected to be removed in the next versions of
>>> QEMU, and code that's unsupported but not planned to be removed
>>> yet?
>>>
>>
>> I maybe should not have used the word deprecated, which I think
>> obfuscates the code quality metric we might be trying to convey: even
>> top-notch, first-tier support code can be deprecated in favor of some
>> newer, better model.
>>
>> So let's not use it for these purposes: as you suggest, even deprecated
>> code should be compiled because it hasn't been removed *yet*, and it
>> serves explicitly as a transitional stage.
>>
>>
>> I just really want to paint a difference between... say,
>>
>> "stuff that maybe works, but isn't tier 1" and
>> "stuff that probably doesn't work without some elbow grease."
>>
>> I'm just having a hard time coming up with a hard metric to
>> differentiate the two, but I can't shake the feeling that "SUPPORTED" vs
>> "UNSUPPORTED" is just simply too strict of a binary.
>>
>> In my mental model, it's three tiers:
>>
>> A: First-class support.
>> B: Proceed with caution.
>> C: Experimental/Development ONLY.
>>
>> As a stoplight, it'd be:
>>
>> GREEN: Expected to work. Well cared for and tested.
>>
>> YELLOW: Some problems, but expected either to transition to green
>> (maintainer willing), OR be deprecated/removed. At a code quality level
>> where non-developers should try using it and report bugs. These devices
>> might work well for certain use cases but aren't fully implemented yet
>> and could break.
>>
>> RED: Broken. Kept in the code base for development reasons; e.g. new
>> ARM/SoC models that are in development, but are known to not work yet.
>> People who are not developers should not waste their time trying to use
>> them to accomplish real work yet.
>>
>>
>> That's kind of the granularity I have in mind, but I don't know if it's
>> practical to grade everything on so fuzzy a scale to begin with. I'd
>> suggest as a first pass marking everything either as YELLOW or RED to
>> suggest "SUPPORTED" vs "UNSUPPORTED", and then individually making the
>> case for specific components to be promoted to GREEN.
>>
>> --js
>>
>>>>
>>>> - Deprecated: This used to work, or used to be maintained, but has been
>>>> superseded or is otherwise scheduled to be removed -- the expectation is
>>>> that this device will get worse, not better. The device model may be
>>>> known to be incorrect, there may be major bugs, and it receives little
>>>> to no care or maintenance. Actively avoid using in an enterprise context.
>>>>
>>>> The idea being that there is definitely a difference between obviously
>>>> and wholly broken components that we're working on replacing or getting
>>>> rid of, and components that are "in beta" or partially functional, and
>>>> those that are full, "tier 1" supported subsystems.
>>>
>>> I agree there's a difference between unsupported and supported
>>> code.
>>>
>>> But I'd say that making this a build time option is a must (as
>>> many distributions would wish to disable unsupported code at
>>> build time).  Making the information available at runtime is just
>>> nice to have.
>>>
>>
>> Oh, yes, it must absolutely be build-time. In my three-tier example, I
>> would expect downstream distributions like Fedora to compile out the
>> RED/C-TIER immediately. For something like RHEL, they might compile out
>> the YELLOW/B-TIER as well.
>>
>> Some use cases might want to compile out RED but leave YELLOW as some
>> run-time warning or run-time error ("please use --use-shoddy-parts if
>> you wish to use %%ramshackle_device%% ...")
>>
>> I think what this buys us is confidence that despite QEMU's very wide
>> scope, we can quickly see both in code and at run-time that we're not
>> using something we shouldn't be, or misunderstanding the relative
>> quality of the parts we're building our virtual machine from.
>>
>> Having conservative green/yellow lists might help reduce the "search
>> space" for documentation and help more users arrive at "good"
>> configurations by default.
> 
> Right, I think we are on the same page here.
> 
> Now, support/stability status and deprecation/removal status seem
> to be independent variables.  It's perfectly valid to say "this
> device is expected to be removed in the future, you need to
> update your configuration" and "this device is expected to be
> stable and we support it" at the same time.  Let's not mix up
> both.
> 

Agree

> This also means keeping in mind the different stories we want to
> address.  The ones I would like to address are:
> 
> 1) As a virtualization app user, I want to be warned at runtime
>    in case my VM configuration relies on features that will be
>    removed in future versions of the stack, so I have a chance to
>    update my VM configuration before updating the software.
> 

Yes, or as a user, get warned if my configuration is less than optimal.
Use of a "yellow" or "red" component that I thought was perfectly safe.
Many people don't realize they're using legacy components until we
actually nominate them for deprecation. They could have found out much
sooner.

Part of Nemu's value now is knowing that whatever is left has a much
higher chance of being something you want, because there's less ways to
do things.

> 2) As a QEMU distributor, downstream vendor, or developer, I want
>    to:
>    * know which parts of the code are really supported and
>      expected to work without bugs;
>    * disable unsupported/unstable features at build time;
>    * disable test cases for known-broken parts of QEMU;
>    * know if an user is relying on an unsupported or known-broken
>      feature of QEMU.
> 

Tying components to test cases is compelling.

> Are there any additional use cases I forgot, here?
> 

Those are the big ones.

> 
>>
>>> Deprecated code, on the other hand, is expected to be enabled at
>>> build time even on enterprise distributions, and will definitely
>>> require a mechanism to generate a warning at runtime.
>>>
>>>
>>>>
>>>> Adding any more nuanced states than this, though, risks making it too
>>>> difficult to make any informed decisions as a user, I think. This patch
>>>> as-is maybe adds too many?
>>>
>>> Agreed.
>>>
>>>>
>>>> --js
>>>>
>>>>>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>>>>>> index 0820923c18..6e5f8faf82 100644
>>>>>> --- a/util/Makefile.objs
>>>>>> +++ b/util/Makefile.objs
>>>>>> @@ -50,5 +50,6 @@ util-obj-y += range.o
>>>>>>   util-obj-y += stats64.o
>>>>>>   util-obj-y += systemd.o
>>>>>>   util-obj-y += iova-tree.o
>>>>>> +util-obj-y += support-state.o
>>>>>>   util-obj-$(CONFIG_LINUX) += vfio-helpers.o
>>>>>>   util-obj-$(CONFIG_OPENGL) += drm.o
>>>>>>
>>>>>
>>>
> 

-- 
—js