Fabiano Rosas <[email protected]> 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 <[email protected]> > Signed-off-by: Fabiano Rosas <[email protected]> > --- > 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?
