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

[John Snow]

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.

- 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.

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?

--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
>>
>