Re: [PATCH v2 2/9] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store

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

 



On 21.01.2015 16:29, Matthias Gatto wrote:
> Uniformize backing store usage by calling virStorageSourceGetBackingStore
> instead of setting backing store manually.
> 
> Signed-off-by: Matthias Gatto <matthias.gatto@xxxxxxxxxxxx>
> ---
>  src/conf/domain_conf.c                |  7 ++++---
>  src/conf/storage_conf.c               | 18 ++++++++++--------
>  src/qemu/qemu_cgroup.c                |  4 ++--
>  src/qemu/qemu_domain.c                |  2 +-
>  src/qemu/qemu_driver.c                | 12 ++++++------
>  src/security/security_dac.c           |  2 +-
>  src/security/security_selinux.c       |  4 ++--
>  src/security/virt-aa-helper.c         |  2 +-
>  src/storage/storage_backend.c         | 30 ++++++++++++++++--------------
>  src/storage/storage_backend_fs.c      | 31 +++++++++++++++++--------------
>  src/storage/storage_backend_gluster.c |  8 +++++---
>  src/storage/storage_backend_logical.c |  6 +++---
>  src/util/virstoragefile.c             | 20 ++++++++++----------
>  tests/virstoragetest.c                | 14 +++++++-------
>  14 files changed, 85 insertions(+), 75 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8792f5e..0668a5b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16704,7 +16704,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
>      /* We currently don't output seclabels for backing chain element */
>      if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
>          virDomainDiskBackingStoreFormat(buf,
> -                                        backingStore->backingStore,
> +                                        virStorageSourceGetBackingStore(backingStore, 0),
>                                          backingStore->backingStoreRaw,
>                                          idx + 1) < 0)
>          return -1;
> @@ -16826,7 +16826,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
>      /* Don't format backingStore to inactive XMLs until the code for
>       * persistent storage of backing chains is ready. */
>      if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
> -        virDomainDiskBackingStoreFormat(buf, def->src->backingStore,
> +        virDomainDiskBackingStoreFormat(buf,
> +                                        virStorageSourceGetBackingStore(def->src, 0),
>                                          def->src->backingStoreRaw, 1) < 0)
>          return -1;
>  
> @@ -20868,7 +20869,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
>          }
>      }
>  
> -    for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
> +    for (tmp = disk->src; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
>          int actualType = virStorageSourceGetActualType(tmp);
>          /* execute the callback only for local storage */
>          if (actualType != VIR_STORAGE_TYPE_NETWORK &&
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index e9aaa8a..f4f7e24 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1339,20 +1339,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>      }
>  
>      if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) {
> +        virStorageSourcePtr backingStorePtr;
>          if (VIR_ALLOC(ret->target.backingStore) < 0)
>              goto error;
>  
> -        ret->target.backingStore->path = backingStore;
> +        backingStorePtr = virStorageSourceGetBackingStore(&ret->target, 0);
> +        backingStorePtr->path = backingStore;
>          backingStore = NULL;
>  
>          if (options->formatFromString) {
>              char *format = virXPathString("string(./backingStore/format/@type)", ctxt);
>              if (format == NULL)
> -                ret->target.backingStore->format = options->defaultFormat;
> +                backingStorePtr->format = options->defaultFormat;
>              else
> -                ret->target.backingStore->format = (options->formatFromString)(format);
> +                backingStorePtr->format = (options->formatFromString)(format);
>  
> -            if (ret->target.backingStore->format < 0) {
> +            if (backingStorePtr->format < 0) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                 _("unknown volume format type %s"), format);
>                  VIR_FREE(format);
> @@ -1361,9 +1363,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>              VIR_FREE(format);
>          }
>  
> -        if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
> +        if (VIR_ALLOC(backingStorePtr->perms) < 0)
>              goto error;
> -        if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
> +        if (virStorageDefParsePerms(ctxt, backingStorePtr->perms,
>                                      "./backingStore/permissions",
>                                      DEFAULT_VOL_PERM_MODE) < 0)
>              goto error;
> @@ -1641,9 +1643,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
>                                       &def->target, "target") < 0)
>          goto cleanup;
>  
> -    if (def->target.backingStore &&
> +    if (virStorageSourceGetBackingStore(&(def->target), 0) &&
>          virStorageVolTargetDefFormat(options, &buf,
> -                                     def->target.backingStore,
> +                                     virStorageSourceGetBackingStore(&(def->target), 0),
>                                       "backingStore") < 0)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index d71ffbc..82b5f2f 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -121,7 +121,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm,
>      virStorageSourcePtr next;
>      bool forceReadonly = false;
>  
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
>          if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0)
>              return -1;
>  
> @@ -139,7 +139,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm,
>  {
>      virStorageSourcePtr next;
>  
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
>          if (qemuSetImageCgroup(vm, next, true) < 0)
>              return -1;
>      }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 99c46d4..68a2a95 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2725,7 +2725,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>      if (virStorageSourceIsEmpty(disk->src))
>          goto cleanup;
>  
> -    if (disk->src->backingStore) {
> +    if (virStorageSourceGetBackingStore(disk->src, 0)) {
>          if (force_probe)
>              virStorageSourceBackingStoreClear(disk->src);
>          else
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5994558..547d2b5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13499,13 +13499,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
>  
>      /* Update vm in place to match changes. */
>      tmp = disk->src;
> -    disk->src = tmp->backingStore;
> +    disk->src = virStorageSourceGetBackingStore(tmp, 0);
>      tmp->backingStore = NULL;
>      virStorageSourceFree(tmp);
>  
>      if (persistDisk) {
>          tmp = persistDisk->src;
> -        persistDisk->src = tmp->backingStore;
> +        persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
>          tmp->backingStore = NULL;
>          virStorageSourceFree(tmp);
>      }
> @@ -15624,7 +15624,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>                  goto endjob;
>              }
>  
> -            if (virStorageFileGetRelativeBackingPath(disk->src->backingStore,
> +            if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0),
>                                                       baseSource,
>                                                       &backingPath) < 0)
>                  goto endjob;
> @@ -16336,7 +16336,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
>          goto endjob;
>      }
>  
> -    if (!topSource->backingStore) {
> +    if (!virStorageSourceGetBackingStore(topSource, 0)) {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("top '%s' in chain for '%s' has no backing file"),
>                         topSource->path, path);
> @@ -16344,14 +16344,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
>      }
>  
>      if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
> -        baseSource = topSource->backingStore;
> +        baseSource = virStorageSourceGetBackingStore(topSource, 0);
>      else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 ||
>               !(baseSource = virStorageFileChainLookup(disk->src, topSource,
>                                                        base, baseIndex, NULL)))
>          goto endjob;
>  
>      if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
> -        baseSource != topSource->backingStore) {
> +        baseSource != virStorageSourceGetBackingStore(topSource, 0)) {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("base '%s' is not immediately below '%s' in chain "
>                           "for '%s'"),
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index deb6980..0c2a226 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -379,7 +379,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr,
>  {
>      virStorageSourcePtr next;
>  
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
>          if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0)
>              return -1;
>      }
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 6e67a86..a1cab9f 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1146,7 +1146,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
>       * be tracked in domain XML, at which point labelskip should be a
>       * per-file attribute instead of a disk attribute. */
>      if (disk_seclabel && disk_seclabel->labelskip &&
> -        !src->backingStore)
> +        !virStorageSourceGetBackingStore(src, 0))
>          return 0;
>  
>      /* Don't restore labels on readonly/shared disks, because other VMs may
> @@ -1276,7 +1276,7 @@ virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr,
>      bool first = true;
>      virStorageSourcePtr next;
>  
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
>          if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, next,
>                                                              first) < 0)
>              return -1;
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index e53779e..7a67071 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -942,7 +942,7 @@ get_files(vahControl * ctl)
>          /* XXX - if we knew the qemu user:group here we could send it in
>           *        so that the open could be re-tried as that user:group.
>           */
> -        if (!disk->src->backingStore) {
> +        if (!virStorageSourceGetBackingStore(disk->src, 0)) {
>              bool probe = ctl->allowDiskFormatProbing;
>              virStorageFileGetMetadata(disk->src, -1, -1, probe, false);
>          }
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index b990a82..4a94d3f 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -822,6 +822,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
>      char *opts = NULL;
>      bool convert = false;
>      bool backing = false;
> +    virStorageSourcePtr backingStore;
>  
>      virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>  
> @@ -873,11 +874,12 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
>  
>      }
>  
> -    if (vol->target.backingStore) {
> +    backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
> +    if (backingStore) {
>          int accessRetCode = -1;
>          char *absolutePath = NULL;
>  
> -        backingType = virStorageFileFormatTypeToString(vol->target.backingStore->format);
> +        backingType = virStorageFileFormatTypeToString(backingStore->format);
>  
>          if (preallocate) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -890,9 +892,9 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
>           * backing store, not really sure what use it serves though, and it
>           * may cause issues with lvm. Untested essentially.
>           */
> -        if (inputvol && inputvol->target.backingStore &&
> -            STRNEQ_NULLABLE(inputvol->target.backingStore->path,
> -                            vol->target.backingStore->path)) {
> +        if (inputvol && virStorageSourceGetBackingStore(&(inputvol->target), 0) &&
> +            STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&(inputvol->target), 0)->path,
> +                            backingStore->path)) {

While this works I'd prefer to use a temporary variable instead of func()->path.

>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("a different backing store cannot be specified."));
>              return NULL;
> @@ -901,24 +903,24 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
>          if (backingType == NULL) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unknown storage vol backing store type %d"),
> -                           vol->target.backingStore->format);
> +                           backingStore->format);
>              return NULL;
>          }
>  
>          /* Convert relative backing store paths to absolute paths for access
>           * validation.
>           */
> -        if ('/' != *(vol->target.backingStore->path) &&
> +        if ('/' != *(backingStore->path) &&
>              virAsprintf(&absolutePath, "%s/%s", pool->def->target.path,
> -                        vol->target.backingStore->path) < 0)
> +                        backingStore->path) < 0)
>              return NULL;
>          accessRetCode = access(absolutePath ? absolutePath
> -                               : vol->target.backingStore->path, R_OK);
> +                               : backingStore->path, R_OK);
>          VIR_FREE(absolutePath);
>          if (accessRetCode != 0) {
>              virReportSystemError(errno,
>                                   _("inaccessible backing store volume %s"),
> -                                 vol->target.backingStore->path);
> +                                 backingStore->path);
>              return NULL;
>          }
>      }
> @@ -959,7 +961,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
>      cmd = virCommandNew(create_tool);
>  
>      convert = !!inputvol;
> -    backing = !inputvol && vol->target.backingStore;
> +    backing = !inputvol && virStorageSourceGetBackingStore(&(vol->target), 0);
>  
>      if (convert)
>          virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL);
> @@ -1086,7 +1088,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
>                         vol->target.format);
>          return -1;
>      }
> -    if (vol->target.backingStore != NULL) {
> +    if (virStorageSourceGetBackingStore(&(vol->target), 0) != NULL) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("copy-on-write image not supported with "
>                           "qcow-create"));
> @@ -1491,8 +1493,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
>                                                      openflags)) < 0)
>          return ret;
>  
> -    if (vol->target.backingStore &&
> -        (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
> +    if (virStorageSourceGetBackingStore(&(vol->target), 0) &&
> +        (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0),
>                                                      updateCapacity,
>                                                      withBlockVolFormat,
>                                                      VIR_STORAGE_VOL_OPEN_DEFAULT |
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 34f2153..56cfc56 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -70,6 +70,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>      int ret = -1;
>      int rc;
>      virStorageSourcePtr meta = NULL;
> +    virStorageSourcePtr backingStore;
>      struct stat sb;
>  
>      if (encryption)
> @@ -96,27 +97,29 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>          goto cleanup;
>  
>      if (meta->backingStoreRaw) {
> -        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
> +        backingStore = virStorageSourceGetBackingStore(target, 0);
> +        if (!(backingStore = virStorageSourceNewFromBacking(meta)))

This won't work. In the preexisting code target->backingStore is modified, while with your change it's left untouched and a local variable is modified instead. I see three possibilities:
1) make virStorageSourceGetBackingStore() return address of src->backingStore. That way the backing store can be modified via: *backingStore = newValue. However, this would require complete rework of your patch.
2) Introduce virStorageSourceGetBackingStoreAddr() which would return the address of src->backingStore and the function would be used just here.
3) access target->backingStore directly.

>              goto cleanup;
>  
> -        target->backingStore->format = backingStoreFormat;
> +        backingStore->format = backingStoreFormat;

This is okay, as the target->backingStore and backingStore points at the same memory address. 

>  
>          /* XXX: Remote storage doesn't play nicely with volumes backed by
>           * remote storage. To avoid trouble, just fake the backing store is RAW
>           * and put the string from the metadata as the path of the target. */
> -        if (!virStorageSourceIsLocalStorage(target->backingStore)) {
> -            virStorageSourceFree(target->backingStore);
> +        if (!virStorageSourceIsLocalStorage(backingStore)) {
> +            virStorageSourceFree(backingStore);
>  
> -            if (VIR_ALLOC(target->backingStore) < 0)
> +            if (VIR_ALLOC(backingStore) < 0)
>                  goto cleanup;
>  
> -            target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> -            target->backingStore->path = meta->backingStoreRaw;
> +            target->backingStore = backingStore;

Aha! So you're going with number 3. Okay. But in that case The first line in the block I've pointed out is useless.

> +            backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> +            backingStore->path = meta->backingStoreRaw;
>              meta->backingStoreRaw = NULL;
> -            target->backingStore->format = VIR_STORAGE_FILE_RAW;
> +            backingStore->format = VIR_STORAGE_FILE_RAW;
>          }
>  
> -        if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) {
> +        if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
>              if ((rc = virStorageFileProbeFormat(target->backingStore->path,

This ^^ should use bare @backingStore, so s/target->// as the target->backingStore can be still NULL here (in case the @backingStore doesn't point to a local storage).

>                                                  -1, -1)) < 0) {
>                  /* If the backing file is currently unavailable or is
> @@ -124,12 +127,12 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>                   * format as RAW and continue. Returning -1 here would
>                   * disable the whole storage pool, making it unavailable for
>                   * even maintenance. */
> -                target->backingStore->format = VIR_STORAGE_FILE_RAW;
> +                backingStore->format = VIR_STORAGE_FILE_RAW;
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("cannot probe backing volume format: %s"),
> -                               target->backingStore->path);
> +                               backingStore->path);
>              } else {
> -                target->backingStore->format = rc;
> +                backingStore->format = rc;
>              }
>          }
>      }

