Re: [PATCH v3 07/10] qapi: implement conditional command arguments

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 28, 2023 at 09:58:01AM -0600, Eric Blake wrote:
>> On Wed, Feb 22, 2023 at 11:23:03AM +0100, Markus Armbruster wrote:
>> > > However, I think it would be simpler, and better, if we piped the
>> > > generated code to clang-format (when available). I made a simple patch
>> > > for that too.
>> > 
>> > Piping through indent or clang-format may well give us neater results
>> > for less effort.
>> > 
>> > We might want to dumb down generator code then.
>> 
>> Indeed, this approach seems like it might be worth pursuing (our
>> generator doesn't have to worry about spacing, because we do that in a
>> second pass with something that will still produce human-legible final
>> results).
>> 
>> > >> > So I would rather assert that we don't introduce such a schema, until 
>> > >> > we
>> > >> > fix the code generator. Or we acknowledge the limitation, and treat 
>> > >> > it as a
>> > >> > schema error. Other ideas?
>> > >>
>> > >> Yes: throw an error.  Assertions are for programming errors.  This isn't
>> > >> a programming error, it's a limitation of the current implementation.
>> > >>
>> > >> How hard would it be to lift the limitation?
>> > >
>> > > Taking this as a problematic example:
>> > >
>> > > void function(first,
>> > > #ifdef A
>> > >     a,
>> > > #endif
>> > > #ifdef B
>> > >     b
>> > > #endif
>> > > )
>> 
>> I am NOT a fan of preprocessor conditionals mid-function-signature.
>> It gets really nasty, really fast.  Is there any way we can have:
>> 
>> struct S {
>> #ifdef A
>>   type a;
>> #endif
>> #ifdef B
>>   type b;
>> #endif
>> };
>> 
>> void function(struct S)
>> 
>> so that the preprocessor conditionals never appear inside ()?

Yes, there is a way: we have conditional object type members, and we
have 'boxed': true for commands.

> I'd question whether we should be doing conditional arguments
> at all.

Note that conditional arguments already exist.  This patch merely makes
them work as advertized even without 'boxed': true.

If we decide they shouldn't exist, we need to make the generator reject
them.  But what *exactly* shouldn't exist?

In my view, a command takes *one* argument (of complex type) and returns
*one* result (of any type, but only because we had to grandfather
non-complex types).

For *struct* arguments, we support a special way to pass arguments:
unboxed.  Actually used for most commands.

Support for conditional members of complex types implies support for
conditional members of (complex) argument types.

We could restrict them to boxed.  Perhaps we should.

> IMHO having an API contract that changes based on configuration
> file settings is going to be nothing but trouble. Not only does
> it make the declaration ugly, but all callers become ugly too
> with conditionals. It will lead to bugs where a caller is written
> and tested with one build combination, and find it forgot the
> conditional calling needed for a different build combination.

Two separate points: ugly and test matrix.

"Ugly" is a valid point.  Ugly in two generated places (generated
declaration and handwritten definition) is kind of bad, ugly in all
places (plus all callers) is worse.

"Test matrix" applies to conditionals anywhere, not just command
arguments.  You could perhaps argue certain conditionals are more
problematic than others.

> Any fields that we conditionally disable must already be marked
> as optional in the schema, to indicate to mgmt apps that they
> may or may not be present depend on what QEMU build the app is
> talking to.

Not quite.

Neither the code nor docs/devel/qapi-code-gen.txt require conditional
members of complex types to be optional.  qapi-schema-test.json covers
conditional mandatory members:

    { 'struct': 'TestIfStruct',
      'data': { 'foo': 'int',
--->            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_BAR'} },
      'if': 'TEST_IF_STRUCT' }

"Optional" isn't for indicating "that a member may or may not be present
depend on what QEMU build" via introspection, it's for indicating the
member exists in *this* QEMU build, but it need not be present on the
wire, no more, no less.

We could perhaps derive "conditional member must be optional" from our
compatibility promise, but my cold-addled brain is not up to that task
right now.

> So if they're optional, what is wrong with generating the arguments
> unconditionally and just leaving them unused/unset in builds that
> don't require them ?  I think it'd be fine if the qmp_getfd API
> decl in QEMU had an 'const char *wsainfo' field even on Linux
> builds. The Linux impl can simply ignore it, or raise an error if
> it is set.

Raise an error.  Silently ignoring is no good.

Taking a step back...  both proposed solutions are kind of ugly.  Let me
recap:

1. QMP command getfd takes an additional argument when QEMU runs on a
   Windows host.

   a. The additional argument is mandatory.

   b. The additional argument in optional, but only nominally; if
      absent, the command fails.

2. QMP comamnd getfd takes an optional argument.  If it's present and
   QEMU runs on a POSIX host, the command fails.  Likewise when it's
   absent and QEMU runs on a Windows host.

What about 3. have an additional command conditional on CONFIG_WIN32?
Existing getfd stays the same: always fails when QEMU runs on a Windows
host.  The new command exists only when QEMU runs on a Windows host.