Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 08:08:42PM +0200, Markus Armbruster wrote:
> Sorry for the delay; too many distractions, and I needed a good think.
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >> 
> >> >> > The 'command-features' pragma allows for defining additional
> >> >> > special features that are unique to a particular QAPI schema
> >> >> > instance and its implementation.
> >> >> 
> >> >> So far, we have special features (predefined, known to the generator and
> >> >> treated specially), and normal features (user-defined, not known to the
> >> >> generator).  You create a new kind in between: user-defined, not known
> >> >> to the generator, yet treated specially (I guess?).  Hmm.
> >> >> 
> >> >> Could you at least hint at indented use here?  What special treatment do
> >> >> you have in mind?
> >> >
> >> > Essentially, these features are a way to attach metadata to commands that
> >> > the server side impl can later query. This eliminates the need to 
> >> > hardcode
> >> > lists of commands, such as in QGA which hardcodes a list of commands 
> >> > which
> >> > are safe to use when filesystems are frozen. This is illustrated later in
> >> > this series.
> >> 
> >> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> >> and maybe section "Features".
> 
> Second thoughts; see below.
> 
> >> I'm not sure conflating the new kind of feature with existing special
> >> features is a good idea.  I need to review more of the series before I
> >> can make up my mind.
> >
> > I originally implemented a completely separate 'tags' concept in the
> > QAPI parser, before deciding I was just re-inventing 'features' for
> > no obvious benefit.
> >
> > The other nice thing about using features is that these are exposed
> > in the schema and docs. With the 'fsfreeze' restriction in code,
> > there's no formal docs of what commands are allowed when frozen, and
> > this is also not exposed in QAPI schema to apps. Using 'features'
> > we get all that as standard.
> 
> When you need to tack a mark to one or more things for whatever purpose
> *and* expose it to QMP clients, then features make sense.  This is the
> case here.
> 
> Initially, features were strictly an external interface annotation, and
> were not meant to be used within QEMU.  All features were user-defined.
> 
> This changed when I created configurable policy for deprecated and
> unstable management interfaces: the policy engine needs to check for
> features 'deprecated' and 'unstable'.  Since the policy engine is partly
> implemented in generated code, these two features need to be baked into
> the generator.  This makes them special.
> 
> You need less than that: a predicate "does <command> have <feature>" for
> certain features, ideally without baking them into the generator.
> 
> The command registry already tracks each command's special features for
> use by the policy engine.  Obvious idea: also track the features you
> want to pass to the predicate.
> 
> Your series adds tracking for exactly the features you need:
> 
> * Enumerate them in the schema with new pragma command-features
> 
>   Missing: documentation for the pragma.
> 
> * Generate an extension QapiSpecialFeatureCustom of existing enum
>   QapiSpecialFeature, which is not generated.  The latter is in
>   qapi/util.h, the former in ${prefix}qapi-init-commands.h.
> 
> * Mark these features special for commands only, so existing registry
>   machinery tracks them.  Do *not* make them special elsewhere, because
>   that would break things.
> 
>   Feels like a hack.  Minor trap: if you use the same feature in
>   multiple schemas, multiple generated headers will define the same enum
>   constant, possibly with different values.  If you manage to include
>   the wrong header *and* the value differs there, you'll likely lose
>   hair.
> 
> * Missing: tests.
> 
> I think we can avoid supplying most of the missing bits.  The main QAPI
> schema uses five features: deprecated, unstable,
> allow-write-only-overlay, dynamic-auto-read-only, fdset.  The QGA QAPI
> schema uses four, namely the four you add in this series.  Why not track
> all features, and dispense with the pragma?  Like this:
> 
> * Change type of feature bitsets to uint64_t (it's unsigned now).
> 
> * Error out if a schema has more than 64 features.
> 
> * Move enum QapiSpecialFeature into a new generated header.
> 
> * Generate a member for each feature, not just the two predefined ones.
> 
> * Pass all command features to the registry, not just the special ones.
> 
> * Recommended: do the same elsewhere, i.e. replace
>   gen_special_features() by gen_features().
> 
> Thoughts?

So basically the code would always have access to all features, and
we would have no notion of "special" features any more.

I'm happy to give that a try.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|