Re: [PATCH] qemu: don't regard cur_balloon in supplied xml in migrate and save

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]