Re: [Qemu-devel] [PATCH v2 6/8] migration: Add multifd-compress parameter

[Markus Armbruster <armbru@redhat.com>]

Juan Quintela <quintela@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> wrote:
>> Juan Quintela <quintela@redhat.com> writes:
>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>
>>> ---
>>> Rename it to NONE
>
>>> @@ -1822,6 +1826,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>>          p->has_multifd_channels = true;
>>>          visit_type_int(v, param, &p->multifd_channels, &err);
>>>          break;
>>> +    case MIGRATION_PARAMETER_MULTIFD_COMPRESS:
>>> +        p->has_multifd_compress = true;
>>> +        visit_type_enum(v, param, &compress_type,
>>> +                        &MultifdCompress_lookup, &err);
>>
>> visit_type_MultifdCompress(), please.
>
> done.
>
> Interesting that I can
>
> #include "qapi/qapi-visit-common.h"
>
> but not what I would expect/want:
>
> #include "qapi/qapi-visit.h"

I guess you tried to include it into target-independent code, which
doesn't work since we added target conditionals like

    { 'command': 'rtc-reset-reinjection',
      'if': 'defined(TARGET_I386)' }

Adding these (merge commit a0430dd8abb) was only possible because the
code generated from the QAPI schema mirrors the QAPI schema's modular
structure: just like qapi-schema.json includes its sub-modules such as
common.json, the generated qapi-visit.h includes the headers generated
for its sub-modules such as qapi-visit-common.h.  See commit 252dc3105f
"qapi: Generate separate .h, .c for each module".

The original motivation for this modularization was actually compile
time.  Before modularization, touching the QAPI schema typically
recompiled everything using QAPI, which nowadays means pretty much
everything.  Modularization let me replace "include all generated QAPI
stuff" by "include just the generated QAPI stuff I actually need", for
*massive* compile time improvements.

Back to target conditionals.  Right now, we confine them to sub-module
target.json.  This means the headers generated for this sub-module, such
as qapi-visit-target.h, can only be included into target-dependent code.
Since qapi-visit.h includes all sub-modules, it can also only be
included there.

> Perhaps we should remove
>
> #include "qapi-visit-target.h"
>
> from there?

No for two reasons:

1. The code generating qapi-visit.h has no idea which sub-modules are
target-specific.

2. You shouldn't include qapi-visit.h anyway, it's bad for compile time.

> Anyways, independent of this patch.

Yes.

>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 5da1439a8b..7c8e71532f 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -645,6 +645,17 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
>>>      .set_default_value = set_default_value_enum,
>>>  };
>>>  
>>> +/* --- MultifdCompress --- */
>>> +
>>> +const PropertyInfo qdev_prop_multifd_compress = {
>>> +    .name = "MultifdCompress",
>>> +    .description = "multifd_compress values",
>>
>> Similar property declarations list the valid values in .description.
>
> Fixed, thanks.
>
>>>  
>>> +#define DEFINE_PROP_MULTIFD_COMPRESS(_n, _s, _f, _d) \
>>> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compress, MultifdCompress)
>>> +
>>
>> As long as qdev_prop_multifd_compress is exposed in qdev-properties.h,
>> hiding the macro using it doesn't make sense to me.
>
> Where do you want it to be?  This should only be used here, but if you
> want it anywhere else, told me where.

Put the declaration of qdev_prop_multifd_compress and the macro in the
same source file.  Simplest: qdev-properties.h.

>>> +##
>>> +# @MultifdCompress:
>>> +#
>>> +# An enumeration of multifd compression.
>>> +#
>>> +# @none: no compression.
>>> +#
>>> +# Since: 4.1
>>> +#
>>> +##
>>> +{ 'enum': 'MultifdCompress',
>>> +  'data': [ 'none' ] }
>>
>> Any particular reason for putting this in common.json?  As is, it looks
>> rather migration-specific to me...
>
> Not sure if with new "qapi" compiler it works, it used to be that it
> failed if you declared an enum anywhere else.  See how I have to put
> property info into qdev-properties.c instead of any migration file.

You can certainly declare enums anywhere, you just have to make sure to
include the right headers in the right places, just like for manually
written enums in C headers.

Rules for modular QAPI schema:

1. If module A uses an entity defined in module B, A must include B

   Example: since migration.json uses MultifdCompress defined in
   common.json, migration.json must include common.json (it does).

2. No circular includes, ever.

3. Try to avoid cross-module dependencies

   Example: migration.json uses MultifdCompress defined in common.json.
   This is a cross-module dependency.  Can we avoid it by moving
   MultifdCompress into migration.json?

   The QAPI schema would be just fine with such a move, but C code using
   the generated headers might be unhappy, because it now has to include
   qapi-types-migration.h instead of just qapi-types-common.h.

   My question was: is C code really unhappy?  Please find out and tell
   me.