[libvirt] [PATCH v2 0/5] virDomainXMLFlags cleanups (incremental backup saga)
Since v1: add two new patches (1, 2), improve commit message wording of patch 4/5 [Dan]. Eric Blake (5): domain: Fix unknown flags diagnosis in virDomainGetXMLDesc qemu: Use correct domain xml flag 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/conf/domain_conf.h | 3 +++ src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 2 ++ src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/libvirt-domain-snapshot.c | 9 ++++----- src/libvirt-domain.c | 22 +++++++++++++--------- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 11 ++++++----- src/remote/remote_protocol.x | 6 +++--- src/test/test_driver.c | 4 ++-- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 4 ++-- 19 files changed, 54 insertions(+), 34 deletions(-) -- 2.20.1
Many drivers had a comment that they did not validate the incoming 'flags' to virDomainGetXMLDesc() because they were relying on virDomainDefFormat() to do it instead. This used to be the case, but regressed in commit 0ecd6851 (1.2.12), when all of the drivers were changed to pass 'flags' through the new helper virDomainDefFormatConvertXMLFlags(). Since this helper silently ignores unknown flags, we need to implement flag checking in each driver instead. Annoyingly, this means that any new flag values added will silently be ignored when targeting an older libvirt, rather than our usual practice of loudly diagnosing an unsupported flag. We'll have to be extra vigilant that any future added flags do not cause a security hole when sent from a newer libvirt client that expects the flag to do one thing, but where the older server silently ignores it instead. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 2 ++ src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 14 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 2bc3f879f7..324fc247b6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3110,6 +3110,9 @@ virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def, unsigned int iothread_id); void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id); +# define VIR_DOMAIN_XML_COMMON_FLAGS \ + (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE | \ + VIR_DOMAIN_XML_MIGRATABLE) unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 912797cfcf..3e192284cc 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -484,6 +484,8 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virCapsPtr caps = NULL; char *ret = NULL; + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); + if (!(vm = bhyveDomObjFromDomain(domain))) goto cleanup; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5d49f4388c..37bbf211c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28996,6 +28996,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, return -1; } +/* Converts VIR_DOMAIN_XML_COMMON_FLAGS into VIR_DOMAIN_DEF_FORMAT_* flags, + * and silently ignores any other flags. */ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) { unsigned int formatFlags = 0; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index b1af646155..379c2bae73 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -2604,7 +2604,7 @@ esxDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainDefPtr def = NULL; char *xml = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); memset(&data, 0, sizeof(data)); diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index f41cd1c939..0e2c6c55ef 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -754,7 +754,7 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) Msvm_ProcessorSettingData *processorSettingData = NULL; Msvm_MemorySettingData *memorySettingData = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(def = virDomainDefNew())) goto cleanup; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7981ccaf21..31b842aeee 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2621,7 +2621,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefPtr def; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c48f6d9067..516a6b4de3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -987,7 +987,7 @@ static char *lxcDomainGetXMLDesc(virDomainPtr dom, virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 06950ce9ed..39eeb2c12e 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -519,7 +519,7 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) return NULL; diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index dc082b1d08..e54799dbb4 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -3214,7 +3214,7 @@ phypDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) unsigned long long memory; unsigned int vcpus; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); memset(&def, 0, sizeof(virDomainDef)); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 971f915619..b039675d1a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7342,7 +7342,8 @@ static char virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU, + NULL); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c1faff46ff..cde9e3d417 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2628,7 +2628,7 @@ static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr privdom; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(privdom = testDomObjFromDomain(domain))) return NULL; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 664650f217..d95fe7c7ae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4052,7 +4052,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (!data->vboxObj) return ret; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (openSessionForMachine(data, dom->uuid, &iid, &machine) < 0) goto cleanup; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index f94b3252fe..f4b0989afd 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -932,7 +932,7 @@ vmwareDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) return NULL; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index b22a44d6ad..f99ade82b6 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -724,7 +724,7 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virDomainObjPtr dom; char *ret = NULL; - /* Flags checked by virDomainDefFormat */ + virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS, NULL); if (!(dom = vzDomObjFromDomain(domain))) return NULL; -- 2.20.1
On 2/14/19 4:29 PM, Eric Blake wrote:
Many drivers had a comment that they did not validate the incoming 'flags' to virDomainGetXMLDesc() because they were relying on virDomainDefFormat() to do it instead. This used to be the case, but regressed in commit 0ecd6851 (1.2.12), when all of the drivers were changed to pass 'flags' through the new helper virDomainDefFormatConvertXMLFlags(). Since this helper silently ignores unknown flags, we need to implement flag checking in each driver instead.
Annoyingly, this means that any new flag values added will silently be ignored when targeting an older libvirt, rather than our usual practice of loudly diagnosing an unsupported flag. We'll have to be extra vigilant that any future added flags do not cause a security hole when sent from a newer libvirt client that expects the flag to do one thing, but where the older server silently ignores it instead.
Since this is limited to the GetXMLDesc processing wouldn't this limit the exposure in such a way that some new flag expecting some default action would not return expected or "new" results on the newer libvirt vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that list, the get wouldn't fail because it doesn't know the flag, but it also wouldn't return any XML related to the new flag, right? Secondary to that knowing this would require someone to read this specific commit message to garnish at least a small understanding of the issue. Perhaps some comments in domain_conf.h near the new VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand those without understanding the impact. Similarly, near the flags definition either a similar noteworthy comment or a comment to go read the domain_conf.h comment. There's a quick comment added before virDomainDefFormatConvertXMLFlags in this patch that could be expandable instead. Doesn't mean someone will read and/or understand it, but at least leaves a trail.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 2 ++ src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 14 files changed, 19 insertions(+), 11 deletions(-)
There is one extra I'd question, qemuDomainDefFormatBufInternal that perhaps could add the virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU). Chasing current callers seems to eventually ensure no more than those flags are used. That may mean an extra check for a couple of paths, but it does ensure others don't need to check. Here's my lookup/tree of callers and what could be passed originally. * qemuDomainDefFormatBuf * qemuMigrationCookieXMLFormat * Uses INACTIVE | SECURE | MIGRATABLE * qemuDomainDefFormatXMLInternal * qemuDomainDefFormatXML * qemuDomainDefCopy * qemuDomainDefCheckABIStability * qemuDomainCheckABIStability * Each uses SECURE | MIGRATABLE (via COPY_FLAGS) * qemuDomainSaveImageUpdateDef * Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE * qemuMigrationSrcRun * Uses SECURE | MIGRATABLE * qemuDomainSaveImageGetXMLDesc * qemuDomainManagedSaveGetXMLDesc * Each checks virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); * qemuDomainSaveImageDefineXML * Uses INACTIVE | SECURE | MIGRATABLE * qemuConnectDomainXMLFromNative * Checks virCheckFlags(0, NULL); * qemuMigrationDstPrepareAny * Uses SECURE | MIGRATABLE * qemuProcessStartHook * qemuProcessStop * qemuProcessAttach * qemuProcessReconnect * Each passes 0 * qemuDomainFormatXML * qemuDomainCheckABIStability * Same as above using COPY_FLAGS * qemuDomainGetXMLDesc * Newly repatched (and why I added UPDATE_CPU flag check for the virCheckFlags in my qemuDomainDefFormatBufInternal query). * qemuMigrationSrcPerformPeer2Peer2 * Uses SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) | MIGRATABLE * qemuDomainDefFormatLive * Will pass only SECURE (via QEMU_DOMAIN_FORMAT_LIVE_FLAGS) and possibly INACTIVE or MIGRATABLE BTW: For the two callers that pass all 3 maybe it'd be good to change those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently: * qemuMigrationCookieXMLFormat * qemuDomainSaveImageDefineXML What's here so far is fine. The qemuDomainDefFormatBufInternal processing is something that perhaps is "extra protection" considering all the paths that could get there. I'll let you decide whether it needs to be part of this, not part of this, or another patch. Beyond that, I assume you can add the correct attribution/comments... Reviewed-by: John Ferlan <jferlan@redhat.com> John FWIW: The following were my notes during review - which I think covers the history that I found... Whether any of it is useful or not is up to you. Commit 461e0f1a2 (0.9.4) is where you first added code to virDomainDefFormat in order to check (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_UPDATE_CPU). Commit 20135c70 (0.9.4) introduces virDomainDefFormatInternal and DUMPXML_FLAGS to mean the same thing as well as adding VIR_DOMAIN_XML_INTERNAL_STATUS to the virCheckFlags so that virDomainObjFormat will work properly. It adds the verify macro to ensure no overlap. Commit 07f413699 (0.9.4) adds VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET to the list of flags that virDomainDefFormatInternal cares about. The verify gets adjusted as well. Commit d84b3626 (0.9.7) adds VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES to the list of flags that virDomainDefFormatInternal cares about. The verify gets adjusted as well. Commit c01ba1a48 (0.9.10) adds VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM and VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT, but does not update the verify macro. Commit 28f8dfdcc (1.0.0) adds VIR_DOMAIN_XML_MIGRATABLE to the DUMPXML_FLAGS list (includes virDomainObjFormat caller too). There is no need to change the verify. Commit e31b5cf39 (1.1.0) adds VIR_DOMAIN_XML_INTERNAL_BASEDATE to the list of flags that virDomainDefFormatInternal cares about. The verify is updated; however, still missing the ALLOW_ROM and ALLOW_BOOT. Commit b8efa6f2e (1.2.5 reverts the VIR_DOMAIN_XML_INTERNAL_BASEDATE from the virDomainDefFormatInternal list of flags. Commit b62d67da3 (1.2.5) adds VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST to the list of flags that virDomainDefFormatInternal cares about. The verify macro gets adjusted, but still not with ALLOW_ROM or ALLOW_BOOT. Commit 79f4c4e69 (1.2.8) moves where the the verify macro closer to where the internal flags are defined and adds ALLOW_ROM and ALLOW_BOOT. (a task that would require a couple of patches in today's world). Commit 37588b25 (1.2.8) adds VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE to the list of flags, but doesn't change virDomainDefFormatInternal since all it cares about is <disk> processing and adds that flag to the list of flags processed in virDomainDiskDefSourceParse. Commit 0ecd6851 (1.2.12) as you note comes along and alters things. It too would probably require more steps in today's world ;-). Commit 43a90eb7 (3.8.0) drops VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU
On 2/19/19 1:31 PM, John Ferlan wrote:
On 2/14/19 4:29 PM, Eric Blake wrote:
Many drivers had a comment that they did not validate the incoming 'flags' to virDomainGetXMLDesc() because they were relying on virDomainDefFormat() to do it instead. This used to be the case, but regressed in commit 0ecd6851 (1.2.12), when all of the drivers were changed to pass 'flags' through the new helper virDomainDefFormatConvertXMLFlags(). Since this helper silently ignores unknown flags, we need to implement flag checking in each driver instead.
Annoyingly, this means that any new flag values added will silently be ignored when targeting an older libvirt, rather than our usual practice of loudly diagnosing an unsupported flag. We'll have to be extra vigilant that any future added flags do not cause a security hole when sent from a newer libvirt client that expects the flag to do one thing, but where the older server silently ignores it instead.
Since this is limited to the GetXMLDesc processing wouldn't this limit the exposure in such a way that some new flag expecting some default action would not return expected or "new" results on the newer libvirt vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that list, the get wouldn't fail because it doesn't know the flag, but it also wouldn't return any XML related to the new flag, right?
Indeed - and that's how I found it: I'm trying to add VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was confused when I patched virsh to support the new flag but the old libvirt didn't react to it in any way (I then confirmed that libvirt running on RHEL 6 _does_ react to the unknown flag). Missing output is not a security breach, but is annoying enough to be worth comments somewhere as to the fact that the problem exists (worse would be if we wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, where failure to obey the flag results in leaking XML information that should have been suppressed).
Secondary to that knowing this would require someone to read this specific commit message to garnish at least a small understanding of the issue. Perhaps some comments in domain_conf.h near the new VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand those without understanding the impact. Similarly, near the flags definition either a similar noteworthy comment or a comment to go read the domain_conf.h comment. There's a quick comment added before virDomainDefFormatConvertXMLFlags in this patch that could be expandable instead. Doesn't mean someone will read and/or understand it, but at least leaves a trail.
Sure, I can add some strategic comments.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/bhyve/bhyve_driver.c | 2 ++ src/conf/domain_conf.c | 2 ++ src/esx/esx_driver.c | 2 +- src/hyperv/hyperv_driver.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/openvz/openvz_driver.c | 2 +- src/phyp/phyp_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 2 +- src/vbox/vbox_common.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 14 files changed, 19 insertions(+), 11 deletions(-)
There is one extra I'd question, qemuDomainDefFormatBufInternal that perhaps could add the virCheckFlags(VIR_DOMAIN_XML_COMMON_FLAGS | VIR_DOMAIN_XML_UPDATE_CPU). Chasing current callers seems to eventually ensure no more than those flags are used. That may mean an extra check for a couple of paths, but it does ensure others don't need to check.
It would be a redundant check at the moment, but you are right that from the maintenance point of view it serves as self-documentation that might help the next person chasing things (that is, if all points that format XML have a local check, even if that local check is a superset of what was done in the caller, then you don't have to chase all callers).
Here's my lookup/tree of callers and what could be passed originally.
Thanks for that audit; it matches what I saw while working on the patch.
BTW: For the two callers that pass all 3 maybe it'd be good to change those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently:
* qemuMigrationCookieXMLFormat * qemuDomainSaveImageDefineXML
Makes sense, although I might split it as a separate patch.
What's here so far is fine. The qemuDomainDefFormatBufInternal processing is something that perhaps is "extra protection" considering all the paths that could get there. I'll let you decide whether it needs to be part of this, not part of this, or another patch.
Beyond that, I assume you can add the correct attribution/comments...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
FWIW: The following were my notes during review - which I think covers the history that I found... Whether any of it is useful or not is up to you.
I might add some of it into the final commit message. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/19/19 2:24 PM, Eric Blake wrote:
Since this is limited to the GetXMLDesc processing wouldn't this limit the exposure in such a way that some new flag expecting some default action would not return expected or "new" results on the newer libvirt vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that list, the get wouldn't fail because it doesn't know the flag, but it also wouldn't return any XML related to the new flag, right?
Indeed - and that's how I found it: I'm trying to add VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was confused when I patched virsh to support the new flag but the old libvirt didn't react to it in any way (I then confirmed that libvirt running on RHEL 6 _does_ react to the unknown flag). Missing output is not a security breach, but is annoying enough to be worth comments somewhere as to the fact that the problem exists (worse would be if we wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, where failure to obey the flag results in leaking XML information that should have been suppressed).
Secondary to that knowing this would require someone to read this specific commit message to garnish at least a small understanding of the issue. Perhaps some comments in domain_conf.h near the new VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand those without understanding the impact. Similarly, near the flags definition either a similar noteworthy comment or a comment to go read the domain_conf.h comment. There's a quick comment added before virDomainDefFormatConvertXMLFlags in this patch that could be expandable instead. Doesn't mean someone will read and/or understand it, but at least leaves a trail.
Sure, I can add some strategic comments.
Hmm - is there an easy way to add comments to the public include/ headers that are useful to developers, but which don't pollute the generated documentation? Or is this the sort of fix where a public doc comment that "some older versions of libvirt failed to diagnose unknown flags" is acceptable? Comments in domain_conf.[ch] are much easier as they don't affect the public docs. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/19/19 3:33 PM, Eric Blake wrote:
On 2/19/19 2:24 PM, Eric Blake wrote:
Since this is limited to the GetXMLDesc processing wouldn't this limit the exposure in such a way that some new flag expecting some default action would not return expected or "new" results on the newer libvirt vs. some older one? That is limited to VIR_DOMAIN_XML_COMMON_FLAGS (SECURE, INACTIVE, MIGRATABLE). If say, CHECKPOINTABLE was added to that list, the get wouldn't fail because it doesn't know the flag, but it also wouldn't return any XML related to the new flag, right?
Indeed - and that's how I found it: I'm trying to add VIR_DOMAIN_XML_SNAPSHOTS and VIR_DOMAIN_XML_CHECKPOINTS flags, and was confused when I patched virsh to support the new flag but the old libvirt didn't react to it in any way (I then confirmed that libvirt running on RHEL 6 _does_ react to the unknown flag). Missing output is not a security breach, but is annoying enough to be worth comments somewhere as to the fact that the problem exists (worse would be if we wanted to add a flag comparable in nature to VIR_DOMAIN_XML_SECURE, where failure to obey the flag results in leaking XML information that should have been suppressed).
Secondary to that knowing this would require someone to read this specific commit message to garnish at least a small understanding of the issue. Perhaps some comments in domain_conf.h near the new VIR_DOMAIN_XML_COMMON_FLAGS would help ensure someone doesn't expand those without understanding the impact. Similarly, near the flags definition either a similar noteworthy comment or a comment to go read the domain_conf.h comment. There's a quick comment added before virDomainDefFormatConvertXMLFlags in this patch that could be expandable instead. Doesn't mean someone will read and/or understand it, but at least leaves a trail.
Sure, I can add some strategic comments.
Hmm - is there an easy way to add comments to the public include/ headers that are useful to developers, but which don't pollute the generated documentation? Or is this the sort of fix where a public doc comment that "some older versions of libvirt failed to diagnose unknown flags" is acceptable? Comments in domain_conf.[ch] are much easier as they don't affect the public docs.
I'm fine keeping away from the public doc comment about this. If (so far) no one has noticed, then why call attention to it. In the long run, I would think it's more important for a libvirt developer to not lose sight of the issue as opposed to a developer on libvirt. When/if the new flag is added whomever adds it would hopefully read the comments and if they don't we can always hope whomever reviews it may ask. John
On 2/19/19 2:24 PM, Eric Blake wrote:
BTW: For the two callers that pass all 3 maybe it'd be good to change those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently:
* qemuMigrationCookieXMLFormat * qemuDomainSaveImageDefineXML
Makes sense, although I might split it as a separate patch.
Actually, no, it doesn't make sense. If VIR_DOMAIN_XML_COMMON_FLAGS gains a new flag, we don't want these callers to start outputting new information merely because they used the convenience macro. (That is, the macro is good for virCheckFlags(), to allow a driver to automatically support a new public flag by deferring to the common domain_conf implementation's support of the new flag; but it is not good for internal uses where we are generating specific output without regards to flags passed in through the public API). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/19/19 6:00 PM, Eric Blake wrote:
On 2/19/19 2:24 PM, Eric Blake wrote:
BTW: For the two callers that pass all 3 maybe it'd be good to change those to pass VIR_DOMAIN_XML_COMMON_FLAGS. Currently:
* qemuMigrationCookieXMLFormat * qemuDomainSaveImageDefineXML
Makes sense, although I might split it as a separate patch.
Actually, no, it doesn't make sense. If VIR_DOMAIN_XML_COMMON_FLAGS gains a new flag, we don't want these callers to start outputting new information merely because they used the convenience macro. (That is, the macro is good for virCheckFlags(), to allow a driver to automatically support a new public flag by deferring to the common domain_conf implementation's support of the new flag; but it is not good for internal uses where we are generating specific output without regards to flags passed in through the public API).
oh right, good thought. John
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE happen to have the same value (1<<1), they come from different enums; and it is nicer to reason about a 'flags' variable if all uses of that variable are compared against the same enum type. Messed up in commit 06f75ff2 (3.8.0). Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b039675d1a..2458343a86 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7360,7 +7360,7 @@ static char * ignore the VIR_DOMAIN_XML_UPDATE_CPU flag. */ if (virDomainObjIsActive(vm) && - !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + !(flags & VIR_DOMAIN_XML_INACTIVE)) flags &= ~VIR_DOMAIN_XML_UPDATE_CPU; ret = qemuDomainFormatXML(driver, vm, flags); -- 2.20.1
On 2/14/19 4:29 PM, Eric Blake wrote:
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE happen to have the same value (1<<1), they come from different enums; and it is nicer to reason about a 'flags' variable if all uses of that variable are compared against the same enum type. Messed up in commit 06f75ff2 (3.8.0).
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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
On 2/14/19 4:29 PM, Eric Blake wrote:
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().
You could just use the same attribution from the referenced commit message, e.g. "virDomainMigrate{,ToURI}2 or virDomainSaveFlags." I'm not insistent, just following back links. There could be other examples by now too I suppose. Reviewed-by: John Ferlan <jferlan@redhat.com> John
* * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value.
On 2/19/19 1:33 PM, John Ferlan wrote:
+ * 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().
Not for the public docs, but I may also mention that VIR_DOMAIN_XML_MIGRATABLE caused CVE-2014-7823 (commit b1674ad5) as part of my commit message.
You could just use the same attribution from the referenced commit message, e.g. "virDomainMigrate{,ToURI}2 or virDomainSaveFlags."
I'm not insistent, just following back links. There could be other examples by now too I suppose.
Yeah, the latter is what I feared, and did not want to audit for. So I'm leaving it as written with just the one example.
Reviewed-by: John Ferlan <jferlan@redhat.com>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/19/19 1:33 PM, John Ferlan wrote:
On 2/14/19 4:29 PM, Eric Blake wrote:
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().
You could just use the same attribution from the referenced commit message, e.g. "virDomainMigrate{,ToURI}2 or virDomainSaveFlags."
I'm not insistent, just following back links. There could be other examples by now too I suppose.
After looking at this one more, I'm going to submit a v3 on this patch with wording more like: + * If @flags contains VIR_DOMAIN_XML_MIGRATABLE, the XML is altered to + * assist in migrations where the source and destination are running + * different libvirt versions, whether by trimming redundant or + * default information that might confuse an older recipient, or by + * exposing internal details that aid a newer recipient; this flag is + * rejected on read-only connections, and the resulting XML might not + * validate against the schema, so it is mainly for internal use. For the rest of the series, I've made the edits you suggested and pushed. Thanks for the review. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 for either API, with support for just VIR_DOMAIN_XML_SECURE), it is easier to just define an explicit set of supported flags directly related to the save image 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). We may someday decide that saved images need to support the _MIGRATABLE flag, as it is possible to load a saved image with a different version of libvirt than the one that created it, but that can be a separate patch if it is ever needed. Meanwhile, 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 old 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 2458343a86..54750dc253 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 2/14/19 4:29 PM, 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 for either API, with support for just VIR_DOMAIN_XML_SECURE), it is easier to just define an explicit set of supported flags directly related to the save image 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). We may someday decide that saved images need to support the _MIGRATABLE flag, as it is possible to load a saved image with a different version of libvirt than the one that created it, but that can be a separate patch if it is ever needed. Meanwhile, 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 old 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(-)
Although it could be pointed out that the comment lines in qemu_driver above each flag change is moot. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 2/19/19 1:38 PM, John Ferlan wrote:
On 2/14/19 4:29 PM, 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 for either API, with support for just VIR_DOMAIN_XML_SECURE), it is easier to just define an explicit set of supported flags directly related to the save image 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). We may someday decide that saved images need to support the _MIGRATABLE flag, as it is possible to load a saved image with a different version of libvirt than the one that created it, but that can be a separate patch if it is ever needed. Meanwhile, 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 old 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(-)
Although it could be pointed out that the comment lines in qemu_driver above each flag change is moot.
Oh, good point. I'll drop those comments.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 2/14/19 4:29 PM, 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 for either API, with support for just VIR_DOMAIN_XML_SECURE), it is easier to just define an explicit set of supported flags directly related to the save image 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). We may someday decide that saved images need to support the _MIGRATABLE flag, as it is possible to load a saved image with a different version of libvirt than the one that created it, but that can be a separate patch if it is ever needed. Meanwhile, 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 old 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(-)
mea culpa and missa something that the subsequent patch points out ;-) The virDomainSaveImageGetXMLDesc and virDomainManagedSaveGetXMLDesc source code should use: if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_SAVE_IMAGE_XML_SECURE)) { and not if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) { <sigh>, John
On 2/19/19 1:44 PM, John Ferlan wrote:
On 2/14/19 4:29 PM, 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.
--- 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(-)
mea culpa and missa something that the subsequent patch points out ;-)
The virDomainSaveImageGetXMLDesc and virDomainManagedSaveGetXMLDesc source code should use:
if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_SAVE_IMAGE_XML_SECURE)) {
and not
if ((conn->flags & VIR_CONNECT_RO) && (flags & VIR_DOMAIN_XML_SECURE)) {
Good spot; will fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 54750dc253..7cca4b72fb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16252,7 +16252,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 cde9e3d417..ce0df1f8e3 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 f99ade82b6..2d2eaf88a6 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
On 2/14/19 4:29 PM, Eric Blake wrote:
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(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
Eric Blake -
John Ferlan