Re: [PATCH 10/18] migration: Validate that all params are set for query

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 02, 2026 at 07:15:38PM -0300, Fabiano Rosas wrote:
>> There are a couple of situations where all fields of a
>> MigrationParameters object need to be marked as present: when cloning
>> an entire object and when creating the transient object in
>> qmp_query_migrate(). The query-migrate-parameters QMP command contract
>> requires that all parameters, except block-bitmap-mapping, are present
>> in the output.
>> 
>> Validate that a given object has all has_* fields set to true.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 54 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index bd7be8f9832..5d17acdd881 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -12,6 +12,7 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/units.h"
>>  #include "exec/target_page.h"
>> @@ -23,8 +24,10 @@
>>  #include "qapi/qmp/qerror.h"
>>  #include "qapi/qobject-input-visitor.h"
>>  #include "qapi/qobject-output-visitor.h"
>> +#include "qobject/qbool.h"
>>  #include "qobject/qdict.h"
>>  #include "qobject/qnull.h"
>> +#include "qobject/qstring.h"
>>  #include "system/runstate.h"
>>  #include "migration/colo.h"
>>  #include "migration/cpr.h"
>> @@ -1149,12 +1152,63 @@ static void migrate_mark_all_params_present(MigrationParameters *p)
>>      }
>>  }
>>  
>> +static bool assert_all_params_present(MigrationParameters *params, Error **errp)
>> +{
>> +    g_autoptr(QDict) d = migrate_params_to_dict(params, errp);
>> +    const QDictEntry *e = NULL;
>> +    int i = 0;
>> +
>> +    if (!d) {
>> +        return false;
>> +    }
>> +
>> +    for (e = qdict_first(d); e; e = qdict_next(d, e), i++) {
>> +        const char *key = qdict_entry_key(e);
>> +        const char *p;
>> +
>> +        if (strstart(key, "tls-", &p)) {
>> +            QString *s = qobject_to(QString, qdict_entry_value(e));
>> +
>> +            if (!s) {
>> +                break;
>> +            }
>> +        } else if (strstart(key, "has-", &p)) {
>
> Does the qdict contain any has- field?  
>

¬¬

I guess that decides the fate of the MigrationParameter enum.

> visit_type_MigrationParameters_members:
>
>     if (visit_optional(v, "announce-initial", &obj->has_announce_initial)) {
>         if (!visit_type_size(v, "announce-initial", &obj->announce_initial, errp)) {
>             return false;
>         }
>     }
>     ...
>
> It seems the has_* fields are only used to identify existance of objects,
> not converted.
>
>> +            if (qdict_haskey(d, p)) {
>> +                QBool *b = qobject_to(QBool, qdict_entry_value(e));
>> +
>> +                If (!b || !qbool_get_bool(b)) {
>> +                    break;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    if (i && !e) {
>> +        return true;
>> +    }
>> +
>> +    /*
>> +     * Should never happen, but avoid asserting becase this is
>> +     * reachable from QMP.
>
> IIUC as long as this fact shouldn't be changed by any possible user input,
> we could still assert.  But I understand you want to be careful, maybe
> either (1) directly assert, or (2) change the function name,
> s/assert/check/?  I vote (1).
>
> Said that, if the qdict trick didn't work it beats the whole patch.. so
> IMHO we can also leave this sanity check for later too.  Your call.
>
>> +     */
>> +    error_setg(errp, "Missing parameter. Query output will be incomplete.");
>> +    return false;
>> +}
>> +
>>  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>  {
>>      MigrationState *s = migrate_get_current();
>>      MigrationParameters *params = QAPI_CLONE(MigrationParameters,
>>                                               &s->parameters);
>>  
>> +    /*
>> +     * Validate all parameters have their has_* field set to true as
>> +     * consequence of the initial migrate_mark_all_params_present().
>> +     */
>> +    if (!assert_all_params_present(params, errp)) {
>> +        return NULL;
>> +    }
>> +
>>      /*
>>       * The block-bitmap-mapping breaks the expected API of
>>       * query-migrate-parameters of having all members present. To keep
>> -- 
>> 2.53.0
>>