[libvirt] [PATCH 0/3] virDomainXMLFlags cleanups
Nir has asked that my upcoming checkpoint APIs have a way to do bulk operations: grab the XML for ALL checkpoints in one call, and in turn redefine checkpoints on a new host using the dumped XML from an old host in one call. But since checkpoints borrow heavily from the APIs used for snapshots, it makes sense to do the same thing for checkpoints. And in the process of implementing that code, I discovered some oddities in the use of flags when dumping domain-related XML, which this series aims to fix. Eric Blake (3): domain: Document VIR_DOMAIN_XML_MIGRATABLE domain: Define explicit flags for saved image xml snapshot: Define explicit flags for snapshot xml include/libvirt/libvirt-domain-snapshot.h | 4 ++++ include/libvirt/libvirt-domain.h | 5 +++++ src/libvirt-domain-snapshot.c | 9 ++++----- src/libvirt-domain.c | 22 +++++++++++++--------- src/qemu/qemu_driver.c | 6 +++--- src/remote/remote_protocol.x | 6 +++--- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 8 files changed, 34 insertions(+), 22 deletions(-) -- 2.20.1
Commit 28f8dfdc (1.0.0) added a flag to virDomainGetXMLDesc, but failed to document its effects. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt-domain.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 54ca18f249..6158382d07 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2559,7 +2559,13 @@ virDomainGetControlInfo(virDomainPtr domain, * currently running domain. If @flags contains * VIR_DOMAIN_XML_UPDATE_CPU, then the portion of the domain XML * describing CPU capabilities is modified to match actual - * capabilities of the host. + * capabilities of the host. If @flags contains VIR_DOMAIN_XML_MIGRATABLE, + * the XML is altered to trim redundant information that might interfere + * with migration to an older version of libvirt, as well as expose additional + * information internal to libvirt; this flag is rejected on read-only + * connections, and the resulting XML might not validate against the schema, + * but it can serve as a starting point for custom XML in calls such as + * virDomainMigrate2(). * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. -- 2.20.1
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use of @flags as a subset of virDomainXMLFlags, documenting that 2 of the 3 flags defined at the time would never be valid. Later, commit 28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but did not adjust the save image documentation to declare it as invalid. Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text into virDomainManagedSaveGetXMLDesc. However, since the flag is not accepted as valid by any of the drivers (remote is just passthrough; and qemu is the only supporting driver with support for just VIR_DOMAIN_XML_SECURE), and it is unlikely that the domain state saved off during saved image creation needs to be migration-friendly (the saved image is loaded by the current libvirt version, and not migrated to a potentially older version), it is easier to just define an explicit set of supported flags directly related to the snapshot API rather than trying to borrow from live domain API, and risking confusion if even more domain flags are added later (in fact, I have an upcoming patch that plans to add a new flag to virDomainGetXMLDesc that makes no sense for saved images). On the other hand, it DOES make sense to reuse the same flags for SaveImage and for ManagedSave (since ManagedSave is really just sugar for creating a normal SaveImage in a location controlled by libvirt instead of by the user). There is no API or ABI impact (since we purposefully used unsigned int rather than an enum type in public API, and since the new flag name carries the same value as the reused name). Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 5 +++++ src/libvirt-domain.c | 14 ++++++-------- src/qemu/qemu_driver.c | 4 ++-- src/remote/remote_protocol.x | 4 ++-- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index d82c75a9d9..1d5bdb545d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1219,6 +1219,7 @@ int virDomainRestoreFlags (virConnectPtr conn, const char *dxml, unsigned int flags); +/* See below for virDomainSaveImageXMLFlags */ char * virDomainSaveImageGetXMLDesc (virConnectPtr conn, const char *file, unsigned int flags); @@ -1571,6 +1572,10 @@ typedef enum { VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ } virDomainXMLFlags; +typedef enum { + VIR_DOMAIN_SAVE_IMAGE_XML_SECURE = VIR_DOMAIN_XML_SECURE, /* dump security sensitive information too */ +} virDomainSaveImageXMLFlags; + char * virDomainGetXMLDesc (virDomainPtr domain, unsigned int flags); diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6158382d07..4528cab0e2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1066,16 +1066,15 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, * virDomainSaveImageGetXMLDesc: * @conn: pointer to the hypervisor connection * @file: path to saved state file - * @flags: bitwise-OR of subset of virDomainXMLFlags + * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags * * This method will extract the XML describing the domain at the time * a saved state file was created. @file must be a file created * previously by virDomainSave() or virDomainSaveFlags(). * * No security-sensitive data will be included unless @flags contains - * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only + * connections. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. @@ -9483,15 +9482,14 @@ virDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) /** * virDomainManagedSaveGetXMLDesc: * @domain: a domain object - * @flags: bitwise-OR of subset of virDomainXMLFlags + * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags * * This method will extract the XML description of the managed save * state file of a domain. * * No security-sensitive data will be included unless @flags contains - * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only + * connections. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 971f915619..40bd24cdf2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7082,7 +7082,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virQEMUSaveDataPtr data = NULL; /* We only take subset of virDomainDefFormat flags. */ - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); fd = qemuDomainSaveImageOpen(driver, path, &def, &data, false, NULL, false, false); @@ -7187,7 +7187,7 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) virQEMUSaveDataPtr data = NULL; /* We only take subset of virDomainDefFormat flags. */ - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL); if (!(vm = qemuDomObjFromDomain(dom))) return ret; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b9d26b1849..42a87d418b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -5235,7 +5235,7 @@ enum remote_procedure { * @generate: both * @priority: high * @acl: domain:read - * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE + * @acl: domain:read_secure:VIR_DOMAIN_SAVE_IMAGE_XML_SECURE */ REMOTE_PROC_DOMAIN_SAVE_IMAGE_GET_XML_DESC = 235, @@ -6230,7 +6230,7 @@ enum remote_procedure { /** * @generate: both * @acl: domain:read - * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE + * @acl: domain:read_secure:VIR_DOMAIN_SAVE_IMAGE_XML_SECURE */ REMOTE_PROC_DOMAIN_MANAGED_SAVE_GET_XML_DESC = 388, -- 2.20.1
On Wed, Feb 13, 2019 at 05:22:50PM -0600, Eric Blake wrote:
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use of @flags as a subset of virDomainXMLFlags, documenting that 2 of the 3 flags defined at the time would never be valid. Later, commit 28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but did not adjust the save image documentation to declare it as invalid. Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the drivers (remote is just passthrough; and qemu is the only supporting driver with support for just VIR_DOMAIN_XML_SECURE), and it is unlikely that the domain state saved off during saved image creation needs to be migration-friendly (the saved image is loaded by the current libvirt version, and not migrated to a potentially older version), it is easier to just define an explicit set of supported
This is not accurate. A save image can definitely be loaded by newer libvirt. The non-managed saved APIs can also created images that will be loaded by older libvirt, since images may be copied across hosts. So the same rules apply to saved images as to migration. That said I don't think it affects your patch, only this commit message
flags directly related to the snapshot API rather than trying to borrow from live domain API, and risking confusion if even more domain flags are added later (in fact, I have an upcoming patch that plans to add a new flag to virDomainGetXMLDesc that makes no sense for saved images). On the other hand, it DOES make sense to reuse the same flags for SaveImage and for ManagedSave (since ManagedSave is really just sugar for creating a normal SaveImage in a location controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int rather than an enum type in public API, and since the new flag name carries the same value as the reused name).
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain.h | 5 +++++ src/libvirt-domain.c | 14 ++++++-------- src/qemu/qemu_driver.c | 4 ++-- src/remote/remote_protocol.x | 4 ++-- 4 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index d82c75a9d9..1d5bdb545d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1219,6 +1219,7 @@ int virDomainRestoreFlags (virConnectPtr conn, const char *dxml, unsigned int flags);
+/* See below for virDomainSaveImageXMLFlags */ char * virDomainSaveImageGetXMLDesc (virConnectPtr conn, const char *file, unsigned int flags); @@ -1571,6 +1572,10 @@ typedef enum { VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ } virDomainXMLFlags;
+typedef enum { + VIR_DOMAIN_SAVE_IMAGE_XML_SECURE = VIR_DOMAIN_XML_SECURE, /* dump security sensitive information too */ +} virDomainSaveImageXMLFlags; + char * virDomainGetXMLDesc (virDomainPtr domain, unsigned int flags);
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6158382d07..4528cab0e2 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -1066,16 +1066,15 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, * virDomainSaveImageGetXMLDesc: * @conn: pointer to the hypervisor connection * @file: path to saved state file - * @flags: bitwise-OR of subset of virDomainXMLFlags + * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags * * This method will extract the XML describing the domain at the time * a saved state file was created. @file must be a file created * previously by virDomainSave() or virDomainSaveFlags(). * * No security-sensitive data will be included unless @flags contains - * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only + * connections. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. @@ -9483,15 +9482,14 @@ virDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) /** * virDomainManagedSaveGetXMLDesc: * @domain: a domain object - * @flags: bitwise-OR of subset of virDomainXMLFlags + * @flags: bitwise-OR of supported virDomainSaveImageXMLFlags * * This method will extract the XML description of the managed save * state file of a domain. * * No security-sensitive data will be included unless @flags contains - * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * VIR_DOMAIN_SAVE_IMAGE_XML_SECURE; this flag is rejected on read-only + * connections. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 971f915619..40bd24cdf2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7082,7 +7082,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, virQEMUSaveDataPtr data = NULL;
/* We only take subset of virDomainDefFormat flags. */ - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL);
fd = qemuDomainSaveImageOpen(driver, path, &def, &data, false, NULL, false, false); @@ -7187,7 +7187,7 @@ qemuDomainManagedSaveGetXMLDesc(virDomainPtr dom, unsigned int flags) virQEMUSaveDataPtr data = NULL;
/* We only take subset of virDomainDefFormat flags. */ - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SAVE_IMAGE_XML_SECURE, NULL);
if (!(vm = qemuDomObjFromDomain(dom))) return ret; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b9d26b1849..42a87d418b 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -5235,7 +5235,7 @@ enum remote_procedure { * @generate: both * @priority: high * @acl: domain:read - * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE + * @acl: domain:read_secure:VIR_DOMAIN_SAVE_IMAGE_XML_SECURE */ REMOTE_PROC_DOMAIN_SAVE_IMAGE_GET_XML_DESC = 235,
@@ -6230,7 +6230,7 @@ enum remote_procedure { /** * @generate: both * @acl: domain:read - * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE + * @acl: domain:read_secure:VIR_DOMAIN_SAVE_IMAGE_XML_SECURE */ REMOTE_PROC_DOMAIN_MANAGED_SAVE_GET_XML_DESC = 388,
-- 2.20.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use of @flags as a subset of virDomainXMLFlags, documenting that 2 of the 3 flags defined at the time would never be valid. Later, commit 28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but did not adjust the snapshot documentation to declare it as invalid. However, since the flag is not accepted as valid by any of the drivers (remote is just passthrough; esx and vbox don't support flags; qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is unlikely that the domain state saved off during a snapshot creation needs to be migration-friendly (as the snapshot is not the source of a migration), it is easier to just define an explicit set of supported flags directly related to the snapshot API rather than trying to borrow from domain API, and risking confusion if even more domain flags are added later (in fact, I have an upcoming patch that plans to add a new flag to virDomainGetXMLDesc that makes no sense for snapshots). There is no API or ABI impact (since we purposefully used unsigned int rather than an enum type in public API, and since the new flag name carries the same value as the reused name). Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt-domain-snapshot.h | 4 ++++ src/libvirt-domain-snapshot.c | 9 ++++----- src/qemu/qemu_driver.c | 2 +- src/remote/remote_protocol.x | 2 +- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 6 files changed, 12 insertions(+), 9 deletions(-) diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h index 0c9985f7f4..2532b99c58 100644 --- a/include/libvirt/libvirt-domain-snapshot.h +++ b/include/libvirt/libvirt-domain-snapshot.h @@ -78,6 +78,10 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, unsigned int flags); +typedef enum { + VIR_DOMAIN_SNAPSHOT_XML_SECURE = VIR_DOMAIN_XML_SECURE, /* dump security sensitive information too */ +} virDomainSnapshotXMLFlags; + /* Dump the XML of a snapshot */ char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags); diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 100326a5e7..a724c66421 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -244,14 +244,13 @@ virDomainSnapshotCreateXML(virDomainPtr domain, /** * virDomainSnapshotGetXMLDesc: * @snapshot: a domain snapshot object - * @flags: bitwise-OR of subset of virDomainXMLFlags + * @flags: bitwise-OR of supported virDomainSnapshotXMLFlags * * Provide an XML description of the domain snapshot. * * No security-sensitive data will be included unless @flags contains - * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only + * connections. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. @@ -268,7 +267,7 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virCheckDomainSnapshotReturn(snapshot, NULL); conn = snapshot->domain->conn; - if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) { + if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_SNAPSHOT_XML_SECURE)) { virReportError(VIR_ERR_OPERATION_DENIED, "%s", _("virDomainSnapshotGetXMLDesc with secure flag")); goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40bd24cdf2..a4c169f71c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16251,7 +16251,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_XML_SECURE, NULL); if (!(vm = qemuDomObjFromSnapshot(snapshot))) return NULL; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 42a87d418b..60cc40e04a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -4902,7 +4902,7 @@ enum remote_procedure { * @generate: both * @priority: high * @acl: domain:read - * @acl: domain:read_secure:VIR_DOMAIN_XML_SECURE + * @acl: domain:read_secure:VIR_DOMAIN_SNAPSHOT_XML_SECURE */ REMOTE_PROC_DOMAIN_SNAPSHOT_GET_XML_DESC = 186, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c1faff46ff..155ebc00d6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6197,7 +6197,7 @@ testDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, char uuidstr[VIR_UUID_STRING_BUFLEN]; testDriverPtr privconn = snapshot->domain->conn->privateData; - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_XML_SECURE, NULL); if (!(vm = testDomObjFromSnapshot(snapshot))) return NULL; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index b22a44d6ad..fe46642e38 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2273,7 +2273,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) virDomainSnapshotObjListPtr snapshots = NULL; vzConnPtr privconn = snapshot->domain->conn->privateData; - virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_XML_SECURE, NULL); if (!(dom = vzDomObjFromDomain(snapshot->domain))) return NULL; -- 2.20.1
participants (2)
-
Daniel P. Berrangé -
Eric Blake