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?
PS: I think I spotted a number of additional minor issues in your
series, but I won't describe them here, as I hope the simplification
will make most of them go away.
