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", 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)
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.