So after reading the whole function, I suggest this diff to be squashed in:

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0857de3..b3bb5f1 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -97,10 +97,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
         goto cleanup;
 
     if (meta->backingStoreRaw) {
-        backingStore = virStorageSourceGetBackingStore(target, 0);
-        if (!(backingStore = virStorageSourceNewFromBacking(meta)))
+        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
             goto cleanup;
 
+        backingStore = virStorageSourceGetBackingStore(target, 0);
         backingStore->format = backingStoreFormat;


> @@ -900,8 +903,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
>          if (vol->target.format == VIR_STORAGE_FILE_DIR)
>              vol->type = VIR_STORAGE_VOL_DIR;
>  
> -        if (vol->target.backingStore) {
> -            ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
> +        if (virStorageSourceGetBackingStore(&(vol->target), 0)) {
> +            ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0),
>                                                                true, false,
>                                                                VIR_STORAGE_VOL_OPEN_DEFAULT));
>              /* If this failed, the backing file is currently unavailable,
> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> index 53e4632..fc48f0f 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -247,6 +247,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
>      char *header = NULL;
>      ssize_t len = VIR_STORAGE_MAX_HEADER;
>      int backingFormat;
> +    virStorageSourcePtr backingStore;
>  
>      *volptr = NULL;
>  
> @@ -302,13 +303,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
>      if (meta->backingStoreRaw) {
>          if (VIR_ALLOC(vol->target.backingStore) < 0)
>              goto cleanup;
> +        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
>  
> -        vol->target.backingStore->path = meta->backingStoreRaw;
> +        backingStore->path = meta->backingStoreRaw;
>  
>          if (backingFormat < 0)
> -            vol->target.backingStore->format = VIR_STORAGE_FILE_RAW;
> +            backingStore->format = VIR_STORAGE_FILE_RAW;
>          else
> -            vol->target.backingStore->format = backingFormat;
> +            backingStore->format = backingFormat;
>          meta->backingStoreRaw = NULL;
>      }
>  
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 8aa68a6..fcec31f 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups,
>          if (VIR_ALLOC(vol->target.backingStore) < 0)
>              goto cleanup;
>  
> -        if (virAsprintf(&vol->target.backingStore->path, "%s/%s",
> +        if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s",
>                          pool->def->target.path, groups[1]) < 0)
>              goto cleanup;
>  
> -        vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
> +        virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
>      }
>  
>      if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
> @@ -764,7 +764,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>      }
>      virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
>                                                      1024));
> -    if (vol->target.backingStore)
> +    if (virStorageSourceGetBackingStore(&(vol->target), 0))
>          virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL);
>      else
>          virCommandAddArg(cmd, pool->def->source.name);
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index dc6cf8f..052debf 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1076,10 +1076,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
>      if (!chain)
>          return 0;
>  
> -    for (tmp = chain; tmp; tmp = tmp->backingStore) {
> +    for (tmp = chain; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
>          /* Break when we hit end of chain; report error if we detected
>           * a missing backing file, infinite loop, or other error */
> -        if (!tmp->backingStore && tmp->backingStoreRaw) {
> +        if (!virStorageSourceGetBackingStore(tmp, 0) && tmp->backingStoreRaw) {
>              if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0)
>                  return -1;
>  
> @@ -1334,8 +1334,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
>      *parent = NULL;
>  
>      if (startFrom) {
> -        while (chain && chain != startFrom->backingStore) {
> -            chain = chain->backingStore;
> +        while (chain && chain != virStorageSourceGetBackingStore(startFrom, 0)) {
> +            chain = virStorageSourceGetBackingStore(chain, 0);
>              i++;
>          }
>          *parent = startFrom;
> @@ -1343,7 +1343,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
>  
>      while (chain) {
>          if (!name && !idx) {
> -            if (!chain->backingStore)
> +            if (!virStorageSourceGetBackingStore(chain, 0))
>                  break;
>          } else if (idx) {
>              VIR_DEBUG("%zu: %s", i, chain->path);
> @@ -1378,7 +1378,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
>              }
>          }
>          *parent = chain;
> -        chain = chain->backingStore;
> +        chain = virStorageSourceGetBackingStore(chain, 0);
>          i++;
>      }
>  
> @@ -1880,8 +1880,8 @@ virStorageSourceCopy(const virStorageSource *src,
>          !(ret->auth = virStorageAuthDefCopy(src->auth)))
>          goto error;
>  
> -    if (backingChain && src->backingStore) {
> -        if (!(ret->backingStore = virStorageSourceCopy(src->backingStore,
> +    if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
> +        if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
>                                                         true)))
>              goto error;
>      }
> @@ -2018,7 +2018,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
>      VIR_FREE(def->backingStoreRaw);
>  
>      /* recursively free backing chain */
> -    virStorageSourceFree(def->backingStore);
> +    virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
>      def->backingStore = NULL;
>  }
>  
> @@ -2832,7 +2832,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top,
>  
>      *relpath = NULL;
>  
> -    for (next = top; next; next = next->backingStore) {
> +    for (next = top; next; next = virStorageSourceGetBackingStore(next, 0)) {
>          if (!next->relPath) {
>              ret = 1;
>              goto cleanup;
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index 2601edc..12670da 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -403,7 +403,7 @@ testStorageChain(const void *args)
>          }
>          VIR_FREE(expect);
>          VIR_FREE(actual);
> -        elt = elt->backingStore;
> +        elt = virStorageSourceGetBackingStore(elt, 0);
>          i++;
>      }
>      if (i != data->nfiles) {
> @@ -1029,8 +1029,8 @@ mymain(void)
>          ret = -1;
>          goto cleanup;
>      }
> -    chain2 = chain->backingStore;
> -    chain3 = chain2->backingStore;
> +    chain2 = virStorageSourceGetBackingStore(chain, 0);
> +    chain3 = virStorageSourceGetBackingStore(chain2, 0);
>  
>  #define TEST_LOOKUP_TARGET(id, target, from, name, index, result,       \
>                             meta, parent)                                \
> @@ -1095,8 +1095,8 @@ mymain(void)
>          ret = -1;
>          goto cleanup;
>      }
> -    chain2 = chain->backingStore;
> -    chain3 = chain2->backingStore;
> +    chain2 = virStorageSourceGetBackingStore(chain, 0);
> +    chain3 = virStorageSourceGetBackingStore(chain2, 0);
>  
>      TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL);
>      TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL);
> @@ -1142,8 +1142,8 @@ mymain(void)
>          ret = -1;
>          goto cleanup;
>      }
> -    chain2 = chain->backingStore;
> -    chain3 = chain2->backingStore;
> +    chain2 = virStorageSourceGetBackingStore(chain, 0);
> +    chain3 = virStorageSourceGetBackingStore(chain2, 0);
>  
>      TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL);
>      TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL);
> 

