On Wed, May 25, 2016 at 16:29:25 +0300, Nikolay Shirokovskiy wrote:

cur_balloon value can change in between preparing external config and using it in operations like save and migrate. As a resutl operation will fail for ABI inconsistency. cur_balloon changes can not be predicted generally and thus operations will fail from time to time.

Skip this check if domain lock can not be hold between preparing external config outside of libvirt and checking it against active config. Insted set cur_balloon value in external config from active config. This way it is protected from forges and is keeped up to date too.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 8 +++++++- src/conf/snapshot_conf.c | 2 +- src/libxl/libxl_domain.c | 2 +- src/qemu/qemu_domain.c | 14 +++++++++----- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 13 +++++++++---- src/qemu/qemu_migration.c | 7 +++++-- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- tests/qemuargv2xmltest.c | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/sexpr2xmltest.c | 2 +- tests/testutils.c | 2 +- tests/vmx2xmltest.c | 2 +- tests/xlconfigtest.c | 2 +- tests/xmconfigtest.c | 2 +- tests/xml2sexprtest.c | 2 +- tests/xml2vmxtest.c | 2 +- 19 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fb05bf7..9afba64 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18090,7 +18090,8 @@ virDomainDefVcpuCheckAbiStability(virDomainDefPtr src, */ bool virDomainDefCheckABIStability(virDomainDefPtr src, - virDomainDefPtr dst) + virDomainDefPtr dst, + unsigned int flags) {

Please rename this function and make the original name a wrapper that will call this with flags = 0.

size_t i; virErrorPtr err; @@ -18134,7 +18135,8 @@ virDomainDefCheckABIStability(virDomainDefPtr src, virDomainDefGetMemoryInitial(src)); goto error; } - if (src->mem.cur_balloon != dst->mem.cur_balloon) { + if (!(flags & VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE) && + src->mem.cur_balloon != dst->mem.cur_balloon) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain current memory %lld does not match source %lld"), dst->mem.cur_balloon, src->mem.cur_balloon); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 82e581b..d90e427 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2591,6 +2591,11 @@ typedef enum { VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 9, } virDomainDefFormatFlags;

+typedef enum { + /* skip checking values like cur_balloon that can be changed meanwhile */ + VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE = 1 << 0, +} virDomainDefABICheckFlags; + virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, const virDomainDef *def, virCapsPtr caps, @@ -2624,7 +2629,8 @@ virDomainObjPtr virDomainObjParseFile(const char *filename, unsigned int flags);

bool virDomainDefCheckABIStability(virDomainDefPtr src, - virDomainDefPtr dst); + virDomainDefPtr dst, + unsigned int flags);

int virDomainDefAddImplicitDevices(virDomainDefPtr def);

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c7794e9..2778bef 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1287,7 +1287,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, if (other->def->dom) { if (def->dom) { if (!virDomainDefCheckABIStability(other->def->dom, - def->dom)) + def->dom, 0)) goto cleanup; } else { /* Transfer the domain def */ diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 113942b..02bce4a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1248,7 +1248,7 @@ libxlDomainDefCheckABIStability(libxlDriverPrivatePtr driver, !(migratableDefDst = virDomainDefCopy(dst, cfg->caps, driver->xmlopt, true))) goto cleanup;

- ret = virDomainDefCheckABIStability(migratableDefSrc, migratableDefDst); + ret = virDomainDefCheckABIStability(migratableDefSrc, migratableDefDst, 0);

cleanup: virDomainDefFree(migratableDefSrc); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c21465d..70dde48 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4539,18 +4539,22 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver, bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr src, - virDomainDefPtr dst) + virDomainDefPtr dst, + unsigned int flags) {

As the balloon in the qemu driver does always allow to change the size and thus the value is not ABI I think we can always call it in the qemu driver with the flag to skip the check, so adding a new parameter isn't necessary.

virDomainDefPtr migratableDefSrc = NULL; virDomainDefPtr migratableDefDst = NULL; - const int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_UPDATE_CPU | VIR_DOMAIN_XML_MIGRATABLE; + const int copy_flags = VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_UPDATE_CPU | + VIR_DOMAIN_XML_MIGRATABLE; bool ret = false;

- if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, flags)) || - !(migratableDefDst = qemuDomainDefCopy(driver, dst, flags))) + + if (!(migratableDefSrc = qemuDomainDefCopy(driver, src, copy_flags)) || + !(migratableDefDst = qemuDomainDefCopy(driver, dst, copy_flags))) goto cleanup;

- ret = virDomainDefCheckABIStability(migratableDefSrc, migratableDefDst); + ret = virDomainDefCheckABIStability(migratableDefSrc, migratableDefDst, flags);

cleanup: virDomainDefFree(migratableDefSrc); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f99f0bb..8e5ae9c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -563,7 +563,8 @@ int qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver,

bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver, virDomainDefPtr src, - virDomainDefPtr dst); + virDomainDefPtr dst, + unsigned int flags);

bool qemuDomainAgentAvailable(virDomainObjPtr vm, bool reportError); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 249393a..5e87c46 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3230,10 +3230,15 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, VIR_DOMAIN_DEF_PARSE_INACTIVE))) { goto endjob; } - if (!qemuDomainDefCheckABIStability(driver, vm->def, def)) { + + if (!qemuDomainDefCheckABIStability(driver, vm->def, def, + VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE)) { virDomainDefFree(def); goto endjob; } + + def->mem.cur_balloon = vm->def->mem.cur_balloon; + xml = qemuDomainDefFormatLive(driver, def, true, true); } else { xml = qemuDomainDefFormatLive(driver, vm->def, true, true); @@ -6267,7 +6272,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, VIR_DOMAIN_XML_MIGRATABLE))) goto cleanup;

