Fabiano Rosas <[email protected]> writes: > Markus Armbruster <[email protected]> writes: > >> Fabiano Rosas <[email protected]> writes: >> >>> The migrate_set_parameter_completion function is the last user of the >>> MigrationParameter enum. >> >> Not true, there's still the assertion in >> migrate_mark_all_params_present(), and hmp_completion_single() in >> tests/qtest/migration/misc-tests.c. >> >> The assertion is unclean: it uses MIGRATION_PARAMETER__MAX for number of >> MigrationParameters members, which tacitly assumes MigrationParameter >> matches MigrationParameters. >> > > I'm not sure I understand what you mean here. The usefulness of > MigrationParameter is quite diminished if it doesn't always match the > members of MigrationParameters.
What we want to assert is "has_fields[] covers all the optional members of MigrationParameters *p. That's hard, so we settle for "has_fields[] covers the exact right number of bool pointers". Works as long as all elements of has_fields[] point into @p, and there are no duplicates. Note that "exact right number" is necessary but not sufficient for the "covers all". In other words, we're settling for a weaker assertion we can actually check. That's okay. But that's still hard, so we assert "has_fields[] covers as many bool pointers as there are MigrationParameter values" instead. Works as long as MigrationParameter matches MigrationParameters. This isn't settling for a weaker assertion, it's asserting something else! It tacitly relies on "MigrationParameter matches MigrationParameters", which is not locally obvious, and not ensured by code, only by patch review. I consider that unclean. Doesn't mean it's worthless, does mean it needs a comment. > In fact... could we code-generate the enum from MigrationParameters > without the need for explicitly declaring MigrationParameter? That way > we keep the usefulness of having a lookup function and a "max" and do > away with the task of updating MigrationParameter to match > MigrationParameters. Yes, we could, but it would be too much QAPI generator and documentation complexity for just one use case. The existing tool to introspect QAPI types from C is visitors. Could it work? A bit of a bother, I'm afraid, but less complex than messing with the QAPI generator. >>> Write the code using an output visitor and >>> QDict instead so we can remove the enum in a future patch. >>> >>> Signed-off-by: Fabiano Rosas <[email protected]>
