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

[Daniel P . Berrangé]

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 ()?

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

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.

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.

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.

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