[libvirt] [PATCH v7 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store

John Ferlan jferlan at redhat.com
Tue Dec 15 00:44:21 UTC 2015



On 12/03/2015 09:35 AM, Matthias Gatto wrote:
> Uniformize backing store usage by calling virStorageSourceGetBackingStore
> instead of setting backing store manually.
> 
> Signed-off-by: Matthias Gatto <matthias.gatto at outscale.com>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/domain_conf.c                |  7 ++++---
>  src/conf/storage_conf.c               |  6 +++---
>  src/qemu/qemu_cgroup.c                |  4 ++--
>  src/qemu/qemu_domain.c                |  2 +-
>  src/qemu/qemu_driver.c                | 18 +++++++++--------
>  src/qemu/qemu_monitor_json.c          |  4 +++-
>  src/security/security_dac.c           |  2 +-
>  src/security/security_selinux.c       |  4 ++--
>  src/security/virt-aa-helper.c         |  2 +-
>  src/storage/storage_backend.c         | 22 ++++++++++++--------
>  src/storage/storage_backend_fs.c      | 38 ++++++++++++++++++++---------------
>  src/storage/storage_backend_gluster.c |  8 +++++---
>  src/storage/storage_backend_logical.c | 12 +++++++----
>  src/util/virstoragefile.c             | 20 +++++++++---------
>  tests/virstoragetest.c                | 14 ++++++-------
>  15 files changed, 93 insertions(+), 70 deletions(-)
> 

storage_backend_fs.c needed a slight merge with some changes I made...

There's also still a bunch of long lines. I've noted them below.

For each of them I propose adjusting the code a bit to do something like:

    virStorageSourcePtr bsp;
...
    bsp = virStorageSourceGetBackingStore(backingStore, 0);

then use bsp instead.

I'll add a "merge" patch to this response - it probably won't apply
cleanly since I had the merge issue to deal with first (in
storage_backend_fs.c - virStorageBackendFileSystemRefresh and call to
virStorageBackendUpdateVolTargetInfo). But I think if you update to top
of tree and then apply, a git am should work


John

I only worked on/through the first 2 patches, but can continue through
patch 5 - that'll at least get part of this series in


> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e6102a0..5b413b5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18625,7 +18625,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),

Long line

>                                          backingStore->backingStoreRaw,
>                                          idx + 1) < 0)
>          return -1;
> @@ -18746,7 +18746,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),

Long line

>                                          def->src->backingStoreRaw, 1) < 0)
>          return -1;
>  
> @@ -22714,7 +22715,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 9b8abea..d048e39 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1330,7 +1330,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>          if (virStorageSize(unit, capacity, &ret->target.capacity) < 0)
>              goto error;
>      } else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) &&
> -               !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && ret->target.backingStore)) {
> +               !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) && virStorageSourceGetBackingStore(&ret->target, 0))) {

Really long line

>          virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element"));
>          goto error;
>      }
> @@ -1644,9 +1644,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),

long line

