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 <farosas@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 */
+    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);
+
+    len = ARRAY_SIZE(has_fields);
+    assert(len + n_str_args == MIGRATION_PARAMETER__MAX);
+
+    for (int i = 0; i < len; i++) {
+        *has_fields[i] = true;
+    }
+}
+
 MigrationParameters *qmp_query_migrate_parameters(Error **errp)
 {
     MigrationParameters *params;
@@ -943,68 +977,52 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
 
     /* TODO use QAPI_CLONE() instead of duplicating it inline */
     params = g_malloc0(sizeof(*params));
-    params->has_throttle_trigger_threshold = true;
+
     params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold;
-    params->has_cpu_throttle_initial = true;
     params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
-    params->has_cpu_throttle_increment = true;
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
-    params->has_cpu_throttle_tailslow = true;
     params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
 
     tls_option_set_str(&params->tls_creds, s->parameters.tls_creds);
     tls_option_set_str(&params->tls_hostname, s->parameters.tls_hostname);
     tls_option_set_str(&params->tls_authz, s->parameters.tls_authz);
 
-    params->has_max_bandwidth = true;
     params->max_bandwidth = s->parameters.max_bandwidth;
-    params->has_avail_switchover_bandwidth = true;
     params->avail_switchover_bandwidth = s->parameters.avail_switchover_bandwidth;
-    params->has_downtime_limit = true;
     params->downtime_limit = s->parameters.downtime_limit;
-    params->has_x_checkpoint_delay = true;
     params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
-    params->has_multifd_channels = true;
     params->multifd_channels = s->parameters.multifd_channels;
-    params->has_multifd_compression = true;
     params->multifd_compression = s->parameters.multifd_compression;
-    params->has_multifd_zlib_level = true;
     params->multifd_zlib_level = s->parameters.multifd_zlib_level;
-    params->has_multifd_qatzip_level = true;
     params->multifd_qatzip_level = s->parameters.multifd_qatzip_level;
-    params->has_multifd_zstd_level = true;
     params->multifd_zstd_level = s->parameters.multifd_zstd_level;
-    params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
-    params->has_max_postcopy_bandwidth = true;
     params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth;
-    params->has_max_cpu_throttle = true;
     params->max_cpu_throttle = s->parameters.max_cpu_throttle;
-    params->has_announce_initial = true;
     params->announce_initial = s->parameters.announce_initial;
-    params->has_announce_max = true;
     params->announce_max = s->parameters.announce_max;
-    params->has_announce_rounds = true;
     params->announce_rounds = s->parameters.announce_rounds;
-    params->has_announce_step = true;
     params->announce_step = s->parameters.announce_step;
-
-    params->has_block_bitmap_mapping = true;
     params->block_bitmap_mapping =
         QAPI_CLONE(BitmapMigrationNodeAliasList,
                    s->parameters.block_bitmap_mapping);
-
-    params->has_x_vcpu_dirty_limit_period = true;
     params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period;
-    params->has_vcpu_dirty_limit = true;
     params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
-    params->has_mode = true;
     params->mode = s->parameters.mode;
-    params->has_zero_page_detection = true;
     params->zero_page_detection = s->parameters.zero_page_detection;
-    params->has_direct_io = true;
     params->direct_io = s->parameters.direct_io;
 
+    /*
+     * query-migrate-parameters expects all members of
+     * MigrationParameters to be present, but we cannot mark them
+     * non-optional in QAPI because the structure is also used for
+     * migrate-set-parameters, which needs the optionality. Force all
+     * parameters to be seen as present now. Note that this depends on
+     * some form of default being set for every member of
+     * MigrationParameters, currently done during qdev init using
+     * migration_properties defined in this file.
+     */
+    migrate_mark_all_params_present(params);
     return params;
 }
 
-- 
2.35.3

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 <farosas@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?

> +    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..

> +
> +    len = ARRAY_SIZE(has_fields);
> +    assert(len + n_str_args == MIGRATION_PARAMETER__MAX);

.. I definitely like this assert.

