Peter Xu <pet...@redhat.com> writes:
> On Mon, Jun 02, 2025 at 10:37:58PM -0300, Fabiano Rosas wrote:
>> MigrationParameters needs to have all of its has_* fields marked as
>> true when used as the return of query_migrate_parameters because the
>> corresponding QMP command has all of its members non-optional by
>> design, despite them being marked as optional in migration.json.
>>
>> Extract this code into a function and make it assert if any field is
>> missing. With this we ensure future changes will not inadvertently
>> leave any parameters missing.
>>
>> Also assert that s->parameters _does not_ have any of its has_* fields
>> set. This structure is internal to the migration code and it should
>> not rely on the QAPI-generate has_* fields. We might want to store
>> migration parameters differently in the future.
>>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> migration/options.c | 74 ++++++++++++++++++++++++++++-----------------
>> 1 file changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/migration/options.c b/migration/options.c
>> index e2e3ab717f..dd62e726cb 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -936,6 +936,40 @@ static void tls_option_set_str(StrOrNull **dstp,
>> StrOrNull *src)
>> }
>> }
>>
>> +static void migrate_mark_all_params_present(MigrationParameters *p)
>> +{
>> + int len, n_str_args = 3; /* tls-creds, tls-hostname, tls-authz */
>
> Could you remind me why we don't set has_*=true for these three?
>
I doesn't exist. These are StrOrNull so their presence is supposed to be
determined by checking against NULL pointer.
>> + bool *has_fields[] = {
>> + &p->has_throttle_trigger_threshold, &p->has_cpu_throttle_initial,
>> + &p->has_cpu_throttle_increment, &p->has_cpu_throttle_tailslow,
>> + &p->has_max_bandwidth, &p->has_avail_switchover_bandwidth,
>> + &p->has_downtime_limit, &p->has_x_checkpoint_delay,
>> + &p->has_multifd_channels, &p->has_multifd_compression,
>> + &p->has_multifd_zlib_level, &p->has_multifd_qatzip_level,
>> + &p->has_multifd_zstd_level, &p->has_xbzrle_cache_size,
>> + &p->has_max_postcopy_bandwidth, &p->has_max_cpu_throttle,
>> + &p->has_announce_initial, &p->has_announce_max,
>> &p->has_announce_rounds,
>> + &p->has_announce_step, &p->has_block_bitmap_mapping,
>> + &p->has_x_vcpu_dirty_limit_period, &p->has_vcpu_dirty_limit,
>> + &p->has_mode, &p->has_zero_page_detection, &p->has_direct_io,
>> + };
>> +
>> + /*
>> + * The has_* fields of MigrationParameters are used by QAPI to
>> + * inform whether an optional struct member is present. Keep this
>> + * decoupled from the internal usage (not QAPI) by leaving the
>> + * has_* fields of s->parameters unused.
>> + */
>> + assert(p != &(migrate_get_current())->parameters);
>
> This is OK, I'm not sure whether we're over-cautious though.. but..
>
Hopefully after this series the code will be encapsulated enough that we
don't need to think about this, but before this series the situation is
definitely confusing enough that we need to know which fields are used
for what.
I don't want to see people passing s->parameters into here thinking it's
all the same, because it isn't. The has_* fields should be used only for
QAPI stuff, user input validation, etc, while s->parameters is the thing
that stores all that after validation and there's not reason to be
messing with has_* since we know that's just an consequence of the fact
that we're choosing to use the same QAPI type for user input/output and
internal storage.
I guess what I'm trying to do is take the pain points where I got
confused while working on the current code and introduce some hard rules
to it.
>> +
>> + len = ARRAY_SIZE(has_fields);
>> + assert(len + n_str_args == MIGRATION_PARAMETER__MAX);
>
> .. I definitely like this assert.
>
Yep, we can't at the moment get rid of the enum because HMP needs it, so
let's put it to good use. I think this is specially useful for new
contributors, so they don't need to guess what needs to be changed when
adding a new parameter.