- if (!virDomainDefCheckABIStability(def, newdef_migr)) { + if (!virDomainDefCheckABIStability(def, newdef_migr, 0)) { virResetLastError();

/* Due to a bug in older version of external snapshot creation @@ -6276,7 +6281,7 @@ qemuDomainSaveImageUpdateDef(virQEMUDriverPtr driver, * saved XML type, we need to check the ABI compatibility against * the user provided XML if the check against the migratable XML * fails. Snapshots created prior to v1.1.3 have this issue. */ - if (!virDomainDefCheckABIStability(def, newdef)) + if (!virDomainDefCheckABIStability(def, newdef, 0)) goto cleanup;

/* use the user provided XML */ @@ -15487,7 +15492,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Transitions 5, 6, 8, 9 */ /* Check for ABI compatibility. We need to do this check against * the migratable XML or it will always fail otherwise */ - if (config && !qemuDomainDefCheckABIStability(driver, vm->def, config)) { + if (config && !qemuDomainDefCheckABIStability(driver, vm->def, config, 0)) { virErrorPtr err = virGetLastError();

if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 25093ac..892f8e8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3211,9 +3211,12 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto cleanup;

- if (!qemuDomainDefCheckABIStability(driver, vm->def, def)) + if (!qemuDomainDefCheckABIStability(driver, vm->def, def, + VIR_DOMAIN_DEF_ABI_CHECK_SKIP_VOLATILE)) goto cleanup;

+ def->mem.cur_balloon = vm->def->mem.cur_balloon; + rv = qemuDomainDefFormatLive(driver, def, false, true); } else { rv = qemuDomainDefFormatLive(driver, vm->def, false, true); @@ -3555,7 +3558,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (!newdef) goto cleanup;

- if (!qemuDomainDefCheckABIStability(driver, *def, newdef)) { + if (!qemuDomainDefCheckABIStability(driver, *def, newdef, 0)) { virDomainDefFree(newdef); goto cleanup; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a51eb09..de0c567 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6477,7 +6477,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (virDomainObjIsActive(vm)) { /* Transitions 5, 6, 8, 9 */ /* Check for ABI compatibility. */ - if (!virDomainDefCheckABIStability(vm->def, config)) { + if (!virDomainDefCheckABIStability(vm->def, config, 0)) { virErrorPtr err = virGetLastError();

if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 177a57a..cfc2b08 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -787,7 +787,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) * So forbid this operation, if config is changed. If it's * not changed - just do nothing. */

- if (!virDomainDefCheckABIStability(olddom->def, def)) { + if (!virDomainDefCheckABIStability(olddom->def, def, 0)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Can't change domain configuration " "in managed save state")); diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 0fe6b9b..9f8c740 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -95,7 +95,7 @@ static int testCompareXMLToArgvFiles(const char *xmlfile, if (testSanitizeDef(vmdef) < 0) goto fail;

- if (!virDomainDefCheckABIStability(vmdef, vmdef)) { + if (!virDomainDefCheckABIStability(vmdef, vmdef, 0)) { VIR_TEST_DEBUG("ABI stability check failed on %s", xmlfile); goto fail; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7bf9300..1a9d6bb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -285,7 +285,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (virBitmapParse("0-3", '\0', &priv->autoNodeset, 4) < 0) goto out;

- if (!virDomainDefCheckABIStability(vm->def, vm->def)) { + if (!virDomainDefCheckABIStability(vm->def, vm->def, 0)) { VIR_TEST_DEBUG("ABI stability check failed on %s", xml); goto out; } diff --git a/tests/sexpr2xmltest.c b/tests/sexpr2xmltest.c index a4b0452..9d86620 100644 --- a/tests/sexpr2xmltest.c +++ b/tests/sexpr2xmltest.c @@ -57,7 +57,7 @@ testCompareFiles(const char *xml, const char *sexpr) tty, vncport, caps, xmlopt))) goto fail;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", xml); goto fail; } diff --git a/tests/testutils.c b/tests/testutils.c index f4fbad2..6b753b9 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1122,7 +1122,7 @@ testCompareDomXML2XMLFiles(virCapsPtr caps, virDomainXMLOptionPtr xmlopt, goto out; }

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { VIR_TEST_DEBUG("ABI stability check failed on %s", infile); result = TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_STABILITY; goto out; diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index efd6325..2992edc 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -81,7 +81,7 @@ testCompareFiles(const char *vmx, const char *xml) if (!(def = virVMXParseConfig(&ctx, xmlopt, caps, vmxData))) goto cleanup;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", vmx); goto cleanup; } diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index 6819bad..3456445 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -67,7 +67,7 @@ testCompareParseXML(const char *xlcfg, const char *xml) VIR_DOMAIN_XML_INACTIVE))) goto fail;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", xml); goto fail; } diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index 9b21a13..de760aa 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -67,7 +67,7 @@ testCompareParseXML(const char *xmcfg, const char *xml) VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto fail;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", xml); goto fail; } diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index 66503b7..d24d787 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -31,7 +31,7 @@ testCompareFiles(const char *xml, const char *sexpr) VIR_DOMAIN_DEF_PARSE_INACTIVE))) goto fail;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", xml); goto fail; } diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index d970240..f14e992 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -82,7 +82,7 @@ testCompareFiles(const char *xml, const char *vmx, int virtualHW_version) if (def == NULL) goto failure;

- if (!virDomainDefCheckABIStability(def, def)) { + if (!virDomainDefCheckABIStability(def, def, 0)) { fprintf(stderr, "ABI stability check failed on %s", xml); goto failure; }

With the wrapper and implicit use in qemu a lot of this stuff can be dropped. Otherwise this change makes sense to me. Peter