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