Re: [PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Peter Xu <peterx@redhat.com>]

On Mon, May 29, 2023 at 12:55:30PM +0000, Wang, Wei W wrote:
> On Saturday, May 27, 2023 5:49 AM, Peter Xu wrote:
> > On Wed, May 24, 2023 at 04:01:57PM +0800, Wei Wang wrote:
> > > qmp_migrate_set_parameters expects to use tmp for parameters check, so
> > > migrate_params_test_apply is expected to copy the related fields from
> > > params to tmp. So fix migrate_params_test_apply to use the function
> > > parameter, *dest, rather than the global one. The dest->has_xxx (xxx
> > > is the feature name) related fields need to be set, as they will be
> > > checked by migrate_params_check.
> > 
> > I think it's fine to do as what you suggested, but I don't see much benefit
> > either.. the old code IIUC will check all params even if 1 param changed,
> > while after your change it only checks the modified ones.
> > 
> > There's slight benefits but not so much, especially "22+, 2-" LOCs, because
> > we don't really do this a lot; some more sanity check also makes sense to me
> > even if everything is always checked, so we'll hit errors if anything
> > accidentally goes wrong too.
> > 
> > Is there a real bug somewhere?
> 
> Yes. Please see qmp_migrate_set_parameters:
> 
> #1    migrate_params_test_apply(params, &tmp);
> 
>  #2   if (!migrate_params_check(&tmp, errp)) {
>         /* Invalid parameter */
>         return;
>     }
>  #3  migrate_params_apply(params, errp);
> 
> #2 tries to do params check using tmp, which is expected to be set up
> by #1, but #1 didn't use "&tmp",

#1 initialized "&tmp" with current parameters, here:

    *dest = migrate_get_current()->parameters;

?

> so "tmp" doesn’t seem to store the
> valid values as expected for the check (that is, #2 above isn’t effectively
> doing any check for the user input params)

Do you have a reproducer where qmp set param will not check properly on
user input?

> 
> The alternative fix would be to remove the intermediate "tmp" params,
> but this might break the usage from commit 1bda8b3c6950, so need thoughts
> from Markus if we want go for this approach.

Thanks,

-- 
Peter Xu