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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/20/19 4:28 PM, John Ferlan wrote:
> 
> 
> 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.

Good find from Coverity. If there are no children to move, I do need the
early exit, so I'll squash that in.

> 
>> +    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.

I'll call out more details in the commit message.

>> -    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.

Yeah, we don't want to continue with either a -1 error (which shouldn't
happen in practice) or a > 0 result (more likely); but adding '!= 0'
does appear to make it clearer that I thought about both directions.

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

I ended up splitting into two - everything else is simple, and then
MoveChildren in isolation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux