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

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(a)outscale.com&gt; > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > 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; > } >