Daniel P. Berrangé <berra...@redhat.com> writes:
> On Fri, Nov 12, 2021 at 12:04:33PM +0100, Juan Quintela wrote:
>> Leonardo Bras <leob...@redhat.com> wrote:
[...]
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index abaf6f9e3d..add3dabc56 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -886,6 +886,10 @@ MigrationParameters
>> > *qmp_query_migrate_parameters(Error **errp)
>> > params->multifd_zlib_level = s->parameters.multifd_zlib_level;
>> > params->has_multifd_zstd_level = true;
>> > params->multifd_zstd_level = s->parameters.multifd_zstd_level;
>> > +#ifdef CONFIG_LINUX
>> > + params->has_zerocopy = true;
>> > + params->zerocopy = s->parameters.zerocopy;
>> > +#endif
>> > params->has_xbzrle_cache_size = true;
>> > params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
>> > params->has_max_postcopy_bandwidth = true;
>> > @@ -1538,6 +1542,11 @@ static void
>> > migrate_params_test_apply(MigrateSetParameters *params,
>> > if (params->has_multifd_compression) {
>> > dest->multifd_compression = params->multifd_compression;
>> > }
>> > +#ifdef CONFIG_LINUX
>> > + if (params->has_zerocopy) {
>> > + dest->zerocopy = params->zerocopy;
>> > + }
>> > +#endif
>> > if (params->has_xbzrle_cache_size) {
>> > dest->xbzrle_cache_size = params->xbzrle_cache_size;
>> > }
>> > @@ -1650,6 +1659,11 @@ static void
>> > migrate_params_apply(MigrateSetParameters *params, Error **errp)
>> > if (params->has_multifd_compression) {
>> > s->parameters.multifd_compression = params->multifd_compression;
>> > }
>> > +#ifdef CONFIG_LINUX
>> > + if (params->has_zerocopy) {
>> > + s->parameters.zerocopy = params->zerocopy;
>> > + }
>> > +#endif
>>
>> After seing all this CONFIG_LINUX mess, I am not sure that it is a good
>> idea to add the parameter only for LINUX. It appears that it is better
>> to add it for all OS's and just not allow to set it to true there.
>>
>> But If QAPI/QOM people preffer that way, I am not going to get into the
>> middle.
>
> I don't like all the conditionals either, but QAPI design wants the
> conditionals, as that allows mgmt apps to query whether the feature
> is supported in a build or not.
Specifically, the conditionals keep @zerocopy out of query-qmp-schema
(a.k.a. schema introspection) when it's not actually supported.
This lets management applications recognize zero-copy support.
Without conditionals, the only way to probe for it is trying to switch
it on. This is inconvenient and error-prone.
Immature ideas to avoid conditionals:
1. Make *values* conditional, i.e. unconditional false, but true only if
CONFIG_LINUX. The QAPI schema language lets you do this for
enumerations today, but not for bool.
2. A new kind of conditional that only applies to schema introspection,
so you can eat your introspection cake and keep the #ifdef-less code
cake (and the slight binary bloat that comes with it).
