Re: [PATCH v2 14/24] migration: Use visitors in migrate_params_test_apply

[Fabiano Rosas <farosas@suse.de>]

Bin Guo <guobin@linux.alibaba.com> writes:

> Fabiano Rosas wrote on Mon, 30 Jun 2025 16:59:03 -0300:
>
>> Convert the code in migrate_params_test_apply() from an open-coded
>> copy of every migration parameter to a copy using visitors. The
>> current code has conditionals for each parameter's has_* field, which
>> is exactly what the visitors do.
>> 
>> This hides the details of QAPI from the migration code and avoids the
>> need to update migrate_params_test_apply() every time a new migration
>> parameter is added. Both were very confusing and while the visitor
>> code can become a bit involved, there is no need for new contributors
>> to ever touch it.
>> 
>> Change the name of the function to a more direct reference of what it
>> does: merging the user params with the temporary copy.
>> 
>> Move the QAPI_CLONE_MEMBERS into the caller, so QAPI_CLONE can be used
>> and there's no need to allocate memory in the migration
>> code. Similarly, turn 'tmp' into a pointer so the proper qapi_free_
>> routine can be used.
>> 
>> An extra call to migrate_mark_all_params_present() is now needed
>> because the visitors update the has_ field for non-present fields, but
>> we actually want them all set so migrate_params_apply() can copy all
>> of them.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 157 +++++++++++++++-----------------------------
>>  1 file changed, 54 insertions(+), 103 deletions(-)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 6619b5f21a..695bec5b8f 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> 
>> -static void migrate_params_test_apply(MigrationParameters *params,
>> -                                      MigrationParameters *dest)
>> +static bool migrate_params_merge(MigrationParameters *dst,
>> +
>> +    ...
>> +    /* read in from src */
>> +    v = qobject_output_visitor_new(&ret_out);
>> +    ok = visit_type_MigrationParameters(v, NULL, &src, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> +    visit_complete(v, &ret_out);
>> +    visit_free(v);
>>  
>> -    if (params->has_max_bandwidth) {
>> -        dest->max_bandwidth = params->max_bandwidth;
>> +    /*
>> +     * Write to dst but leave existing fields intact (except for has_*
>> +     * which will be updated according to their presence in src).
>> +     */
>> +    v = qobject_input_visitor_new(ret_out);
>> +    ok = visit_start_struct(v, NULL, NULL, 0, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> -
>> -    if (params->has_avail_switchover_bandwidth) {
>> -        dest->avail_switchover_bandwidth = params->avail_switchover_bandwidth;
>> +    ok = visit_type_MigrationParameters_members(v, dst, errp);
>> +    if (!ok) {
>> +        goto out;
>>      }
>> -
>> -    if (params->has_downtime_limit) {
>> -        dest->downtime_limit = params->downtime_limit;
>> +    ok = visit_check_struct(v, errp);
>> +    visit_end_struct(v, NULL);
>> +    if (!ok) {
>> +        goto out;
>>      }
>>  
>> -    if (params->has_x_checkpoint_delay) {
>> -        dest->x_checkpoint_delay = params->x_checkpoint_delay;
>> -    }
>> +out:
>> +    visit_free(v);
>> +    qobject_unref(ret_out);
>> +    return ok;
>> }
>
> If visit_start_struct is executed successfully, then visit_end_struct
> should be executed. IMHO:
>
>     v = qobject_input_visitor_new(ret_out);
>     ok = visit_start_struct(v, NULL, NULL, 0, errp);
>     if (!ok) {
>         goto out;
>     }
>
>     ok = visit_type_MigrationParameters_members(v, dst, errp);
>     if (!ok) {
>         goto out_end;
>     }
>
>     ok = visit_check_struct(v, errp);
>
> out_end:
>     visit_end_struct(v, NULL);
>
> out:
>     visit_free(v);
>     qobject_unref(ret_out);
>
>     return ok;
> }
>

New code after Peter's suggestions is:

+/*
+ * Merge @src over @dst by copying deep clones of the present members
+ * from @src to @dst. Non-present on @src are left untouched on @dst.
+ */
+#define QAPI_MERGE(type, dst_, src_)                                    \
+    ({                                                                  \
+        QObject *out_ = NULL;                                           \
+        Visitor *v_;                                                    \
+        /* read in from src */                                          \
+        v_ = qobject_output_visitor_new(&out_);                         \
+        visit_type_ ## type (v_, NULL, &src_, &error_abort);            \
+        visit_complete(v_, &out_);                                      \
+        visit_free(v_);                                                 \
+        /*                                                              \
+         * Write to dst but leave existing fields intact (except for    \
+         * has_* which will be updated according to their presence in   \
+         * src).                                                        \
+         */                                                             \
+        v_ = qobject_input_visitor_new(out_);                           \
+        visit_start_struct(v_, NULL, NULL, 0, &error_abort);            \
+        visit_type_ ## type ## _members(v_, dst_, &error_abort);        \
+        visit_check_struct(v_, &error_abort);                           \
+        visit_end_struct(v_, NULL);                                     \
+        visit_free(v_);                                                 \
+        qobject_unref(out_);                                            \
+    })