> +
> +    for (int i = 0; i < len; i++) {
> +        *has_fields[i] = true;
> +    }
> +}
> +
>  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>  {
>      MigrationParameters *params;
> @@ -943,68 +977,52 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>  
>      /* TODO use QAPI_CLONE() instead of duplicating it inline */
>      params = g_malloc0(sizeof(*params));
> -    params->has_throttle_trigger_threshold = true;
> +
>      params->throttle_trigger_threshold = s->parameters.throttle_trigger_threshold;
> -    params->has_cpu_throttle_initial = true;
>      params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
> -    params->has_cpu_throttle_increment = true;
>      params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
> -    params->has_cpu_throttle_tailslow = true;
>      params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
>  
>      tls_option_set_str(&params->tls_creds, s->parameters.tls_creds);
>      tls_option_set_str(&params->tls_hostname, s->parameters.tls_hostname);
>      tls_option_set_str(&params->tls_authz, s->parameters.tls_authz);
>  
> -    params->has_max_bandwidth = true;
>      params->max_bandwidth = s->parameters.max_bandwidth;
> -    params->has_avail_switchover_bandwidth = true;
>      params->avail_switchover_bandwidth = s->parameters.avail_switchover_bandwidth;
> -    params->has_downtime_limit = true;
>      params->downtime_limit = s->parameters.downtime_limit;
> -    params->has_x_checkpoint_delay = true;
>      params->x_checkpoint_delay = s->parameters.x_checkpoint_delay;
> -    params->has_multifd_channels = true;
>      params->multifd_channels = s->parameters.multifd_channels;
> -    params->has_multifd_compression = true;
>      params->multifd_compression = s->parameters.multifd_compression;
> -    params->has_multifd_zlib_level = true;
>      params->multifd_zlib_level = s->parameters.multifd_zlib_level;
> -    params->has_multifd_qatzip_level = true;
>      params->multifd_qatzip_level = s->parameters.multifd_qatzip_level;
> -    params->has_multifd_zstd_level = true;
>      params->multifd_zstd_level = s->parameters.multifd_zstd_level;
> -    params->has_xbzrle_cache_size = true;
>      params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
> -    params->has_max_postcopy_bandwidth = true;
>      params->max_postcopy_bandwidth = s->parameters.max_postcopy_bandwidth;
> -    params->has_max_cpu_throttle = true;
>      params->max_cpu_throttle = s->parameters.max_cpu_throttle;
> -    params->has_announce_initial = true;
>      params->announce_initial = s->parameters.announce_initial;
> -    params->has_announce_max = true;
>      params->announce_max = s->parameters.announce_max;
> -    params->has_announce_rounds = true;
>      params->announce_rounds = s->parameters.announce_rounds;
> -    params->has_announce_step = true;
>      params->announce_step = s->parameters.announce_step;
> -
> -    params->has_block_bitmap_mapping = true;
>      params->block_bitmap_mapping =
>          QAPI_CLONE(BitmapMigrationNodeAliasList,
>                     s->parameters.block_bitmap_mapping);
> -
> -    params->has_x_vcpu_dirty_limit_period = true;
>      params->x_vcpu_dirty_limit_period = s->parameters.x_vcpu_dirty_limit_period;
> -    params->has_vcpu_dirty_limit = true;
>      params->vcpu_dirty_limit = s->parameters.vcpu_dirty_limit;
> -    params->has_mode = true;
>      params->mode = s->parameters.mode;
> -    params->has_zero_page_detection = true;
>      params->zero_page_detection = s->parameters.zero_page_detection;
> -    params->has_direct_io = true;
>      params->direct_io = s->parameters.direct_io;
>  
> +    /*
> +     * query-migrate-parameters expects all members of
> +     * MigrationParameters to be present, but we cannot mark them
> +     * non-optional in QAPI because the structure is also used for
> +     * migrate-set-parameters, which needs the optionality. Force all
> +     * parameters to be seen as present now. Note that this depends on
> +     * some form of default being set for every member of
> +     * MigrationParameters, currently done during qdev init using
> +     * migration_properties defined in this file.
> +     */
> +    migrate_mark_all_params_present(params);
>      return params;
>  }
>  
> -- 
> 2.35.3
> 

-- 
Peter Xu

Peter Xu <peterx@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 <farosas@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.

On Fri, Jun 06, 2025 at 12:51:58PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <farosas@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.

Yes, this makes sense.

-- 
Peter Xu