[libvirt] [PATCH v2 11/11] qemu: Implement bulk snapshot operations

[Eric Blake]

On 2/27/19 11:27 AM, John Ferlan wrote:
> 
> 
> On 2/23/19 4:24 PM, Eric Blake wrote:
>> Implement the new flags for bulk snapshot dump and redefine. This
>> borrows from ideas in the test driver, but is further complicated
>> by the fact that qemu must write snapshot XML to disk, and thus must
>> do additional validation after the initial parse to ensure the user
>> didn't attempt to rename a snapshot with "../" or similar.
>>
>> Of note: all prior callers of qemuDomainSnapshotDiscardAllMetadata()
>> were at points where it did not matter if vm->current_snapshot and
>> the metaroot in vm->snapshots were left pointing to stale memory,
>> because we were about to delete the entire vm object; but now it is
>> important to reset things properly so that the domain still shows
>> as having no snapshots on failure.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>  src/qemu/qemu_domain.h |  2 +-
>>  src/qemu/qemu_domain.c | 35 +++++++++++++++++------
>>  src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 89 insertions(+), 12 deletions(-)
>>
> 
> NB: I couldn't get this one to git am -3 apply - I didn't chase the
> conflict though.

My fault for too many patches in flight at once. As I'll be doing a v3
anyways, that one wil be cleaner.

> 
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 7c6b50184c..37c9813ec5 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -683,7 +683,7 @@ int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
>>                                      virDomainSnapshotObjPtr snapshot,
>>                                      virCapsPtr caps,
>>                                      virDomainXMLOptionPtr xmlopt,
>> -                                    char *snapshotDir);
>> +                                    const char *snapshotDir);
> 
> Theoretically separable.

Yeah, I debated about it. Since you called me on it, I'll make the split.


>> @@ -7881,7 +7884,14 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver,
>>      }
>>
>>   format:
>> -    ret = virDomainDefFormatInternal(def, caps, NULL, NULL,
>> +    if (snapshots && !vm) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("snapshots XML requested but not provided"));
> 
> Error msg is a bit odd - the error is that a snapshot listing was
> desired, but consumer didn't pass the @vm object.

It's a programmer's error, not user-triggerable.  (If we could assert()
or abort(), that's what I'd have done instead).  And really, it is no
snapshots were provided, because it is vm->snapshots that we depend on.

> 
>> +        goto cleanup;
>> +    }
>> +    ret = virDomainDefFormatInternal(def, caps,
>> +                                     snapshots ? vm->snapshots : NULL,
>> +                                     snapshots ? vm->current_snapshot : NULL,
>>                                       virDomainDefFormatConvertXMLFlags(flags),
>>                                       buf, driver->xmlopt);
> 
> Perhaps this one should [be | have been] turned into a
> virDomainDefFormatFull (or whatever new name is chosen if one is
> chosen).  I think it shows the hazards of exposing *Internal to many
> consumers.

It's a ripple effect - I had to decide how many interfaces needed an
additional parameter, even if the caller wouldn't be using it. But your
idea of an auxiliary struct may make it nicer.


>> +        /* Return is arbitrary, so use the first root */
>> +        snap = virDomainSnapshotFindByName(vm->snapshots, NULL);
> 
> Similar to test driver - this isn't used during cleanup.
> 
> John
> 
>> +        snapshot = virGetDomainSnapshot(domain, snap->first_child->def->name);

Yeah, but it's not leaked, and it let me avoid a long line.

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