Re: [libvirt] [PATCH 07/16] snapshot: Add accessors for updating snapshot list relations

On 3/20/19 1:40 AM, Eric Blake wrote: > Rather than allowing a leaky abstraction where multiple drivers have > to open-code operations that update the relations in a > virDomainSnapshotObjList, it is better to add accessor functions so > that updates to relations are maintained closer to the internals. The > goal here is to avoid access to, nchildren, first_child, or sibling > outside of the lowest level functions, making it easier to refactor > later on. While many of the conversions are fairly straightforward, > the MoveChildren refactoring can be a bit interesting to follow. Understatement ;-) Some black magic occurs The qemuDomainSnapshotReparentChildren "snap->parent = rep->parent" gets replaced by the new for loop... Tough to see without watching really closely.

> +/* Take all children of @from and convert them into children of @to. */ > +void > +virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from, > + virDomainSnapshotObjPtr to) > +{ > + virDomainSnapshotObjPtr child; > + virDomainSnapshotObjPtr last; > + > + for (child = from->first_child; child; child = child->sibling) { > + child->parent = to; > + if (!child->sibling) > + last = child; > + } > + to->nchildren += from->nchildren; > + last->sibling = to->first_child; Silly Coverity compiler gets quite confused thinking that @last couldn't be set while not considering the above loop couldn't end without it unless of course from->first_child == NULL I suppose, which would be a different issue. Still if before the for loop we check "if (!from->first_child) return;", then coverity is happy.

> + to->first_child = from->first_child; > + from->nchildren = 0; > + from->first_child = NULL; Or virDomainSnapshotDropChildren... Still makes sense, albeit new. Seems to make VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY processing unnecessary. It's not a problem but perhaps worthy of a mention.

> - if (snapshots->metaroot.nchildren || snapshots->current) { > + if (virDomainSnapshotObjListSize(snapshots)) { The only way the call could return < 0 from virHashSize is if @snapshots == NULL... Just noting it - no problem.

> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > _("bulk define of snapshots only possible with " > "no existing snapshot")); Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John BTW: There could have been 3 patches out of this, but I'm fine with 1.