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

[Eduardo Habkost]

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?

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

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

-- 
Eduardo