[libvirt] [PATCHv2 05/26] snapshot: one less point of failure in qemu

[Eric Blake]

On 08/15/2011 05:33 PM, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=727709
> mentions that if qemu fails to create the snapshot (such as what
> happens on Fedora 15 qemu, which has qmp but where savevm is only
> in hmp, and where libvirt is old enough to not try the hmp fallback),
> then 'virsh snapshot-list dom' will show a garbage snapshot entry,
> and the libvirt internal directory for storing snapshot metadata.
>
> This fixes the fallout bug of polluting the snapshot-list with
> garbage on failure (the root cause of the F15 bug of not having
> fallback to hmp has already been fixed in newer libvirt releases).

Not quite perfect, yet.  Both before and after this v2 patch, there is a 
potential null dereference:

> @@ -8511,32 +8519,25 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>         if (qemuDomainSnapshotCreateActive(domain->conn, driver,
>                                            &vm, snap) < 0)
>               goto cleanup;
>       }

Oops - qemuDomainSnapshotCreateActive can set vm to NULL yet still 
return 0, in the case of a transient domain where the snapshot is 
successfully taken, but then the vm quits and another thread cleans up 
while this thread is regaining the lock.

>
> -    /* FIXME: if we fail after this point, there's not a whole lot we can
> +    /* If we fail after this point, there's not a whole lot we can
>        * do; we've successfully taken the snapshot, and we are now running
>        * on it, so we have to go forward the best we can
>        */
> -
> -    if (vm->current_snapshot) {

Yet old code...

> -        def->parent = strdup(vm->current_snapshot->def->name);
> -        if (def->parent == NULL) {
> -            virReportOOMError();
> -            goto cleanup;
> -        }
> -    }
> -
> -    /* Now we set the new current_snapshot for the domain */
> -    vm->current_snapshot = snap;
> -
> -    if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot,
> -                                        driver->snapshotDir)<  0)
> -        /* qemuDomainSnapshotWriteMetadata set the error */
> +    if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir)<  0)

and new both blindly dereference vm.

Should qemuDomainSnapshotCreateXML temporarily bump the refs to the 
domain, so that qemuDomainSnapshotCreateActive is guaranteed that it is 
not releasing the last vm reference even if the vm quit in the meantime 
while locks were dropped?  But if the vm disappeared, then we do not 
want to record any metadata files.  I'm still playing with ideas for how 
to fix this.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org