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é <berra...@redhat.com> writes:
>
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berra...@redhat.com> writes:
> >>
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berra...@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 :|
