v3:
- Collected R-bs
- Patch 2: some reindents, use ARRAY_SIZE (Thomas)

v2:
- Collected R-bs
- Patch 3: convert to use StrOrNull rather than str for the tls_fields
  (it contains a lot of changes, I'll skip listing details, but please
   refer to the commit message)

Patch 1 fixes the tls-authz crashing when someone specifies "null"
parameter for tls-authz.

Patch 2 added a test case for all three tls-auth parameters specifying
"null" to make sure nothing will crash ever with 'null' passed into it.

Patch 3-4 are the proposed patches to deduplicate the three migration
parameter objects in qapi/migration.json.  Note that in this version (patch
3) we used 'str' to replace 'StrOrNull' for tls-* parameters to make then
deduplicate-able.

Please review, thanks.

Peter Xu (4):
  migration/qmp: Fix crash on setting tls-authz with null
  tests/migration-test: Add a test for null parameter setups
  migration/qapi: Replace @MigrateSetParameters with
    @MigrationParameters
  migration/qapi: Drop @MigrationParameter enum

 qapi/migration.json            | 370 +--------------------------------
 include/hw/qdev-properties.h   |   3 +
 migration/options.h            |  50 +++++
 hw/core/qdev-properties.c      |  40 ++++
 migration/migration-hmp-cmds.c |  23 +-
 migration/options.c            | 266 ++++++++++--------------
 migration/tls.c                |   3 +-
 tests/qtest/migration-test.c   |  22 ++
 8 files changed, 247 insertions(+), 530 deletions(-)

-- 
2.41.0

Let me try to summarize our findings so far.

PATCH 1 has been merged.  PATCH 2 has been queued, but not merged (not
sure why, accident?).

The remaining two are the actual de-triplication:

PATCH 3: Fuse MigrationParameters and MigrateSetParameters

PATCH 4: De-document MigrationParameter

The latter is a much simpler problem, so let's discuss it first.


Enum MigrationParameter is used only internally.  It's in the QAPI
schema just because we want code generated for it.  It shouldn't be
documented in the QEMU QMP Reference Manual, but is, because the
generator is too stupid to elide internal-only stuff.

PATCH 4 moves it out of the schema.  It has to add back the lost
generated code in hand-written form, which is a bit unfortunate.  I
proposed to instead drop most of the useless doc comment by exploiting a
QAPI generator loophole.

Aside: the QAPI generator should elide internal-only stuff from the QEMU
QMP Reference manual, and it should not require doc comments then.
Future work, let's not worry about it now.


The fusing of MigrationParameters and MigrateSetParameters is kind of
stuck.  Several options, all with drawbacks or problems:

1. Pick StrOrNull for the tls_FOO members

   This is what PATCH 3 does.  Blocked on the pre-existing class of
   crash bugs discussed in

    Subject: QAPI string visitors crashes
    Message-ID: <875y3epv3y.fsf@pond.sub.org>
    https://lore.kernel.org/qemu-devel/875y3epv3y.fsf@pond.sub.org/

   Needs fixing, but when a fix will be available is unclear.

2. Pick str for the tls_FOO members

   This is what v1 did.  Incompatible change: JSON null no longer works.
   Libvirt doesn't use it (it uses deprecated "" instead), but we cannot
   know for sure nothing else out there uses it.

   I don't think reducing development friction (that's what
   de-duplication accomplishes) justifies breaking our compatibility
   promise.

   To keep the promise, we'd have to deprecate null, un-deprecate "",
   let the grace period pass, and only then de-duplicate.

3. Do nothing, live with the duplication

   Giving up like this would be sad.  Unless we commit to a more
   complete overhaul of migration's QAPI/QMP configuration interface,
   but I doubt we're ready for that.

Thoughts?

On Mon, Oct 16, 2023 at 09:08:40AM +0200, Markus Armbruster wrote:
> Let me try to summarize our findings so far.

Thanks.  I'll reply here instead of all the rest places.

> 
> PATCH 1 has been merged.  PATCH 2 has been queued, but not merged (not
> sure why, accident?).

