Re: [PATCH 09/21] migration: Extract code to mark all parameters as present

[Peter Xu <peterx@redhat.com>]

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