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