>                                       "backingStore") < 0)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index b52ce3a..6405944 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)) {

long line

>          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)) {

long line

>          if (qemuSetImageCgroup(vm, next, true) < 0)
>              return -1;
>      }
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ed21245..28bbe3f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3101,7 +3101,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 ae1d8e7..8ba7566 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14309,13 +14309,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);
>      }
> @@ -16309,7 +16309,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
>                  goto endjob;
>              }
>  
> -            if (virStorageFileGetRelativeBackingPath(disk->src->backingStore,
> +            if (virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0),

long line

>                                                       baseSource,
>                                                       &backingPath) < 0)
>                  goto endjob;
> @@ -16662,6 +16662,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>      virQEMUDriverConfigPtr cfg = NULL;
>      const char *format = NULL;
>      int desttype = virStorageSourceGetActualType(mirror);
> +    virStorageSourcePtr backingStore;
>  
>      /* Preliminaries: find the disk we are editing, sanity checks */
>      virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
> @@ -16705,8 +16706,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>      if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
>          goto endjob;
>  
> +    backingStore = virStorageSourceGetBackingStore(disk->src, 0);
>      /* clear the _SHALLOW flag if there is only one layer */
> -    if (!disk->src->backingStore)
> +    if (!backingStore)
>          flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW;
>  
>      /* unless the user provides a pre-created file, shallow copy into a raw
> @@ -17113,7 +17115,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);
> @@ -17121,14 +17123,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'"),
> @@ -19406,7 +19408,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>                  goto cleanup;
>              visited++;
>              backing_idx++;
> -            src = src->backingStore;
> +            src = virStorageSourceGetBackingStore(src, 0);
>          }
>      }
>  
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 86b8c7b..790d7bc 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3785,7 +3785,9 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image,
>          return NULL;
>      if (top != target) {
>          backing = virJSONValueObjectGetObject(image, "backing-image");
> -        return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore,
> +        virStorageSourcePtr backingStore =
> +            virStorageSourceGetBackingStore(top, 0);
> +        return qemuMonitorJSONDiskNameLookupOne(backing, backingStore,
>                                                  target);
>      }
>      if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) < 0)
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index cdde34e..7fdf40f 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -454,7 +454,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr,
>  {
>      virStorageSourcePtr next;
>  
> -    for (next = disk->src; next; next = next->backingStore) {
> +    for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {

long line

>          if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0)
>              return -1;
>      }
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index b8ebdcc..5d694d8 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1216,7 +1216,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
> @@ -1350,7 +1350,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)) {

long line

>          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 5de56e5..dddde1c 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -967,7 +967,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 194736b..08ed1dd 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -940,6 +940,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>          .features = vol->target.features,
>          .nocow = vol->target.nocow,
>      };
> +    virStorageSourcePtr backingStore;
>  
>      virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
>  
> @@ -988,12 +989,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
>          }
>      }
>  
> -    if (vol->target.backingStore) {
> +    backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
> +    if (backingStore) {
>          int accessRetCode = -1;
>          char *absolutePath = NULL;
>  
> -        info.backingFormat = vol->target.backingStore->format;
> -        info.backingPath = vol->target.backingStore->path;
> +        info.backingFormat = backingStore->format;
> +        info.backingPath = backingStore->path;
>  
>          if (info.preallocate) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -1006,8 +1008,10 @@ virStorageBackendCreateQemuImgCmdFromVol(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, info.backingPath)) {
> +        if (inputvol &&
> +            virStorageSourceGetBackingStore(&inputvol->target, 0) &&
> +            STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&inputvol->target, 0)->path,
> +                            info.backingPath)) {

long line

This is also another one of those cases where using a temp variable is
going to help.

>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("a different backing store cannot be specified."));
>              return NULL;
> @@ -1197,7 +1201,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"));
> @@ -1639,14 +1643,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
>                                 unsigned int openflags)
>  {
>      int ret;
> +    virStorageSourcePtr backingStore;
>  
>      if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target,
>                                                      withBlockVolFormat,
>                                                      openflags)) < 0)
>          return ret;
>  
> -    if (vol->target.backingStore &&
> -        (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
> +    backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
> +    if (backingStore &&
> +        (ret = virStorageBackendUpdateVolTargetInfo(backingStore,
>                                                      withBlockVolFormat,
>                                                      VIR_STORAGE_VOL_OPEN_DEFAULT |
>                                                      VIR_STORAGE_VOL_OPEN_NOERROR) < 0))
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 99ea394..7b05d4d 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)
> @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>          if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
>              goto cleanup;
>  
> -        target->backingStore->format = backingStoreFormat;
> +        backingStore = virStorageSourceGetBackingStore(target, 0);
> +        backingStore->format = backingStoreFormat;
>  
>          /* 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);

Although you cleaned up the reference Peter noted before - there's
double free problem here if the VIR_ALLOC fails. Currently backingStore
is just the local copy of target->backingStore.  If we free it here,
then target->backingStore still contains the address/pointer. If we jump
to cleanup, then target->backingStore could be free'd again

Perhaps one way around this is to do the 'set' code first, but I think
at least the following would be OK after the SourceFree

               target->backingStore = NULL;
>  
> -            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;
> +            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 ((rc = virStorageFileProbeFormat(target->backingStore->path,
> +        if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> +            if ((rc = virStorageFileProbeFormat(backingStore->path,
>                                                  -1, -1)) < 0) {
>                  /* If the backing file is currently unavailable or is
>                   * accessed via remote protocol only log an error, fake the
>                   * 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;
>              }
>          }
>      }
> @@ -860,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
>      struct stat statbuf;
>      virStorageVolDefPtr vol = NULL;
>      virStorageSourcePtr target = NULL;
> +    virStorageSourcePtr backingStore;
>      int direrr;
>      int fd = -1, ret = -1;
>  
> @@ -918,10 +922,12 @@ 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,
> -                                                              false,
> -                                                              VIR_STORAGE_VOL_OPEN_DEFAULT));
> +        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
> +        if (backingStore) {
> +            ignore_value(virStorageBackendUpdateVolTargetInfo(
> +                             backingStore,
> +                             false,
> +                             VIR_STORAGE_VOL_OPEN_DEFAULT));
>              /* If this failed, the backing file is currently unavailable,
>               * the capacity, allocation, owner, group and mode are unknown.
>               * An error message was raised, but we just continue. */
> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> index d2e79bc..9bddb3b 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 536e617..e2a287d 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -88,6 +88,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
>      size_t i;
>      int err, nextents, nvars, ret = -1;
>      const char *attrs = groups[9];
> +    virStorageSourcePtr backingStore;
>  
>      /* Skip inactive volume */
>      if (attrs[4] != 'a')
> @@ -151,11 +152,12 @@ virStorageBackendLogicalMakeVol(char **const groups,
>          if (VIR_ALLOC(vol->target.backingStore) < 0)
>              goto cleanup;
>  
> -        if (virAsprintf(&vol->target.backingStore->path, "%s/%s",
> +        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);

I see you fixed up the references here, but we still have a problem if
backingStore == NULL (now *technically* I know that cannot happen yet,
but at some point in the future it could.

So what should be done if backingStore == NULL here? Something like:

        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
        if (backingStore) {
            if (virAsprintf(&backingStore->path, "%s/%s",
                            pool->def->target.path, groups[1]) < 0)
                goto cleanup;

            backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
        }

> +        if (virAsprintf(&backingStore->path, "%s/%s",
>                          pool->def->target.path, groups[1]) < 0)
>              goto cleanup;
>  
> -        vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
> +        backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
>      }
>  
>      if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
> @@ -732,6 +734,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>      virErrorPtr err;
>      struct stat sb;
>      bool created = false;
> +    virStorageSourcePtr backingStore;
>  
>      if (vol->target.encryption != NULL) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> @@ -764,8 +767,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>      }
>      virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
>                                                      1024));
> -    if (vol->target.backingStore)
> -        virCommandAddArgList(cmd, "-s", vol->target.backingStore->path, NULL);
> +    backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
> +    if (backingStore)
> +        virCommandAddArgList(cmd, "-s", backingStore->path, NULL);
>      else
>          virCommandAddArg(cmd, pool->def->source.name);
>  
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 016beaa..af17d82 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++;
>          }
>  
> @@ -1352,7 +1352,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);
> @@ -1387,7 +1387,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
>              }
>          }
>          *parent = chain;
> -        chain = chain->backingStore;
> +        chain = virStorageSourceGetBackingStore(chain, 0);
>          i++;
>      }
>  
> @@ -1893,8 +1893,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)))

long line

>              goto error;
>      }
> @@ -2035,7 +2035,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
>      VIR_FREE(def->backingStoreRaw);
>  
>      /* recursively free backing chain */
> -    virStorageSourceFree(def->backingStore);
> +    virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
>      def->backingStore = NULL;
>  }
>  
> @@ -2898,7 +2898,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 38ce09e..5bd4637 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) {
> @@ -1044,8 +1044,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)                                \
> @@ -1110,8 +1110,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);
> @@ -1157,8 +1157,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);
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-merge.patch
Type: text/x-patch
Size: 11695 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151214/debd9ec2/attachment-0001.bin>


More information about the libvir-list mailing list