| Message ID | 20230905162335.235619-1-peterx@redhat.com |
|---|---|
| Headers | show |
| Series | qapi/migration: Dedup migration parameter objects and fix tls-authz crash | expand |
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 <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.
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 <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 <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.
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(-)