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

Re: [libvirt] [PATCH v5 4/9] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store



On Mon, May 4, 2015 at 7:21 PM, John Ferlan <jferlan redhat com> wrote:
>
>
> On 04/23/2015 08:41 AM, Matthias Gatto wrote:
>> Replace the parts of the code where a backing store is set manually
>> with virStorageSourceSetBackingStore
>>
>> Signed-off-by: Matthias Gatto <matthias gatto outscale com>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/conf/domain_conf.c                |  3 ++-
>>  src/conf/storage_conf.c               | 17 ++++++++++-------
>>  src/qemu/qemu_driver.c                | 17 +++++++++++------
>>  src/storage/storage_backend_fs.c      |  7 +++++--
>>  src/storage/storage_backend_gluster.c |  5 +++--
>>  src/storage/storage_backend_logical.c |  9 ++++++---
>>  src/storage/storage_driver.c          |  3 ++-
>>  src/util/virstoragefile.c             |  8 +++++---
>>  tests/virstoragetest.c                |  4 ++--
>>  9 files changed, 46 insertions(+), 27 deletions(-)
>>
>
> Other than a minor goto error issue in storage_backend_gluster.c - up
> through here things seem fine to me.  Doesn't seem to be any new readers
> or setters of "->backingStore" in recent changes (since patches 5-9
> compile too). The only references are now ->backingStoreRaw fetches and
> saves.
>
> However, for patches 5-9 as I understand it have some concerns from
> Peter which hopefully he can elaborate on.
>
> So that progress can be made and readers/writers of backingStore go
> through the virStorageSource{Get|Set}BackingStore API's until the rest
> of the concerns are understood - I'm of the opinion the patches 1-4 can
> be pushed, but I'll wait until Peter chimes in before doing so...
>
> John
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 2a05291..09f0bca 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6006,7 +6006,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
>>          virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
>>          goto cleanup;
>>
>> -    src->backingStore = backingStore;
>> +    if (!virStorageSourceSetBackingStore(src, backingStore, 0))
>> +        goto cleanup;
>>      ret = 0;
>>
>>   cleanup:
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 5781374..ca3a6d5 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -1254,6 +1254,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>>      char *capacity = NULL;
>>      char *unit = NULL;
>>      char *backingStore = NULL;
>> +    virStorageSourcePtr backingStorePtr;
>>      xmlNodePtr node;
>>      xmlNodePtr *nodes = NULL;
>>      size_t i;
>> @@ -1290,20 +1291,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>>      }
>>
>>      if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) {
>> -        if (VIR_ALLOC(ret->target.backingStore) < 0)
>> +        if (VIR_ALLOC(backingStorePtr) < 0)
>>              goto error;
>>
>> -        ret->target.backingStore->path = backingStore;
>> +        if (!virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0))
>> +            goto error;
>> +        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);
>> @@ -1312,9 +1315,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;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 26be9d6..1a98601 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -14274,13 +14274,18 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>>      /* Update vm in place to match changes.  */
>>      need_unlink = false;
>>
>> -    newDiskSrc->backingStore = disk->src;
>> -    disk->src = newDiskSrc;
>> +    if (!virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0)) {
>> +        ret = -1;
>> +        goto cleanup;
>> +    }
>>      newDiskSrc = NULL;
>>
>>      if (persistDisk) {
>> -        persistDiskSrc->backingStore = persistDisk->src;
>> -        persistDisk->src = persistDiskSrc;
>> +        if (!virStorageSourceSetBackingStore(persistDiskSrc,
>> +                                             persistDisk->src, 0)) {
>> +            ret = -1;
>> +            goto cleanup;
>> +        }
>>          persistDiskSrc = NULL;
>>      }
>>
>> @@ -14323,13 +14328,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
>>      /* Update vm in place to match changes. */
>>      tmp = disk->src;
>>      disk->src = virStorageSourceGetBackingStore(tmp, 0);
>> -    tmp->backingStore = NULL;
>> +    ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
>>      virStorageSourceFree(tmp);
>>
>>      if (persistDisk) {
>>          tmp = persistDisk->src;
>>          persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
>> -        tmp->backingStore = NULL;
>> +        ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
>>          virStorageSourceFree(tmp);
>>      }
>>  }
>> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
>> index 151af47..bab2569 100644
>> --- a/src/storage/storage_backend_fs.c
>> +++ b/src/storage/storage_backend_fs.c
>> @@ -97,7 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>>          goto cleanup;
>>
>>      if (meta->backingStoreRaw) {
>> -        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
>> +        if (!virStorageSourceSetBackingStore(target,
>> +                                             virStorageSourceNewFromBacking(meta),
>> +                                             0))
>>              goto cleanup;
>>
>>          backingStore = virStorageSourceGetBackingStore(target, 0);
>> @@ -112,7 +114,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
>>              if (VIR_ALLOC(backingStore) < 0)
>>                  goto cleanup;
>>
>> -            target->backingStore = backingStore;
>> +            if (!virStorageSourceSetBackingStore(target, backingStore, 0))
>> +                goto cleanup;
>>              backingStore->type = VIR_STORAGE_TYPE_NETWORK;
>>              backingStore->path = meta->backingStoreRaw;
>>              meta->backingStoreRaw = NULL;
>> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
>> index 9bddb3b..f0a3b9b 100644
>> --- a/src/storage/storage_backend_gluster.c
>> +++ b/src/storage/storage_backend_gluster.c
>> @@ -301,9 +301,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
>>          goto cleanup;
>>
>>      if (meta->backingStoreRaw) {
>> -        if (VIR_ALLOC(vol->target.backingStore) < 0)
>> +        if (VIR_ALLOC(backingStore) < 0)
>>              goto cleanup;
>> -        backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
>> +        if (!virStorageSourceSetBackingStore(&vol->target, backingStore, 0))
>> +            goto error;
>
> Should be goto cleanup - build failure otherwise
>
>>
>>          backingStore->path = meta->backingStoreRaw;
>>
>> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
>> index 27fdc64..16d508e 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')
>> @@ -148,14 +149,16 @@ virStorageBackendLogicalMakeVol(char **const groups,
>>       *  lv is created with "--virtualsize").
>>       */
>>      if (groups[1] && !STREQ(groups[1], "") && (groups[1][0] != '[')) {
>> -        if (VIR_ALLOC(vol->target.backingStore) < 0)
>> +        if (VIR_ALLOC(backingStore) < 0)
>>              goto cleanup;
>>
>> -        if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s",
>> +        if (!virStorageSourceSetBackingStore(&vol->target, backingStore, 0))
>> +            goto cleanup;
>> +        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)
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 306d98e..cae7484 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -3044,7 +3044,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>>          goto cleanup;
>>      }
>>
>> -    src->backingStore = backingStore;
>> +    if (!virStorageSourceSetBackingStore(src, backingStore, 0))
>> +        goto cleanup;
>>      backingStore = NULL;
>>      ret = 0;
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index d69f49d..234a72b 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1902,8 +1902,10 @@ virStorageSourceCopy(const virStorageSource *src,
>>          goto error;
>>
>>      if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
>> -        if (!(ret->backingStore = virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
>> -                                                       true)))
>> +        if (!virStorageSourceSetBackingStore(ret,
>> +                                             virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
>> +                                                                  true),
>> +                                             0))
>>              goto error;
>>      }
>>
>> @@ -2044,7 +2046,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
>>
>>      /* recursively free backing chain */
>>      virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
>> -    def->backingStore = NULL;
>> +    virStorageSourceSetBackingStore(def, NULL, 0);
>>  }
>>
>>
>> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
>> index 5bd4637..58f505d 100644
>> --- a/tests/virstoragetest.c
>> +++ b/tests/virstoragetest.c
>> @@ -583,9 +583,9 @@ testPathRelativePrepare(void)
>>
>>      for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) {
>>          if (i < ARRAY_CARDINALITY(backingchain) - 1)
>> -            backingchain[i].backingStore = &backingchain[i + 1];
>> +            virStorageSourceSetBackingStore(&backingchain[i], &backingchain[i + 1], 0);
>>          else
>> -            backingchain[i].backingStore = NULL;
>> +            virStorageSourceSetBackingStore(&backingchain[i], NULL, 0);
>>
>>          backingchain[i].relPath = NULL;
>>      }
>>

ok sorry for the goto mistake.

If I can help you to understand my I'll be happy to help.


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