(I don't know, either; could be in the next pull)

> 
> The remaining two are the actual de-triplication:
> 
> PATCH 3: Fuse MigrationParameters and MigrateSetParameters
> 
> PATCH 4: De-document MigrationParameter
> 
> The latter is a much simpler problem, so let's discuss it first.
> 
> 
> Enum MigrationParameter is used only internally.  It's in the QAPI
> schema just because we want code generated for it.  It shouldn't be
> documented in the QEMU QMP Reference Manual, but is, because the
> generator is too stupid to elide internal-only stuff.
> 
> PATCH 4 moves it out of the schema.  It has to add back the lost
> generated code in hand-written form, which is a bit unfortunate.  I
> proposed to instead drop most of the useless doc comment by exploiting a
> QAPI generator loophole.
> 
> Aside: the QAPI generator should elide internal-only stuff from the QEMU
> QMP Reference manual, and it should not require doc comments then.
> Future work, let's not worry about it now.

Just to double check: @MigrationParameter will not be exported in any form
even today, including query-qmp-schema, am I right?

> 
> 
> The fusing of MigrationParameters and MigrateSetParameters is kind of
> stuck.  Several options, all with drawbacks or problems:
> 
> 1. Pick StrOrNull for the tls_FOO members
> 
>    This is what PATCH 3 does.  Blocked on the pre-existing class of
>    crash bugs discussed in
> 
>     Subject: QAPI string visitors crashes
>     Message-ID: <875y3epv3y.fsf@pond.sub.org>
>     https://lore.kernel.org/qemu-devel/875y3epv3y.fsf@pond.sub.org/
> 
>    Needs fixing, but when a fix will be available is unclear.
> 
> 2. Pick str for the tls_FOO members
> 
>    This is what v1 did.  Incompatible change: JSON null no longer works.
>    Libvirt doesn't use it (it uses deprecated "" instead), but we cannot
>    know for sure nothing else out there uses it.
> 
>    I don't think reducing development friction (that's what
>    de-duplication accomplishes) justifies breaking our compatibility
>    promise.
> 
>    To keep the promise, we'd have to deprecate null, un-deprecate "",
>    let the grace period pass, and only then de-duplicate.

Is "" deprecated already anywhere?

> 
> 3. Do nothing, live with the duplication
> 
>    Giving up like this would be sad.  Unless we commit to a more
>    complete overhaul of migration's QAPI/QMP configuration interface,
>    but I doubt we're ready for that.
> 
> Thoughts?

I already went 3) on the patch I posted for avail-switchover-bw.  I don't
know what's the best for 1) and 2), but if we can at least reduce
duplication from 3->2 that's a progress.  I replied in the other thread for
that loophole experiment.

How hard it is to mark an object not requiring documentation on each of its
field, if that's what we want in this case?  Currently the loophole didn't
work for me for some reason.  If we can have a marker for objects to escape
doc check legally, we can apply that to both @MigrationParameter and one
other (perhaps @MigrationSetParameters).

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Oct 16, 2023 at 09:08:40AM +0200, Markus Armbruster wrote:
>> Let me try to summarize our findings so far.
>
> Thanks.  I'll reply here instead of all the rest places.
>
>> 
>> PATCH 1 has been merged.  PATCH 2 has been queued, but not merged (not
>> sure why, accident?).
>
> (I don't know, either; could be in the next pull)
>
>> 
>> The remaining two are the actual de-triplication:
>> 
>> PATCH 3: Fuse MigrationParameters and MigrateSetParameters
>> 
>> PATCH 4: De-document MigrationParameter
>> 
>> The latter is a much simpler problem, so let's discuss it first.
>> 
>> 
>> Enum MigrationParameter is used only internally.  It's in the QAPI
>> schema just because we want code generated for it.  It shouldn't be
>> documented in the QEMU QMP Reference Manual, but is, because the
>> generator is too stupid to elide internal-only stuff.
>> 
>> PATCH 4 moves it out of the schema.  It has to add back the lost
>> generated code in hand-written form, which is a bit unfortunate.  I
>> proposed to instead drop most of the useless doc comment by exploiting a
>> QAPI generator loophole.
>> 
>> Aside: the QAPI generator should elide internal-only stuff from the QEMU
>> QMP Reference manual, and it should not require doc comments then.
>> Future work, let's not worry about it now.
>
> Just to double check: @MigrationParameter will not be exported in any form
> even today, including query-qmp-schema, am I right?

You are right.

Checking whether something is in the output of query-qmp-schema is easy:
look for it in the generated qapi-introspect.c.  Command names appear
like

    QLIT_QDICT(((QLitDictEntry[]) {
        [...]
        { "meta-type", QLIT_QSTR("command"), },
        { "name", QLIT_QSTR("query-migrate"), },
        [...]
    })),

Events names appear like

    QLIT_QDICT(((QLitDictEntry[]) {
        [...]
        { "meta-type", QLIT_QSTR("event"), },
        { "name", QLIT_QSTR("MIGRATION"), },
        [...]
    })),

Type names appear in comments instead of code, like

    /* "145" = MigrationParameters */

MigrationParameter does not appear.

>> The fusing of MigrationParameters and MigrateSetParameters is kind of
>> stuck.  Several options, all with drawbacks or problems:
>> 
>> 1. Pick StrOrNull for the tls_FOO members
>> 
>>    This is what PATCH 3 does.  Blocked on the pre-existing class of
>>    crash bugs discussed in
>> 
>>     Subject: QAPI string visitors crashes
>>     Message-ID: <875y3epv3y.fsf@pond.sub.org>
>>     https://lore.kernel.org/qemu-devel/875y3epv3y.fsf@pond.sub.org/
>> 
>>    Needs fixing, but when a fix will be available is unclear.
>> 
>> 2. Pick str for the tls_FOO members
>> 
>>    This is what v1 did.  Incompatible change: JSON null no longer works.
>>    Libvirt doesn't use it (it uses deprecated "" instead), but we cannot
>>    know for sure nothing else out there uses it.
>> 
>>    I don't think reducing development friction (that's what
>>    de-duplication accomplishes) justifies breaking our compatibility
>>    promise.
>> 
>>    To keep the promise, we'd have to deprecate null, un-deprecate "",
>>    let the grace period pass, and only then de-duplicate.
>
> Is "" deprecated already anywhere?

