[libvirt] [PATCH 0/8] more snapshot improvements [incremental backup saga]
Various things that I've tweaked while working on addressing Peter's comments about my v8.5 checkpoint series. I'm less certain about patches 7 and 8 (as having multiple ways to spell an operation, but where one way fails with older servers, can be confusing), we may want to drop those two and just take the first 6. Eric Blake (8): snapshot: Rename qemu domain snapshot test files snapshot: Fix virDomainUndefineFlags docs regarding snapshots snapshot: Add internal option to validate XML against schema snapshot: Add VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE flag snapshot: Factor out redefine cycle validation backup: Add support for filtering based on current moment snapshot: Add ListAll filters for current snapshot snapshot: Expose new filter flags in virsh include/libvirt/libvirt-domain-snapshot.h | 7 +++ src/conf/snapshot_conf.h | 1 + src/conf/virdomainmomentobjlist.h | 14 ++++- src/conf/virdomainsnapshotobjlist.h | 10 +++- src/conf/snapshot_conf.c | 54 +++++++------------ src/conf/virdomainmomentobjlist.c | 49 ++++++++++++++++- src/conf/virdomainsnapshotobjlist.c | 13 +++++ src/libvirt-domain-snapshot.c | 17 +++++- src/libvirt-domain.c | 9 ++-- src/qemu/qemu_driver.c | 6 ++- src/test/test_driver.c | 6 ++- src/vbox/vbox_common.c | 11 ++-- src/vz/vz_driver.c | 5 +- tests/Makefile.am | 14 ++--- .../description_only.xml | 0 .../disk-invalid.xml | 0 .../disk-network-seclabel-invalid.xml | 0 .../disk-seclabel.xml | 0 .../disk_driver_name_null.xml | 0 .../disk_snapshot.xml | 0 .../empty.xml | 0 .../external_vm.xml | 0 .../name_and_description.xml | 0 .../name_only.xml | 0 .../noparent.xml | 0 .../all_parameters.xml | 0 .../disk-seclabel.xml | 0 .../disk_driver_name_null.xml | 0 .../disk_snapshot.xml | 0 .../disk_snapshot_redefine.xml | 0 .../empty.xml | 0 .../external_vm.xml | 0 .../external_vm_redefine.xml | 0 .../full_domain.xml | 0 .../metadata.xml | 0 .../name_and_description.xml | 0 .../noparent.xml | 0 .../noparent_nodescription.xml | 0 .../noparent_nodescription_noactive.xml | 0 ...test.c => qemudomainsnapshotxml2xmltest.c} | 15 +++--- tests/virschematest.c | 4 +- tests/virsh-snapshot | 16 ++++-- tools/virsh-snapshot.c | 25 ++++++++- tools/virsh.pod | 14 +++-- 44 files changed, 213 insertions(+), 77 deletions(-) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/description_only.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-invalid.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-network-seclabel-invalid.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-seclabel.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk_driver_name_null.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk_snapshot.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/empty.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/external_vm.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/name_and_description.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/name_only.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/noparent.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/all_parameters.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk-seclabel.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_driver_name_null.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_snapshot.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_snapshot_redefine.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/empty.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/external_vm.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/external_vm_redefine.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/full_domain.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/metadata.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/name_and_description.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent_nodescription.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent_nodescription_noactive.xml (100%) rename tests/{domainsnapshotxml2xmltest.c => qemudomainsnapshotxml2xmltest.c} (91%) -- 2.20.1
Make it obvious that the domainsnapshotxml2xml test is only run when compiling in support for qemu. Suggested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/Makefile.am | 14 +++++++------- .../description_only.xml | 0 .../disk-invalid.xml | 0 .../disk-network-seclabel-invalid.xml | 0 .../disk-seclabel.xml | 0 .../disk_driver_name_null.xml | 0 .../disk_snapshot.xml | 0 .../empty.xml | 0 .../external_vm.xml | 0 .../name_and_description.xml | 0 .../name_only.xml | 0 .../noparent.xml | 0 .../all_parameters.xml | 0 .../disk-seclabel.xml | 0 .../disk_driver_name_null.xml | 0 .../disk_snapshot.xml | 0 .../disk_snapshot_redefine.xml | 0 .../empty.xml | 0 .../external_vm.xml | 0 .../external_vm_redefine.xml | 0 .../full_domain.xml | 0 .../metadata.xml | 0 .../name_and_description.xml | 0 .../noparent.xml | 0 .../noparent_nodescription.xml | 0 .../noparent_nodescription_noactive.xml | 0 ...l2xmltest.c => qemudomainsnapshotxml2xmltest.c} | 12 ++++++------ tests/virschematest.c | 4 ++-- 28 files changed, 15 insertions(+), 15 deletions(-) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/description_only.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-invalid.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-network-seclabel-invalid.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk-seclabel.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk_driver_name_null.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/disk_snapshot.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/empty.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/external_vm.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/name_and_description.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/name_only.xml (100%) rename tests/{domainsnapshotxml2xmlin => qemudomainsnapshotxml2xmlin}/noparent.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/all_parameters.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk-seclabel.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_driver_name_null.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_snapshot.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/disk_snapshot_redefine.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/empty.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/external_vm.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/external_vm_redefine.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/full_domain.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/metadata.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/name_and_description.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent_nodescription.xml (100%) rename tests/{domainsnapshotxml2xmlout => qemudomainsnapshotxml2xmlout}/noparent_nodescription_noactive.xml (100%) rename tests/{domainsnapshotxml2xmltest.c => qemudomainsnapshotxml2xmltest.c} (93%) diff --git a/tests/Makefile.am b/tests/Makefile.am index 115afa1c1a..e57d0da58a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -85,8 +85,6 @@ EXTRA_DIST = \ domaincapsschemadata \ domainconfdata \ domainschemadata \ - domainsnapshotxml2xmlin \ - domainsnapshotxml2xmlout \ fchostdata \ genericxml2xmlindata \ genericxml2xmloutdata \ @@ -112,6 +110,8 @@ EXTRA_DIST = \ qemublocktestdata \ qemucapabilitiesdata \ qemucaps2xmloutdata \ + qemudomainsnapshotxml2xmlin \ + qemudomainsnapshotxml2xmlout \ qemuhotplugtestcpus \ qemuhotplugtestdevices \ qemuhotplugtestdomains \ @@ -275,7 +275,7 @@ endif WITH_LIBXL if WITH_QEMU test_programs += qemuxml2argvtest qemuxml2xmltest \ - domainsnapshotxml2xmltest \ + qemudomainsnapshotxml2xmltest \ qemumonitorjsontest qemuhotplugtest \ qemuagenttest qemucapabilitiestest qemucaps2xmltest \ qemumemlocktest \ @@ -656,10 +656,10 @@ qemublocktest_LDADD = \ $(qemu_LDADDS) \ $(NULL) -domainsnapshotxml2xmltest_SOURCES = \ - domainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \ +qemudomainsnapshotxml2xmltest_SOURCES = \ + qemudomainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h -domainsnapshotxml2xmltest_LDADD = $(qemu_LDADDS) +qemudomainsnapshotxml2xmltest_LDADD = $(qemu_LDADDS) qemumemlocktest_SOURCES = \ qemumemlocktest.c \ @@ -691,7 +691,7 @@ qemufirmwaretest_LDADD = $(qemu_LDADDS) else ! WITH_QEMU EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c \ - domainsnapshotxml2xmltest.c \ + qemudomainsnapshotxml2xmltest.c \ testutilsqemu.c testutilsqemu.h \ testutilsqemuschema.c testutilsqemuschema.h \ qemumonitorjsontest.c qemuhotplugtest.c \ diff --git a/tests/domainsnapshotxml2xmlin/description_only.xml b/tests/qemudomainsnapshotxml2xmlin/description_only.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/description_only.xml rename to tests/qemudomainsnapshotxml2xmlin/description_only.xml diff --git a/tests/domainsnapshotxml2xmlin/disk-invalid.xml b/tests/qemudomainsnapshotxml2xmlin/disk-invalid.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/disk-invalid.xml rename to tests/qemudomainsnapshotxml2xmlin/disk-invalid.xml diff --git a/tests/domainsnapshotxml2xmlin/disk-network-seclabel-invalid.xml b/tests/qemudomainsnapshotxml2xmlin/disk-network-seclabel-invalid.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/disk-network-seclabel-invalid.xml rename to tests/qemudomainsnapshotxml2xmlin/disk-network-seclabel-invalid.xml diff --git a/tests/domainsnapshotxml2xmlin/disk-seclabel.xml b/tests/qemudomainsnapshotxml2xmlin/disk-seclabel.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/disk-seclabel.xml rename to tests/qemudomainsnapshotxml2xmlin/disk-seclabel.xml diff --git a/tests/domainsnapshotxml2xmlin/disk_driver_name_null.xml b/tests/qemudomainsnapshotxml2xmlin/disk_driver_name_null.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/disk_driver_name_null.xml rename to tests/qemudomainsnapshotxml2xmlin/disk_driver_name_null.xml diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/disk_snapshot.xml rename to tests/qemudomainsnapshotxml2xmlin/disk_snapshot.xml diff --git a/tests/domainsnapshotxml2xmlin/empty.xml b/tests/qemudomainsnapshotxml2xmlin/empty.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/empty.xml rename to tests/qemudomainsnapshotxml2xmlin/empty.xml diff --git a/tests/domainsnapshotxml2xmlin/external_vm.xml b/tests/qemudomainsnapshotxml2xmlin/external_vm.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/external_vm.xml rename to tests/qemudomainsnapshotxml2xmlin/external_vm.xml diff --git a/tests/domainsnapshotxml2xmlin/name_and_description.xml b/tests/qemudomainsnapshotxml2xmlin/name_and_description.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/name_and_description.xml rename to tests/qemudomainsnapshotxml2xmlin/name_and_description.xml diff --git a/tests/domainsnapshotxml2xmlin/name_only.xml b/tests/qemudomainsnapshotxml2xmlin/name_only.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/name_only.xml rename to tests/qemudomainsnapshotxml2xmlin/name_only.xml diff --git a/tests/domainsnapshotxml2xmlin/noparent.xml b/tests/qemudomainsnapshotxml2xmlin/noparent.xml similarity index 100% rename from tests/domainsnapshotxml2xmlin/noparent.xml rename to tests/qemudomainsnapshotxml2xmlin/noparent.xml diff --git a/tests/domainsnapshotxml2xmlout/all_parameters.xml b/tests/qemudomainsnapshotxml2xmlout/all_parameters.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/all_parameters.xml rename to tests/qemudomainsnapshotxml2xmlout/all_parameters.xml diff --git a/tests/domainsnapshotxml2xmlout/disk-seclabel.xml b/tests/qemudomainsnapshotxml2xmlout/disk-seclabel.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/disk-seclabel.xml rename to tests/qemudomainsnapshotxml2xmlout/disk-seclabel.xml diff --git a/tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml b/tests/qemudomainsnapshotxml2xmlout/disk_driver_name_null.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/disk_driver_name_null.xml rename to tests/qemudomainsnapshotxml2xmlout/disk_driver_name_null.xml diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/disk_snapshot.xml rename to tests/qemudomainsnapshotxml2xmlout/disk_snapshot.xml diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot_redefine.xml b/tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/disk_snapshot_redefine.xml rename to tests/qemudomainsnapshotxml2xmlout/disk_snapshot_redefine.xml diff --git a/tests/domainsnapshotxml2xmlout/empty.xml b/tests/qemudomainsnapshotxml2xmlout/empty.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/empty.xml rename to tests/qemudomainsnapshotxml2xmlout/empty.xml diff --git a/tests/domainsnapshotxml2xmlout/external_vm.xml b/tests/qemudomainsnapshotxml2xmlout/external_vm.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/external_vm.xml rename to tests/qemudomainsnapshotxml2xmlout/external_vm.xml diff --git a/tests/domainsnapshotxml2xmlout/external_vm_redefine.xml b/tests/qemudomainsnapshotxml2xmlout/external_vm_redefine.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/external_vm_redefine.xml rename to tests/qemudomainsnapshotxml2xmlout/external_vm_redefine.xml diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/qemudomainsnapshotxml2xmlout/full_domain.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/full_domain.xml rename to tests/qemudomainsnapshotxml2xmlout/full_domain.xml diff --git a/tests/domainsnapshotxml2xmlout/metadata.xml b/tests/qemudomainsnapshotxml2xmlout/metadata.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/metadata.xml rename to tests/qemudomainsnapshotxml2xmlout/metadata.xml diff --git a/tests/domainsnapshotxml2xmlout/name_and_description.xml b/tests/qemudomainsnapshotxml2xmlout/name_and_description.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/name_and_description.xml rename to tests/qemudomainsnapshotxml2xmlout/name_and_description.xml diff --git a/tests/domainsnapshotxml2xmlout/noparent.xml b/tests/qemudomainsnapshotxml2xmlout/noparent.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/noparent.xml rename to tests/qemudomainsnapshotxml2xmlout/noparent.xml diff --git a/tests/domainsnapshotxml2xmlout/noparent_nodescription.xml b/tests/qemudomainsnapshotxml2xmlout/noparent_nodescription.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/noparent_nodescription.xml rename to tests/qemudomainsnapshotxml2xmlout/noparent_nodescription.xml diff --git a/tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml b/tests/qemudomainsnapshotxml2xmlout/noparent_nodescription_noactive.xml similarity index 100% rename from tests/domainsnapshotxml2xmlout/noparent_nodescription_noactive.xml rename to tests/qemudomainsnapshotxml2xmlout/noparent_nodescription_noactive.xml diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/qemudomainsnapshotxml2xmltest.c similarity index 93% rename from tests/domainsnapshotxml2xmltest.c rename to tests/qemudomainsnapshotxml2xmltest.c index c34ab0bc89..10ea9cf8d2 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/qemudomainsnapshotxml2xmltest.c @@ -144,18 +144,18 @@ mymain(void) } while (0) # define DO_TEST_IN(name, uuid) DO_TEST("in->in", name, \ - "domainsnapshotxml2xmlin", \ - "domainsnapshotxml2xmlin", \ + "qemudomainsnapshotxml2xmlin", \ + "qemudomainsnapshotxml2xmlin", \ uuid, 0, 0) # define DO_TEST_OUT(name, uuid, internal) \ - DO_TEST("out->out", name, "domainsnapshotxml2xmlout", \ - "domainsnapshotxml2xmlout", uuid, 0, internal | TEST_REDEFINE) + DO_TEST("out->out", name, "qemudomainsnapshotxml2xmlout", \ + "qemudomainsnapshotxml2xmlout", uuid, 0, internal | TEST_REDEFINE) # define DO_TEST_INOUT(name, uuid, time, flags) \ DO_TEST("in->out", name, \ - "domainsnapshotxml2xmlin",\ - "domainsnapshotxml2xmlout",\ + "qemudomainsnapshotxml2xmlin",\ + "qemudomainsnapshotxml2xmlout",\ uuid, time, flags) /* Unset or set all envvars here that are copied in qemudBuildCommandLine diff --git a/tests/virschematest.c b/tests/virschematest.c index ff25f7781e..b7c2f7cfaa 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -222,8 +222,8 @@ mymain(void) "genericxml2xmloutdata", "xlconfigdata", "libxlxml2domconfigdata", "qemuhotplugtestdomains"); DO_TEST_DIR("domaincaps.rng", "domaincapsschemadata"); - DO_TEST_DIR("domainsnapshot.rng", "domainsnapshotxml2xmlin", - "domainsnapshotxml2xmlout"); + DO_TEST_DIR("domainsnapshot.rng", "qemudomainsnapshotxml2xmlin", + "qemudomainsnapshotxml2xmlout"); DO_TEST_DIR("interface.rng", "interfaceschemadata"); DO_TEST_DIR("network.rng", "../src/network", "networkxml2xmlin", "networkxml2xmlout", "networkxml2confdata"); -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:28 -0500, Eric Blake wrote:
Make it obvious that the domainsnapshotxml2xml test is only run when compiling in support for qemu.
Suggested-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
The docs talked about an active snapshot when they meant an active domain; they also claimed the flag was a no-op for hypervisors with no snapshot metadata even though the flag is rejected as unrecognized for hypervisors with no snapshot support at all. Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt-domain.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index e2594a3392..3d12e7c125 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -6270,10 +6270,11 @@ virDomainUndefine(virDomainPtr domain) * virDomainSnapshotNum()), then including * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove * that metadata. Omitting the flag will cause the undefine of an - * inactive domain to fail. Active snapshots will retain snapshot - * metadata until the (now-transient) domain halts, regardless of - * whether this flag is present. On hypervisors where snapshots do - * not use libvirt metadata, this flag has no effect. + * inactive domain with snapshots to fail. Active domains will retain + * snapshot metadata until the (now-transient) domain halts, + * regardless of whether this flag is present. On hypervisors that + * support snapshots, but where snapshots do not use libvirt metadata, + * this flag has no effect. * * If the domain has any nvram specified, the undefine process will fail * unless VIR_DOMAIN_UNDEFINE_KEEP_NVRAM is specified, or if -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:29 -0500, Eric Blake wrote:
The docs talked about an active snapshot when they meant an active domain; they also claimed the flag was a no-op for hypervisors with no snapshot metadata even though the flag is rejected as unrecognized for hypervisors with no snapshot support at all.
Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt-domain.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
I'd probably go for the trivial addition of the flag to all of the undefine APIs since that does not require clients from encoding the knowledge whether the given hypervisor supports snapshots at all. This works too though as we'd reject the flag at this point anyways. ACK
On 7/8/19 2:40 AM, Peter Krempa wrote:
On Fri, Jul 05, 2019 at 23:37:29 -0500, Eric Blake wrote:
The docs talked about an active snapshot when they meant an active domain; they also claimed the flag was a no-op for hypervisors with no snapshot metadata even though the flag is rejected as unrecognized for hypervisors with no snapshot support at all.
Reported-by: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt-domain.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
I'd probably go for the trivial addition of the flag to all of the undefine APIs since that does not require clients from encoding the knowledge whether the given hypervisor supports snapshots at all.
I can do that as well. But as you observed, if you have a new virsh talking to an old libvirtd, it doesn't help, and I don't see a problem with the current documentation wording even if other drivers (silently) accept and ignore the flag.
This works too though as we'd reject the flag at this point anyways.
ACK
I'll post a followup mail for review on adding no-op support in the remaining domains. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Similar to VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; for now used only by the testsuite, but will be exposed in the public API in the next patch. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 1 + src/conf/snapshot_conf.c | 13 +++++++++++++ tests/qemudomainsnapshotxml2xmltest.c | 3 ++- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 08b2a3b955..a0c87e7ade 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -92,6 +92,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE = 1 << 4, } virDomainSnapshotParseFlags; typedef enum { diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c7f29360e7..daea6c616d 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -25,6 +25,7 @@ #include <sys/stat.h> #include <unistd.h> +#include "configmake.h" #include "internal.h" #include "virbitmap.h" #include "virbuffer.h" @@ -409,6 +410,18 @@ virDomainSnapshotDefParseNode(xmlDocPtr xml, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE) { + VIR_AUTOFREE(char *) schema = NULL; + + schema = virFileFindResource("domainsnapshot.rng", + abs_top_srcdir "/docs/schemas", + PKGDATADIR "/schemas"); + if (!schema) + return NULL; + if (virXMLValidateAgainstSchema(schema, xml) < 0) + return NULL; + } + ctxt = xmlXPathNewContext(xml); if (ctxt == NULL) { virReportOOMError(); diff --git a/tests/qemudomainsnapshotxml2xmltest.c b/tests/qemudomainsnapshotxml2xmltest.c index 10ea9cf8d2..55cb8575f7 100644 --- a/tests/qemudomainsnapshotxml2xmltest.c +++ b/tests/qemudomainsnapshotxml2xmltest.c @@ -35,7 +35,8 @@ testCompareXMLToXMLFiles(const char *inxml, char *outXmlData = NULL; char *actual = NULL; int ret = -1; - unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parseflags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE); unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; bool cur = false; VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL; -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:30 -0500, Eric Blake wrote:
Similar to VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA; for now used only by the testsuite, but will be exposed in the public API in the next patch.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/snapshot_conf.h | 1 + src/conf/snapshot_conf.c | 13 +++++++++++++ tests/qemudomainsnapshotxml2xmltest.c | 3 ++- 3 files changed, 16 insertions(+), 1 deletion(-)
[...]
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c7f29360e7..daea6c616d 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c
[....]
@@ -409,6 +410,18 @@ virDomainSnapshotDefParseNode(xmlDocPtr xml, goto cleanup;
^^^
}
+ if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE) { + VIR_AUTOFREE(char *) schema = NULL; + + schema = virFileFindResource("domainsnapshot.rng", + abs_top_srcdir "/docs/schemas", + PKGDATADIR "/schemas"); + if (!schema) + return NULL;
This would skip the cleanup section while the above block does not. While this is not a problem code wise, it's semantically wrong.
+ if (virXMLValidateAgainstSchema(schema, xml) < 0) + return NULL; + } + ctxt = xmlXPathNewContext(xml); if (ctxt == NULL) { virReportOOMError(); diff --git a/tests/qemudomainsnapshotxml2xmltest.c b/tests/qemudomainsnapshotxml2xmltest.c index 10ea9cf8d2..55cb8575f7 100644 --- a/tests/qemudomainsnapshotxml2xmltest.c +++ b/tests/qemudomainsnapshotxml2xmltest.c @@ -35,7 +35,8 @@ testCompareXMLToXMLFiles(const char *inxml, char *outXmlData = NULL; char *actual = NULL; int ret = -1; - unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parseflags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE);
We schema-check all the snapshot examples explitly and additionally this does prevent us from ever testing upgrade from invalid snapshot XML. ACK if you use "goto cleanup" in the implementation and drop the test change.
We've been doing a terrible job of performing XML validation in our various API that parse XML with a corresponding schema (we started with domains back in commit dd69a14f, v1.2.12, but didn't catch all domain-related APIs, and didn't cover other XMLM). New APIs (like checkpoints) should do the validation unconditionally, but it doesn't hurt to retrofit existing APIs to at least allow the option. Wire up a new snapshot XML creation flag through all the hypervisors that support snapshots, as well as exposing it in 'virsh snapshot-create'. For 'virsh snapshot-create-as', we blindly set the flag without a command-line option, since the XML we create from the command line should always comply, but we have to add in code to disable validation if the server is too old to understand the flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 2 ++ src/libvirt-domain-snapshot.c | 3 +++ src/qemu/qemu_driver.c | 6 +++++- src/test/test_driver.c | 6 +++++- src/vbox/vbox_common.c | 11 ++++++++--- src/vz/vz_driver.c | 5 ++++- tests/virsh-snapshot | 6 +++--- tools/virsh-snapshot.c | 15 ++++++++++++++- tools/virsh.pod | 7 +++++-- 9 files changed, 49 insertions(+), 12 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 602e5def59..90673ed0fb 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -71,6 +71,8 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot while the guest is running */ + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE = (1 << 9), /* validate the XML + against the schema */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 0c8023d9f6..2687a34b96 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -115,6 +115,9 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * becomes current (see virDomainSnapshotCurrent()), and is a child * of any previous current snapshot. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, then @xmlDesc + * must validate against the <domainsnapshot> XML schema. + * * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, then this * is a request to reinstate snapshot metadata that was previously * captured from virDomainSnapshotGetXMLDesc() before removing that diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c05ab4ad1..97f3d7f786 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15508,7 +15508,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, @@ -15549,6 +15550,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, !virDomainObjIsActive(vm)) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; + if (!(def = virDomainSnapshotDefParseString(xmlDesc, caps, driver->xmlopt, NULL, parse_flags))) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7dd448bb20..e7ad4dbbd7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -7163,7 +7163,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_CREATE_HALT | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC | - VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_LIVE | + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))) update_current = false; @@ -7179,6 +7180,9 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; + if (!(def = virDomainSnapshotDefParseString(xmlDesc, privconn->caps, privconn->xmlopt, diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e31bec9d..8a912da50c 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5487,6 +5487,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, nsresult rc; resultCodeUnion result; virDomainSnapshotPtr ret = NULL; + unsigned int parse_flags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL; if (!data->vboxObj) @@ -5496,12 +5498,15 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | - VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps, data->xmlopt, NULL, - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | - VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE))) + parse_flags))) goto cleanup; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2286f9a04f..50c883feca 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2586,7 +2586,7 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, bool job = false; VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL; - virCheckFlags(0, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); if (!(dom = vzDomObjFromDomain(domain))) return NULL; @@ -2594,6 +2594,9 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, dom->def, flags) < 0) goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; + if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, driver->xmlopt, NULL, parse_flags))) diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot index cb498cf54e..8eab67c9e0 100755 --- a/tests/virsh-snapshot +++ b/tests/virsh-snapshot @@ -180,11 +180,11 @@ compare exp err || fail=1 # Restore state with redefine $abs_top_builddir/tools/virsh -c test:///default >out 2>err <<EOF || fail=1 # Redefine must be in topological order; this will fail - snapshot-create test --redefine s2.xml + snapshot-create test --redefine s2.xml --validate echo --err marker # This is the right order - snapshot-create test --redefine s3.xml - snapshot-create test --redefine s2.xml --current + snapshot-create test --redefine s3.xml --validate + snapshot-create test --redefine s2.xml --current --validate snapshot-info test --current EOF diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e9f0ee0810..f7772ce549 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -50,6 +50,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + /* If no source file but validate was not recognized, try again without + * that flag. */ + if (!snapshot && last_error->code == VIR_ERR_NO_SUPPORT && !from) { + flags &= ~VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE; + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + } + /* Emulate --halt on older servers. */ if (!snapshot && last_error->code == VIR_ERR_INVALID_ARG && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { @@ -147,6 +154,10 @@ static const vshCmdOptDef opts_snapshot_create[] = { .help = N_("require atomic operation") }, VIRSH_COMMON_OPT_LIVE(N_("take a live snapshot")), + {.name = "validate", + .type = VSH_OT_BOOL, + .help = N_("validate the XML against the schema"), + }, {.name = NULL} }; @@ -177,6 +188,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "validate")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -366,7 +379,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *desc = NULL; const char *memspec = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - unsigned int flags = 0; + unsigned int flags = VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE; const vshCmdOpt *opt = NULL; if (vshCommandOptBool(cmd, "no-metadata")) diff --git a/tools/virsh.pod b/tools/virsh.pod index dc39004a66..865fb2b0da 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4588,10 +4588,13 @@ used to represent properties of snapshots. =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] -[I<--quiesce>] [I<--atomic>] [I<--live>]} +[I<--quiesce>] [I<--atomic>] [I<--live>]} [I<--validate>] Create a snapshot for domain I<domain> with the properties specified in -I<xmlfile>. Normally, the only properties settable for a domain snapshot +I<xmlfile>. Optionally, the I<--validate> option can be passed to +validate the format of the input XML file against an internal RNG +schema (identical to using L<virt-xml-validate(1)> tool). Normally, +the only properties settable for a domain snapshot are the <name> and <description> elements, as well as <disks> if I<--disk-only> is given; the rest of the fields are ignored, and automatically filled in by libvirt. If I<xmlfile> is -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:31 -0500, Eric Blake wrote:
We've been doing a terrible job of performing XML validation in our various API that parse XML with a corresponding schema (we started with domains back in commit dd69a14f, v1.2.12, but didn't catch all domain-related APIs, and didn't cover other XMLM). New APIs (like
s/XMLM/XMLs/ ?
checkpoints) should do the validation unconditionally, but it doesn't hurt to retrofit existing APIs to at least allow the option. Wire up a new snapshot XML creation flag through all the hypervisors that support snapshots, as well as exposing it in 'virsh snapshot-create'. For 'virsh snapshot-create-as', we blindly set the flag without a command-line option, since the XML we create from the command line should always comply, but we have to add in code to disable validation if the server is too old to understand the flag.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 2 ++ src/libvirt-domain-snapshot.c | 3 +++ src/qemu/qemu_driver.c | 6 +++++- src/test/test_driver.c | 6 +++++- src/vbox/vbox_common.c | 11 ++++++++--- src/vz/vz_driver.c | 5 ++++-
The 'esx' driver also implements 'domainSnapshotCreateXML' as esxDomainSnapshotCreateXML
tests/virsh-snapshot | 6 +++--- tools/virsh-snapshot.c | 15 ++++++++++++++- tools/virsh.pod | 7 +++++-- 9 files changed, 49 insertions(+), 12 deletions(-)
[...]
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 0c8023d9f6..2687a34b96 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -115,6 +115,9 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * becomes current (see virDomainSnapshotCurrent()), and is a child * of any previous current snapshot. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, then @xmlDesc + * must validate against the <domainsnapshot> XML schema.
s/must validate/is validated/ ?
* If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, then this * is a request to reinstate snapshot metadata that was previously * captured from virDomainSnapshotGetXMLDesc() before removing that
[...]
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e31bec9d..8a912da50c 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5487,6 +5487,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, nsresult rc; resultCodeUnion result; virDomainSnapshotPtr ret = NULL; + unsigned int parse_flags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE);
Parentheses unnecessary.
VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL;
if (!data->vboxObj)
[...]
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index e9f0ee0810..f7772ce549 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -50,6 +50,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer,
snapshot = virDomainSnapshotCreateXML(dom, buffer, flags);
+ /* If no source file but validate was not recognized, try again without + * that flag. */ + if (!snapshot && last_error->code == VIR_ERR_NO_SUPPORT && !from) { + flags &= ~VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE; + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + }
I think this compatibility glue is unnecessary ...
+ /* Emulate --halt on older servers. */ if (!snapshot && last_error->code == VIR_ERR_INVALID_ARG && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
[...]
@@ -177,6 +188,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "validate")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -366,7 +379,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *desc = NULL; const char *memspec = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - unsigned int flags = 0; + unsigned int flags = VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
... just to validate something we always generated ourselves. ACK if you remove the use of the flag in cmdSnapshotCreateAs. Other are at your discretion.
On 7/8/19 2:56 AM, Peter Krempa wrote:
On Fri, Jul 05, 2019 at 23:37:31 -0500, Eric Blake wrote:
We've been doing a terrible job of performing XML validation in our various API that parse XML with a corresponding schema (we started with domains back in commit dd69a14f, v1.2.12, but didn't catch all domain-related APIs, and didn't cover other XMLM). New APIs (like
s/XMLM/XMLs/ ?
Yes. Not sure how I let that one through, but I also spotted it locally before your mail.
The 'esx' driver also implements 'domainSnapshotCreateXML' as esxDomainSnapshotCreateXML
Fixed on my tree: diff --git i/src/esx/esx_driver.c w/src/esx/esx_driver.c index 47d95abd6d..9173896fe1 100644 --- i/src/esx/esx_driver.c +++ w/src/esx/esx_driver.c @@ -4101,18 +4101,23 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, bool diskOnly = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) != 0; bool quiesce = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) != 0; VIR_AUTOUNREF(virDomainSnapshotDefPtr) def = NULL; + unsigned int parse_flags = 0; /* ESX supports disk-only and quiesced snapshots; libvirt tracks no * snapshot metadata so supporting that flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY | VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE | - VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, NULL); + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE) + format_flags = VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE; if (esxVI_EnsureSession(priv->primary) < 0) return NULL; def = virDomainSnapshotDefParseString(xmlDesc, priv->caps, - priv->xmlopt, NULL, 0); + priv->xmlopt, NULL, parse_flags); if (!def) return NULL;
+++ b/src/libvirt-domain-snapshot.c @@ -115,6 +115,9 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * becomes current (see virDomainSnapshotCurrent()), and is a child * of any previous current snapshot. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE, then @xmlDesc + * must validate against the <domainsnapshot> XML schema.
s/must validate/is validated/ ?
Sure. Oddly enough, we do NOT document the VIR_DOMAIN_DEFINE_VALIDATE flag and friends; I guess I'll add another patch to my queue to rectify that. (Uggh, this pile of yak shavings at my desk is growing taller...)
+++ b/src/vbox/vbox_common.c @@ -5487,6 +5487,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, nsresult rc; resultCodeUnion result; virDomainSnapshotPtr ret = NULL; + unsigned int parse_flags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE);
Parentheses unnecessary.
But compliant with the syntax-check, and allows for nicer indentation of the second line. Qemu just recently had a patch related to that: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01726.html
+++ b/tools/virsh-snapshot.c @@ -50,6 +50,13 @@ virshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer,
snapshot = virDomainSnapshotCreateXML(dom, buffer, flags);
+ /* If no source file but validate was not recognized, try again without + * that flag. */ + if (!snapshot && last_error->code == VIR_ERR_NO_SUPPORT && !from) { + flags &= ~VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE; + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + }
I think this compatibility glue is unnecessary ...
It's necessary if snapshot-create-as uses the flag...
@@ -366,7 +379,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *desc = NULL; const char *memspec = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - unsigned int flags = 0; + unsigned int flags = VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
... just to validate something we always generated ourselves.
...but I can drop the use here, if you think we are safe.
ACK if you remove the use of the flag in cmdSnapshotCreateAs. Other are at your discretion.
Okay, will drop the use in snapshot-create-as. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Jul 08, 2019 at 22:12:38 -0500, Eric Blake wrote:
On 7/8/19 2:56 AM, Peter Krempa wrote:
On Fri, Jul 05, 2019 at 23:37:31 -0500, Eric Blake wrote:
We've been doing a terrible job of performing XML validation in our various API that parse XML with a corresponding schema (we started with domains back in commit dd69a14f, v1.2.12, but didn't catch all domain-related APIs, and didn't cover other XMLM). New APIs (like
[...]
@@ -366,7 +379,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *desc = NULL; const char *memspec = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; - unsigned int flags = 0; + unsigned int flags = VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE;
... just to validate something we always generated ourselves.
...but I can drop the use here, if you think we are safe.
ACK if you remove the use of the flag in cmdSnapshotCreateAs. Other are at your discretion.
Hmm, on a second thought, the XML is created from user-provided bits which may be validated insufficiently, so if you didn't follow through on this one you can use my ACK even with the compat glue and explicit validation.
The code to check whether a redefined snapshot/checkpoint XML is attempting to create a cycle in the list of moments is lengthy, and common between the two types of list. Therefore, it belongs in the shared base file. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/virdomainmomentobjlist.h | 3 +++ src/conf/virdomainsnapshotobjlist.h | 3 +++ src/conf/snapshot_conf.c | 41 ++++------------------------- src/conf/virdomainmomentobjlist.c | 40 ++++++++++++++++++++++++++++ src/conf/virdomainsnapshotobjlist.c | 9 +++++++ tests/virsh-snapshot | 2 +- 6 files changed, 61 insertions(+), 37 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index 68f00a114f..4067e928f4 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -117,3 +117,6 @@ int virDomainMomentForEach(virDomainMomentObjListPtr moments, virHashIterator iter, void *data); int virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments); +int virDomainMomentCheckCycles(virDomainMomentObjListPtr list, + virDomainMomentDefPtr def, + const char *domname); diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h index 26fa456730..fed8d22bc8 100644 --- a/src/conf/virdomainsnapshotobjlist.h +++ b/src/conf/virdomainsnapshotobjlist.h @@ -52,6 +52,9 @@ int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, virHashIterator iter, void *data); int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); +int virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotDefPtr def, + const char *domname); #define VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA \ (VIR_DOMAIN_SNAPSHOT_LIST_METADATA | \ diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index daea6c616d..3521177f0b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -966,46 +966,15 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, { virDomainSnapshotDefPtr def = *defptr; virDomainMomentObjPtr other; - virDomainSnapshotDefPtr otherdef; + virDomainSnapshotDefPtr otherdef = NULL; bool check_if_stolen; - /* Prevent circular chains */ - if (def->parent.parent_name) { - if (STREQ(def->parent.name, def->parent.parent_name)) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot set snapshot %s as its own parent"), - def->parent.name); - return -1; - } - other = virDomainSnapshotFindByName(vm->snapshots, - def->parent.parent_name); - if (!other) { - virReportError(VIR_ERR_INVALID_ARG, - _("parent %s for snapshot %s not found"), - def->parent.parent_name, def->parent.name); - return -1; - } - otherdef = virDomainSnapshotObjGetDef(other); - while (otherdef->parent.parent_name) { - if (STREQ(otherdef->parent.parent_name, def->parent.name)) { - virReportError(VIR_ERR_INVALID_ARG, - _("parent %s would create cycle to %s"), - otherdef->parent.name, def->parent.name); - return -1; - } - other = virDomainSnapshotFindByName(vm->snapshots, - otherdef->parent.parent_name); - if (!other) { - VIR_WARN("snapshots are inconsistent for %s", - vm->def->name); - break; - } - otherdef = virDomainSnapshotObjGetDef(other); - } - } + if (virDomainSnapshotCheckCycles(vm->snapshots, def, vm->def->name) < 0) + return -1; other = virDomainSnapshotFindByName(vm->snapshots, def->parent.name); - otherdef = other ? virDomainSnapshotObjGetDef(other) : NULL; + if (other) + otherdef = virDomainSnapshotObjGetDef(other); check_if_stolen = other && otherdef->parent.dom; if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt, flags) < 0) { diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index f56b516343..0ea5e9af80 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -519,3 +519,43 @@ virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments) moments->current = NULL; return act.err; } + + +int +virDomainMomentCheckCycles(virDomainMomentObjListPtr list, + virDomainMomentDefPtr def, + const char *domname) +{ + virDomainMomentObjPtr other; + + if (def->parent_name) { + if (STREQ(def->name, def->parent_name)) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot set moment %s as its own parent"), + def->name); + return -1; + } + other = virDomainMomentFindByName(list, def->parent_name); + if (!other) { + virReportError(VIR_ERR_INVALID_ARG, + _("parent %s for moment %s not found"), + def->parent_name, def->name); + return -1; + } + while (other->def->parent_name) { + if (STREQ(other->def->parent_name, def->name)) { + virReportError(VIR_ERR_INVALID_ARG, + _("parent %s would create cycle to %s"), + other->def->name, def->name); + return -1; + } + other = virDomainMomentFindByName(list, other->def->parent_name); + if (!other) { + VIR_WARN("moments are inconsistent for domain %s", + domname); + break; + } + } + } + return 0; +} diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c index 9bcc8d1036..99bc4bb0c5 100644 --- a/src/conf/virdomainsnapshotobjlist.c +++ b/src/conf/virdomainsnapshotobjlist.c @@ -234,6 +234,15 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) } +int +virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotDefPtr def, + const char *domname) +{ + return virDomainMomentCheckCycles(snapshots->base, &def->parent, domname); +} + + int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainMomentObjPtr from, diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot index 8eab67c9e0..510904f470 100755 --- a/tests/virsh-snapshot +++ b/tests/virsh-snapshot @@ -206,7 +206,7 @@ Metadata: yes EOF cat <<EOF > exp || fail=1 -error: invalid argument: parent s3 for snapshot s2 not found +error: invalid argument: parent s3 for moment s2 not found error: marker EOF compare exp err || fail=1 -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:32 -0500, Eric Blake wrote:
The code to check whether a redefined snapshot/checkpoint XML is attempting to create a cycle in the list of moments is lengthy, and common between the two types of list. Therefore, it belongs in the shared base file.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/virdomainmomentobjlist.h | 3 +++ src/conf/virdomainsnapshotobjlist.h | 3 +++ src/conf/snapshot_conf.c | 41 ++++------------------------- src/conf/virdomainmomentobjlist.c | 40 ++++++++++++++++++++++++++++ src/conf/virdomainsnapshotobjlist.c | 9 +++++++ tests/virsh-snapshot | 2 +- 6 files changed, 61 insertions(+), 37 deletions(-)
ACK, although it would be great if the new function would also document what it's doing.
Right now, the snapshot API permits at most one current snapshot, and includes specific API for getting at that snapshot (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent, virDomainSnapshotIsCurrent). However, with upcoming checkpoints, it is conceivable that a hypervisor could mark multiple checkpoints as current (the qemu implementation has only one current checkpoint for an incremental backup, and computes the set of changes for a differential backup by merging a chain of checkpoints - but other hypervisors may treat all differential checkpoints as current). As such, it is more desirable to avoid explicit API for getting at the one current checkpoint, and instead have the List API include a filter for that purpose. Still, it is easier to implement that filter directly in the common virDomainMomentObjList code, since that is the only code that tracks the one moment that is current (both for existing snapshots, and for how qemu will be using current checkpoints). Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/virdomainmomentobjlist.h | 11 +++++++++-- src/conf/virdomainmomentobjlist.c | 9 ++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index 4067e928f4..7628df5e29 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -78,8 +78,10 @@ typedef enum { VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1), VIR_DOMAIN_MOMENT_LIST_LEAVES = (1 << 2), VIR_DOMAIN_MOMENT_LIST_NO_LEAVES = (1 << 3), - VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 4), - VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_CURRENT = (1 << 4), + VIR_DOMAIN_MOMENT_LIST_NO_CURRENT = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 6), + VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7), } virDomainMomentFilters; #define VIR_DOMAIN_MOMENT_FILTERS_METADATA \ @@ -90,10 +92,15 @@ typedef enum { (VIR_DOMAIN_MOMENT_LIST_LEAVES | \ VIR_DOMAIN_MOMENT_LIST_NO_LEAVES) +#define VIR_DOMAIN_MOMENT_FILTERS_CURRENT \ + (VIR_DOMAIN_MOMENT_LIST_CURRENT | \ + VIR_DOMAIN_MOMENT_LIST_NO_CURRENT) + #define VIR_DOMAIN_MOMENT_FILTERS_ALL \ (VIR_DOMAIN_MOMENT_LIST_ROOTS | \ VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL | \ VIR_DOMAIN_MOMENT_FILTERS_METADATA | \ + VIR_DOMAIN_MOMENT_FILTERS_CURRENT | \ VIR_DOMAIN_MOMENT_FILTERS_LEAVES) int virDomainMomentObjListGetNames(virDomainMomentObjListPtr moments, diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index 0ea5e9af80..55d1db9fb0 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -283,6 +283,7 @@ struct virDomainMomentNameData { unsigned int flags; int count; bool error; + virDomainMomentObjPtr current; /* The current moment, if any */ virDomainMomentObjListFilter filter; unsigned int filter_flags; }; @@ -303,6 +304,11 @@ static int virDomainMomentObjListCopyNames(void *payload, return 0; if ((data->flags & VIR_DOMAIN_MOMENT_LIST_NO_LEAVES) && !obj->nchildren) return 0; + if ((data->flags & VIR_DOMAIN_MOMENT_LIST_CURRENT) && obj != data->current) + return 0; + if ((data->flags & VIR_DOMAIN_MOMENT_LIST_NO_CURRENT) && + obj == data->current) + return 0; if (!data->filter(obj, data->filter_flags)) return 0; @@ -327,7 +333,8 @@ virDomainMomentObjListGetNames(virDomainMomentObjListPtr moments, unsigned int filter_flags) { struct virDomainMomentNameData data = { names, maxnames, flags, 0, - false, filter, filter_flags }; + false, moments->current, + filter, filter_flags }; size_t i; virCheckFlags(VIR_DOMAIN_MOMENT_FILTERS_ALL, -1); -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:33 -0500, Eric Blake wrote:
Right now, the snapshot API permits at most one current snapshot, and includes specific API for getting at that snapshot (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent, virDomainSnapshotIsCurrent). However, with upcoming checkpoints, it is conceivable that a hypervisor could mark multiple checkpoints as
The same happens to snapshots too. Since you can create an "incomplete" snapshot by unselecting some disks and then do a complement snapshot of all the remaining disks then you get to the same situation. For more fun you can create a snapshot which partially overlaps and then neither of the "current" snapshots makes sense any more.
current (the qemu implementation has only one current checkpoint for an incremental backup, and computes the set of changes for a differential backup by merging a chain of checkpoints - but other hypervisors may treat all differential checkpoints as current). As such, it is more desirable to avoid explicit API for getting at the one current checkpoint, and instead have the List API include a filter for that purpose. Still, it is easier to implement that filter directly in the common virDomainMomentObjList code, since that is the only code that tracks the one moment that is current (both for existing snapshots, and for how qemu will be using current checkpoints).
I don't quite understand why we need the notion of the current checkpoint anyways. Technically it's just an implementation detal (I will object to the decisions separately in the patch which implements it ) and all the checkpoints are "current" by getting updates. It even does not make sense to stop recording the differences at all since the changed blocks are not kept separately thus if any of the checkpoints stops being "current" (by stopping updating the bitmap or chain of bitmaps) it becomes totally worthless since you won't be able to reconstruct it. As such I don't understand why we need the notion of the current checkpoint altogether. It didn't clarify for me from all the backs-and-forths when I was commenting about it, but you seem to think it's necessary, but I can't seem to understand why. As long as the justification is other than 'it's for symmetry with snapshots' (which is a bogus justification) I'm willing to go with it, but as such since I don't really undestand why this is necessary I won't be able to maintain such code.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/virdomainmomentobjlist.h | 11 +++++++++-- src/conf/virdomainmomentobjlist.c | 9 ++++++++- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index 4067e928f4..7628df5e29 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -78,8 +78,10 @@ typedef enum { VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1), VIR_DOMAIN_MOMENT_LIST_LEAVES = (1 << 2), VIR_DOMAIN_MOMENT_LIST_NO_LEAVES = (1 << 3), - VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 4), - VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_CURRENT = (1 << 4), + VIR_DOMAIN_MOMENT_LIST_NO_CURRENT = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 6), + VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),
Why the reorder?
} virDomainMomentFilters;
#define VIR_DOMAIN_MOMENT_FILTERS_METADATA \
ACK
On 7/8/19 3:27 AM, Peter Krempa wrote:
On Fri, Jul 05, 2019 at 23:37:33 -0500, Eric Blake wrote:
Right now, the snapshot API permits at most one current snapshot, and includes specific API for getting at that snapshot (virDomainHasCurrentSnapshot, virDomainSnapshotCurrent, virDomainSnapshotIsCurrent). However, with upcoming checkpoints, it is conceivable that a hypervisor could mark multiple checkpoints as
The same happens to snapshots too. Since you can create an "incomplete" snapshot by unselecting some disks and then do a complement snapshot of all the remaining disks then you get to the same situation.
For more fun you can create a snapshot which partially overlaps and then neither of the "current" snapshots makes sense any more.
For now, I'm deferring this patch to later (or maybe even dropping it), based on the v9 review comments about using the one-and-only leaf node snapshot as the default parent at least for the short term where we interlock against having snapshots. If we find that changing a checkpoint current node makes sense with snapshots, we can always add additional filtering at that time, but right now is premature. While it is more churn for me, I agree with the goal of stripping down the initial implementation to the bare minimum needed, and this does not qualify in that regards.
@@ -78,8 +78,10 @@ typedef enum { VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL = (1 << 1), VIR_DOMAIN_MOMENT_LIST_LEAVES = (1 << 2), VIR_DOMAIN_MOMENT_LIST_NO_LEAVES = (1 << 3), - VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 4), - VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_CURRENT = (1 << 4), + VIR_DOMAIN_MOMENT_LIST_NO_CURRENT = (1 << 5), + VIR_DOMAIN_MOMENT_LIST_METADATA = (1 << 6), + VIR_DOMAIN_MOMENT_LIST_NO_METADATA = (1 << 7),
Why the reorder?
Because if we like the idea of filtering on a current node better than the idea of no_metadata, then patches that add current filtering in v9 use bit 6 right away, while bits for metadata filtering of checkpoints sit unused, and it looks odd to have a gap in the numbering. The snapshot code already has to do mapping between internal bits from the public API flags, but my initial goal was to have the internal bits match the checkpoint public bits. That said, since I'm now working on stripping the notion of current filtering out of checkpoints before the initial implementation, the question is moot until (or if) this patch ever gets revived.
} virDomainMomentFilters;
#define VIR_DOMAIN_MOMENT_FILTERS_METADATA \
ACK
Thanks for the review, even if later patches changed our minds. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add a new filter group VIR_DOMAIN_SNAPSHOT_LIST_CURRENT/NO_CURRENT, which restricts the output based on whether a snapshot is the domain's current snapshot. This is redundant with existing API (both virDomainHasCurrentSnapshot and virDomainSnapshotCurrent can be emulated by a single call to virDomainListAllSnapshots, and replacing virDomainSnapshotIsCurrent is also possible but a bit more indirect). However, adding the new filters makes snapshots slightly more consistent with the plans for the upcoming checkpoint API additions to ONLY provide access to the notion of being current via list filters or XML inspection. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 5 +++++ src/conf/virdomainsnapshotobjlist.h | 7 ++++++- src/conf/virdomainsnapshotobjlist.c | 4 ++++ src/libvirt-domain-snapshot.c | 14 ++++++++++++-- 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 90673ed0fb..02cdabafbc 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -141,6 +141,11 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur before children in the resulting list */ + + VIR_DOMAIN_SNAPSHOT_LIST_CURRENT = (1 << 11), /* Filter to just current + snapshot */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT = (1 << 12), /* Filter out current + snapshot */ } virDomainSnapshotListFlags; /* Return the number of snapshots for this domain */ diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h index fed8d22bc8..59a176aaef 100644 --- a/src/conf/virdomainsnapshotobjlist.h +++ b/src/conf/virdomainsnapshotobjlist.h @@ -73,11 +73,16 @@ int virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots, (VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL | \ VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) +#define VIR_DOMAIN_SNAPSHOT_FILTERS_CURRENT \ + (VIR_DOMAIN_SNAPSHOT_LIST_CURRENT | \ + VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT) + #define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES | \ VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | \ - VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION) + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION | \ + VIR_DOMAIN_SNAPSHOT_FILTERS_CURRENT) int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainMomentObjPtr from, diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c index 99bc4bb0c5..cac0834e1f 100644 --- a/src/conf/virdomainsnapshotobjlist.c +++ b/src/conf/virdomainsnapshotobjlist.c @@ -124,6 +124,10 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, VIR_DOMAIN_MOMENT_LIST_LEAVES, }, { VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, VIR_DOMAIN_MOMENT_LIST_NO_LEAVES, }, + { VIR_DOMAIN_SNAPSHOT_LIST_CURRENT, + VIR_DOMAIN_MOMENT_LIST_CURRENT, }, + { VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT, + VIR_DOMAIN_MOMENT_LIST_NO_CURRENT, }, { VIR_DOMAIN_SNAPSHOT_LIST_METADATA, VIR_DOMAIN_MOMENT_LIST_METADATA, }, { VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 2687a34b96..5dd2c5390b 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -465,6 +465,12 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, * whether the snapshot is stored inside the disk images or as * additional files. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_CURRENT and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT, to filter based on whether a + * snapshot is current. This filter provides the same functionality as + * virDomainHasCurrentSnapshot(), virDomainSnapshotCurrent(), and + * virDomainSnapshotIsCurrent(). + * * Returns the number of domain snapshots found or -1 and sets @snaps to * NULL in case of error. On success, the array stored into @snaps is * guaranteed to have an extra allocated element set to NULL but not included @@ -736,7 +742,9 @@ virDomainSnapshotLookupByName(virDomainPtr domain, * @domain: pointer to the domain object * @flags: extra flags; not used yet, so callers should always pass 0 * - * Determine if the domain has a current snapshot. + * Determine if the domain has a current snapshot. This can also be + * determined by using virDomainListAllSnapshots() with the + * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter. * * Returns 1 if such snapshot exists, 0 if it doesn't, -1 on error. */ @@ -771,7 +779,9 @@ virDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) * @domain: a domain object * @flags: extra flags; not used yet, so callers should always pass 0 * - * Get the current snapshot for a domain, if any. + * Get the current snapshot for a domain, if any. This can also be + * determined by using virDomainListAllSnapshots() with the + * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter. * * virDomainSnapshotFree should be used to free the resources after the * snapshot object is no longer needed. -- 2.20.1
On Fri, Jul 05, 2019 at 23:37:34 -0500, Eric Blake wrote:
Add a new filter group VIR_DOMAIN_SNAPSHOT_LIST_CURRENT/NO_CURRENT, which restricts the output based on whether a snapshot is the domain's current snapshot. This is redundant with existing API (both virDomainHasCurrentSnapshot and virDomainSnapshotCurrent can be emulated by a single call to virDomainListAllSnapshots, and replacing virDomainSnapshotIsCurrent is also possible but a bit more indirect). However, adding the new filters makes snapshots slightly more consistent with the plans for the upcoming checkpoint API additions to ONLY provide access to the notion of being current via list filters or XML inspection.
This will be justified only if you make the same changes to expose multiple "current" snapshots as well, since external snapshots allow for the exact same problems which you'll get with checkpoints too.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 5 +++++ src/conf/virdomainsnapshotobjlist.h | 7 ++++++- src/conf/virdomainsnapshotobjlist.c | 4 ++++ src/libvirt-domain-snapshot.c | 14 ++++++++++++-- 4 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 90673ed0fb..02cdabafbc 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -141,6 +141,11 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur before children in the resulting list */ + + VIR_DOMAIN_SNAPSHOT_LIST_CURRENT = (1 << 11), /* Filter to just current + snapshot */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT = (1 << 12), /* Filter out current + snapshot */
This will require plural form of 'snapshot'
} virDomainSnapshotListFlags;
/* Return the number of snapshots for this domain */
[...]
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 2687a34b96..5dd2c5390b 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -465,6 +465,12 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, * whether the snapshot is stored inside the disk images or as * additional files. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_CURRENT and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT, to filter based on whether a + * snapshot is current. This filter provides the same functionality as + * virDomainHasCurrentSnapshot(), virDomainSnapshotCurrent(), and + * virDomainSnapshotIsCurrent(). + * * Returns the number of domain snapshots found or -1 and sets @snaps to * NULL in case of error. On success, the array stored into @snaps is * guaranteed to have an extra allocated element set to NULL but not included @@ -736,7 +742,9 @@ virDomainSnapshotLookupByName(virDomainPtr domain, * @domain: pointer to the domain object * @flags: extra flags; not used yet, so callers should always pass 0 * - * Determine if the domain has a current snapshot. + * Determine if the domain has a current snapshot. This can also be + * determined by using virDomainListAllSnapshots() with the + * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter.
This implies that there's only one 'current' snapshot. The documentation will need to be updated to reflect that the bulk-list API will be a superset of the APIs.
* * Returns 1 if such snapshot exists, 0 if it doesn't, -1 on error. */
This patch makes sense if we do the same changes to support multiple "current" snapshots similarly to what you plan with checkpoints. In that case I'd like to see it together with the patchset making that happen. Or at least I'd like to review the wording changes. (The last possibility obviously is to stop thinking about "current" snapshots and checkpoints since it's quite tricky).
We already burned 'virsh snapshot-list --current' for a different purpose, so this adds --current-only/--no-current as the new bool flags to select the new filter bits. Update tests/virsh-snapshot to exercise the new flags. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virsh-snapshot | 8 +++++++- tools/virsh-snapshot.c | 10 ++++++++++ tools/virsh.pod | 7 ++++++- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/virsh-snapshot b/tests/virsh-snapshot index 510904f470..8b0813233f 100755 --- a/tests/virsh-snapshot +++ b/tests/virsh-snapshot @@ -52,6 +52,7 @@ $abs_top_builddir/tools/virsh --connect test:///default >out 2>err ' snapshot-current test --name # Deleting current root leads to multiple roots, demonstrate list filtering snapshot-delete test --current + snapshot-list test --current-only --name echo --err marker snapshot-current test --name echo --err marker @@ -63,7 +64,7 @@ $abs_top_builddir/tools/virsh --connect test:///default >out 2>err ' # More fun with delete flags, current node moves up to remaining parent snapshot-current test s4 snapshot-delete test --children-only s6 - snapshot-current test --name + snapshot-list test --current-only --name snapshot-delete test --children s7 snapshot-current test --name snapshot-delete test s6 @@ -71,6 +72,7 @@ $abs_top_builddir/tools/virsh --connect test:///default >out 2>err ' # Now the tree is linear, so we have an unambiguous topological order snapshot-list test --name snapshot-list test --name --topological + snapshot-list test --name --no-current # Capture some XML for later redefine echo "<!--MarkerA-->" snapshot-dumpxml test s3 @@ -123,6 +125,7 @@ Domain snapshot s1 deleted + Name Creation Time State --------------------------------------------- s3 TIMESTAMP running @@ -155,6 +158,7 @@ Snapshot s4 set as current Domain snapshot s6 children deleted s6 + Domain snapshot s7 deleted s6 @@ -167,6 +171,8 @@ s3 s3 s2 +s2 + EOF compare exp out.cooked || fail=1 diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index f7772ce549..9f8d1c5e5c 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1463,6 +1463,14 @@ static const vshCmdOptDef opts_snapshot_list[] = { .type = VSH_OT_BOOL, .help = N_("sort list topologically rather than by name"), }, + {.name = "current-only", + .type = VSH_OT_BOOL, + .help = N_("filter to just the current snapshot"), + }, + {.name = "no-current", + .type = VSH_OT_BOOL, + .help = N_("filter out the current snapshot"), + }, {.name = NULL} }; @@ -1522,6 +1530,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) FILTER("disk-only", DISK_ONLY); FILTER("internal", INTERNAL); FILTER("external", EXTERNAL); + FILTER("current-only", CURRENT); + FILTER("no-current", NO_CURRENT); #undef FILTER if (vshCommandOptBool(cmd, "topological")) diff --git a/tools/virsh.pod b/tools/virsh.pod index 865fb2b0da..546727a90f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -4767,7 +4767,8 @@ with I<--current>. [{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] [I<--topological>] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]] [I<--leaves>] [I<--no-leaves>] [I<--inactive>] [I<--active>] -[I<--disk-only>] [I<--internal>] [I<--external>] +[I<--disk-only>] [I<--internal>] [I<--external>] [I<--current-only>] +[I<--no-current>] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -4824,6 +4825,10 @@ that use internal storage of existing disk images. If I<--external> is specified, the list will be filtered to snapshots that use external files for disk images or memory state. +If I<--current-only> is specified, the list will be filtered to just +the current snapshot (if it meets all other filters). If I<--no-current> +is specified, the list will be filtered to exclude the current snapshot. + =item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] Output the snapshot XML for the domain's snapshot named I<snapshot>. -- 2.20.1
participants (2)
-
Eric Blake -
Peter Krempa