There are some other issues I've found, so ACK with this squashed in:

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index a768290..7fcff09 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -931,9 +931,13 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
     if (backingStore) {
         int accessRetCode = -1;
         char *absolutePath = NULL;
+        virStorageSourcePtr targetBackingStore = NULL;
 
         backingType = virStorageFileFormatTypeToString(backingStore->format);
 
+        if (inputvol)
+            targetBackingStore = virStorageSourceGetBackingStore(&inputvol->target, 0);
+
         if (preallocate) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("metadata preallocation conflicts with backing"
@@ -945,9 +949,8 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
          * backing store, not really sure what use it serves though, and it
          * may cause issues with lvm. Untested essentially.
          */
-        if (inputvol && virStorageSourceGetBackingStore(&(inputvol->target), 0) &&
-            STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&(inputvol->target), 0)->path,
-                            backingStore->path)) {
+        if (inputvol && targetBackingStore &&
+            STRNEQ_NULLABLE(targetBackingStore->path, backingStore->path)) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("a different backing store cannot be specified."));
             return NULL;
@@ -1014,7 +1017,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
     cmd = virCommandNew(create_tool);
 
     convert = !!inputvol;
-    backing = !inputvol && virStorageSourceGetBackingStore(&(vol->target), 0);
+    backing = !inputvol && backingStore;
 
     if (convert)
         virCommandAddArgList(cmd, "convert", "-f", inputType, "-O", type, NULL);
