Re: [PATCH v2 5/9] qapi: Add QAPI_MERGE

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> The migration subsystem currently has code to merge two objects of the
> same type. It does so by checking which fields are present in a source
> object and overwriting the corresponding fields on the destination
> object. This leads to a lot of open-coded lines such as:
>
>     if (src->has_foobar) {
>         dst->foobar = src->foobar;
>     }

It does this to update configuration.  Both the configuration and the
update is a struct MigrationParameters.  To enable update, not just
complete overwrite, the struct's members are all optional.  If update's
member is present, it replaces the configuration's member.

Note that this is a *shallow* operation.  There's no recursion.

MigrationParameters is almost flat: two members are arrays.  These
members are updated wholesale, i.e. an array is either not touched or
replaced completely.  We don't look at array elements.

> This pattern could be replaced by a copy using visitors. Implement a
> macro that extracts elements from a source object using an output
> visitor and merges it with a destination object using an input
> visitor.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  include/qapi/type-helpers.h | 40 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
> index fc8352cdec..f22d7b7139 100644
> --- a/include/qapi/type-helpers.h
> +++ b/include/qapi/type-helpers.h
> @@ -10,6 +10,9 @@
>   */
>  
>  #include "qapi/qapi-types-common.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
> +#include "qapi/dealloc-visitor.h"
>  
>  HumanReadableText *human_readable_text_from_str(GString *str);
>  
> @@ -20,3 +23,40 @@ HumanReadableText *human_readable_text_from_str(GString *str);
>   * cleanup.
>   */
>  char **strv_from_str_list(const strList *list);
> +
> +/*
> + * 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. Pointer members that are overwritten are also freed.
> + */
> +#define QAPI_MERGE(type, dst_, src_)                                    \

The comment uses @dst and @src, the code uses dst_ and src_.  Make them
consistent, please.

> +    ({                                                                  \
> +        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_);                                                 \

This serializes @src_ to @out_.

> +        /*                                                              \
> +         * Free the memory for which the pointers will be overwritten   \
> +         * in the next step.                                            \
> +         */                                                             \
> +        v_ = qapi_dealloc_present_visitor_new(out_);                    \
> +        visit_start_struct(v_, NULL, NULL, 0, &error_abort);            \
> +        visit_type_ ## type ## _members(v_, dst_, &error_abort);        \
> +        visit_end_struct(v_, NULL);                                     \
> +        visit_free(v_);                                                 \

This frees members of @dst_ that are about to be overwritten, using the
dealloc present visitor from the previous patch.  Which I haven't
reviewed, yet.

> +        /*                                                              \
> +         * 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_);                                                 \

This deserializes @out_ into @dst_, but in an unusual way.  The usual
way would be like

           visit_start_struct(v, name, (void **)&dst_, sizeof(type),
                              &error_abort);
           visit_type_ ## type ## _members(v, dst_, &error_abort);
           visit_check_struct(v, &error_abort);
           visit_end_struct(v, (void **)&dst_);

This is code from generated visit_type_TYPE() with @obj replaced by
&dst_, @errp replaced by &error_abort, and handling now impossible
errors deleted.

There's a a small difference that doesn't actually matter:
visit_start_struct() argument @name.  visit_start_struct() ignores this
argument when visiting the root of a tree (again, see the big comment in
qapi/visitor.h).

The difference that matters follows.

The usual way passes &dst_ to visit_start_struct(), visit_end_struct(),
and dst_ to visit_type_TYPE_members().

The other usual way passes NULL to all three.  That's a virtual walk.
See also the big comment in qapi/visitor.h.

Your QAPI_MERGE() mixes the two.  Hmm.

Input visitors create new objects / work on zero-initialized objects:
visit_type_TYPE_members() with an input visitor takes a TYPE *obj
pointing to zero-initialized memory.

QAPI_MERGE() does not zero-initialize the memory it passes to the input
visitor.  It merely frees objects the input visitor is going to
overwrite.  

Memory the input visitor doesn't overwrite remains as it is.  As long as
the input visitor overwrite exactly all the member memory for each
member it actually visits, this results in a merge operation.

At this point, I have a queasy feeling in my stomach: we're upending
design assumptions.  I can't point to anything wrong, but this is risky
business.

Another observation: visitors are deep, not shallow.  What happens when
@src isn't flat?  Say it contains an array member.  The input visitor
will first replace @dst's array wholesale by @src's.  Then it'll duly
recurse into the array, and replace each of its members by itself.  I
think.  Does that work?  I hope.  Is it weird and surprising?  Yes!

> +        qobject_unref(out_);                                            \
> +    })

Could we do this in a simpler and cleaner way?

In handwritten code, we'd simply walk source and destination
simultaneously.  The visitor infrastructure doesn't let us do that; it
fundamentally walks a single object.  Perhaps we could somehow cajole it
into letting us do it.  Can't tell offhand.

We could have an input visitor that updates an existing object.  No need
for a dealloc present visitor then.  It should take care to avoid the
recursive update I called weird and surprising.

We could serialize both objects to be merged, shallowly merge the
resulting QDicts, deserialize.  One more serialize, which is expensive,
but if two expensive expensive serialize / deserialize is fine, three
should be fine, too.  No need for a dealloc present visitor then.

Thoughts?