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 <ebl...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