@@ -1022,7 +1025,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn,
         virCommandAddArgList(cmd, "create", "-f", type, NULL);
 
     if (backing)
-        virCommandAddArgList(cmd, "-b", vol->target.backingStore->path, NULL);
+        virCommandAddArgList(cmd, "-b", backingStore->path, NULL);
 
     if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS) {
         if (vol->target.format == VIR_STORAGE_FILE_QCOW2 && !compat &&
@@ -1539,6 +1542,7 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
                                unsigned int openflags)
 {
     int ret;
+    virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
 
     if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target,
                                                     updateCapacity,
@@ -1546,12 +1550,12 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
                                                     openflags)) < 0)
         return ret;
 
-    if (virStorageSourceGetBackingStore(&(vol->target), 0) &&
-        (ret = virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0),
+    if (backingStore &&
+        (ret = virStorageBackendUpdateVolTargetInfo(backingStore,
                                                     updateCapacity,
                                                     withBlockVolFormat,
                                                     VIR_STORAGE_VOL_OPEN_DEFAULT |
-                                                    VIR_STORAGE_VOL_OPEN_NOERROR) < 0))
+                                                    VIR_STORAGE_VOL_OPEN_NOERROR)) < 0)
         return ret;
 
     return 0;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0857de3..6c922e4 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -97,10 +97,10 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
         goto cleanup;
 
     if (meta->backingStoreRaw) {
-        backingStore = virStorageSourceGetBackingStore(target, 0);
-        if (!(backingStore = virStorageSourceNewFromBacking(meta)))
+        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
             goto cleanup;
 
+        backingStore = virStorageSourceGetBackingStore(target, 0);
         backingStore->format = backingStoreFormat;
 
         /* XXX: Remote storage doesn't play nicely with volumes backed by
@@ -863,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
 
     while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) {
         int ret;
+        virStorageSourcePtr backingStore;
 
         if (VIR_ALLOC(vol) < 0)
             goto error;
@@ -903,8 +904,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
         if (vol->target.format == VIR_STORAGE_FILE_DIR)
             vol->type = VIR_STORAGE_VOL_DIR;
 
-        if (virStorageSourceGetBackingStore(&(vol->target), 0)) {
-            ignore_value(virStorageBackendUpdateVolTargetInfo(virStorageSourceGetBackingStore(&(vol->target), 0),
+        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+        if (backingStore) {
+            ignore_value(virStorageBackendUpdateVolTargetInfo(backingStore,
                                                               true, false,
                                                               VIR_STORAGE_VOL_OPEN_DEFAULT));
             /* If this failed, the backing file is currently unavailable,
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index fcec31f..a383454 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -148,14 +148,17 @@ virStorageBackendLogicalMakeVol(char **const groups,
      *  lv is created with "--virtualsize").
      */
     if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) {
+        virStorageSourcePtr backingStore;
+
         if (VIR_ALLOC(vol->target.backingStore) < 0)
             goto cleanup;
 
-        if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s",
+        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+        if (virAsprintf(&backingStore->path, "%s/%s",
                         pool->def->target.path, groups[1]) < 0)
             goto cleanup;
 
-        virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
+        backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
     }
 
     if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)


Michal

--
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]