Deprecation was cleary intended (see commit message of 01fa5598269), but
it looks like it wasn't (and isn't) properly documented.  We were much
less organized about deprecating stuff back then.

>> 3. Do nothing, live with the duplication
>> 
>>    Giving up like this would be sad.  Unless we commit to a more
>>    complete overhaul of migration's QAPI/QMP configuration interface,
>>    but I doubt we're ready for that.
>> 
>> Thoughts?
>
> I already went 3) on the patch I posted for avail-switchover-bw.  I don't
> know what's the best for 1) and 2), but if we can at least reduce
> duplication from 3->2 that's a progress.  I replied in the other thread for
> that loophole experiment.
>
> How hard it is to mark an object not requiring documentation on each of its
> field, if that's what we want in this case?  Currently the loophole didn't
> work for me for some reason.  If we can have a marker for objects to escape
> doc check legally, we can apply that to both @MigrationParameter and one
> other (perhaps @MigrationSetParameters).

I can see two useful QAPI generator features:

* Improved handling of missing member documentation

  Problem: many members lack documentation.  We silently generate
  documentation like

      name-of-member
          Not documented

  for them.

  Possible improvement: make missing member documentation a hard error,
  create a knob to suppress the error for a type.  Open question: how to
  best document member documentation is incomplete.

* Suppress documentation for internal-only definitions

  Problem: generated documentation covers everything, even types that
  aren't visible in QMP.  The irrelevant material is distracting and
  possibly confusing for users, and may be bothersome to maintain for
  developers.

  Possible improvement: include only the types visible in QMP in
  documentation, similar to how we do for query-qmp-schema.  Open
  question: what level of documentation to require for internal-only
  types.

On Tue, Oct 17, 2023 at 08:32:02AM +0200, Markus Armbruster wrote:
> I can see two useful QAPI generator features:

Agreed.

> 
> * Improved handling of missing member documentation
> 
>   Problem: many members lack documentation.  We silently generate
>   documentation like
> 
>       name-of-member
>           Not documented
> 
>   for them.
> 
>   Possible improvement: make missing member documentation a hard error,
>   create a knob to suppress the error for a type.  Open question: how to
>   best document member documentation is incomplete.

@MigrationSetParameters should fall into this category.

IMHO it's just wanted in some use case that we don't want to list member
documentations, instead we want to show something else. In this case
referring to documentation of another object (@MigrationParameters).

> 
> * Suppress documentation for internal-only definitions
> 
>   Problem: generated documentation covers everything, even types that
>   aren't visible in QMP.  The irrelevant material is distracting and
>   possibly confusing for users, and may be bothersome to maintain for
>   developers.
> 
>   Possible improvement: include only the types visible in QMP in
>   documentation, similar to how we do for query-qmp-schema.  Open
>   question: what level of documentation to require for internal-only
>   types.

@MigrationParameter should fall into this category.

IMHO we should treat them the same as any code written, for example, in C.
We don't necessarily need to apply any rule on it, like we don't require
comment for any line of code, but we prefer comments / documentations when
necessary.  That (how much documentation needed for the code) is judged
during code review, and can apply also to internally used QAPI definitions.

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Tue, Oct 17, 2023 at 08:32:02AM +0200, Markus Armbruster wrote:
>> I can see two useful QAPI generator features:
>
> Agreed.
>
>> 
>> * Improved handling of missing member documentation
>> 
>>   Problem: many members lack documentation.  We silently generate
>>   documentation like
>> 
>>       name-of-member
>>           Not documented
>> 
>>   for them.
>> 
>>   Possible improvement: make missing member documentation a hard error,
>>   create a knob to suppress the error for a type.  Open question: how to
>>   best document member documentation is incomplete.
>
> @MigrationSetParameters should fall into this category.

Unless we can get rid of it.

> IMHO it's just wanted in some use case that we don't want to list member
> documentations, instead we want to show something else. In this case
> referring to documentation of another object (@MigrationParameters).

Yes.  A different example is QKeyCode.

>> * Suppress documentation for internal-only definitions
>> 
>>   Problem: generated documentation covers everything, even types that
>>   aren't visible in QMP.  The irrelevant material is distracting and
>>   possibly confusing for users, and may be bothersome to maintain for
>>   developers.
>> 
>>   Possible improvement: include only the types visible in QMP in
>>   documentation, similar to how we do for query-qmp-schema.  Open
>>   question: what level of documentation to require for internal-only
>>   types.
>
> @MigrationParameter should fall into this category.

Yes.

> IMHO we should treat them the same as any code written, for example, in C.
> We don't necessarily need to apply any rule on it, like we don't require
> comment for any line of code, but we prefer comments / documentations when
> necessary.  That (how much documentation needed for the code) is judged
> during code review, and can apply also to internally used QAPI definitions.

Makes sense, but even then tools to assist with spotting missing
documentation would be useful.  How to best do that is not obvious to
me.

Markus Armbruster <armbru@redhat.com> writes:

[...]

> I can see two useful QAPI generator features:
>
> * Improved handling of missing member documentation
>
>   Problem: many members lack documentation.  We silently generate
>   documentation like
>
>       name-of-member
>           Not documented
>
>   for them.
>
>   Possible improvement: make missing member documentation a hard error,
>   create a knob to suppress the error for a type.  Open question: how to
>   best document member documentation is incomplete.
>
> * Suppress documentation for internal-only definitions
>
>   Problem: generated documentation covers everything, even types that
>   aren't visible in QMP.  The irrelevant material is distracting and
>   possibly confusing for users, and may be bothersome to maintain for
>   developers.
>
>   Possible improvement: include only the types visible in QMP in
>   documentation, similar to how we do for query-qmp-schema.  Open
>   question: what level of documentation to require for internal-only
>   types.

I expored this one, and ran into a roadblock.

I wrote a bit of code to mark the stuff we want in QMP documentation:
commands, events, and the types they use, directly or indirectly.

I then hacked QAPISchemaGenRSTVisitor.symbol() to skip entities not
so marked.

Surprisingly, this does not work: the generated QEMU QMP Reference
Manual lacks stuff that is clearly in QMP.

I double-checked, and yes, the missing stuff does get marked.  WTF?!?

I dug some, and it looks like this is due to interference between the
QEMU QMP Reference Manual (generated from qapi/qapi-schema.json) and the
QEMU Storage Daemon QMP Reference Manual (generated from
storage-daemon/qapi/qapi-schema.json).

Both qapi-schema.json include submodules from qapi/., the former all of
them, the latter only some of them.  Yes, this is a bit of a hack.

sphinx-build runs the QAPI machinery separately for each of the two.
Somehow results from one run can leak into the other one.  For instance,
type HumanReadableText is marked in one run and not the other (correct),
but its documentation is generated in *neither* run.  I suspect this is
an artifact of incremental doc building magic.

I'm shelving the idea for now.

Peter, looks like you missed my review of v2.

On Mon, Sep 25, 2023 at 02:59:30PM +0200, Markus Armbruster wrote:
> Peter, looks like you missed my review of v2.

I definitely missed that if there is, but I didn't see any, and also
nothing I spot on the list, either..

https://lore.kernel.org/all/20230825171517.1215317-1-peterx@redhat.com/

Could you share a pointer?

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Sep 25, 2023 at 02:59:30PM +0200, Markus Armbruster wrote:
>> Peter, looks like you missed my review of v2.
>
> I definitely missed that if there is, but I didn't see any, and also
> nothing I spot on the list, either..
>
> https://lore.kernel.org/all/20230825171517.1215317-1-peterx@redhat.com/
>
> Could you share a pointer?

Turns out several of my messages got eaten on the way out *sigh*.  I'll
resend my as replies to v3.