Re: [libvirt] [PATCH v2 5/9] virstoragefile: Treat backingStore as a pointer or an array

On 21.01.2015 16:29, Matthias Gatto wrote: > As explain in the former patchs, backingStore can be treat an array or > a pointer. > If we have only one backingStore we have to use it as a normal ptr > but if there is more backing store, we use it as a pointer's array. > > Because it would be complicated to expend backingStore manually, and do > the convertion from a pointer to an array, I've created the > virStorageSourcePushBackingStore function to help. > > This function allocate an array of pointer only if there is more than 1 bs. > > Because we can not remove a child from a quorum, i didn't create the function > virStorageSourcePopBackingStore. > > I've changed virStorageSourceBackingStoreClear, virStorageSourceSetBackingStore > and virStorageSourceGetBackingStore to handle the case where backingStore > is an array. > > Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com&gt; > --- > src/conf/storage_conf.c | 7 ++- > src/libvirt_private.syms | 1 + > src/storage/storage_backend_fs.c | 2 + > src/storage/storage_backend_logical.c | 2 +- > src/util/virstoragefile.c | 89 ++++++++++++++++++++++++++++++++--- > src/util/virstoragefile.h | 2 + > 6 files changed, 94 insertions(+), 9 deletions(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index fac85fa..41887eb 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -1343,7 +1343,12 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, > if (VIR_ALLOC(backingStorePtr) < 0) > goto error; > > - backingStorePtr = virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0); > + if (!virStorageSourcePushBackingStore(&ret->target)) > + goto error; > + > + backingStorePtr = virStorageSourceSetBackingStore(&ret->target, > + backingStorePtr, > + ret->target.nBackingStores - 1); > backingStorePtr->path = backingStore; > backingStore = NULL; > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 980235b..5752907 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2033,6 +2033,7 @@ virStorageSourceParseRBDColonString; > virStorageSourcePoolDefFree; > virStorageSourcePoolModeTypeFromString; > virStorageSourcePoolModeTypeToString; > +virStorageSourcePushBackingStore; > virStorageSourceSetBackingStore; > virStorageTypeFromString; > virStorageTypeToString; > diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c > index a3b6688..0d8c5ad 100644 > --- a/src/storage/storage_backend_fs.c > +++ b/src/storage/storage_backend_fs.c > @@ -112,6 +112,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, > > if (VIR_ALLOC(backingStore) < 0) > goto cleanup; > + if (virStorageSourcePushBackingStore(target) == false) > + goto cleanup; > virStorageSourceSetBackingStore(target, backingStore, 0); > backingStore->type = VIR_STORAGE_TYPE_NETWORK; > backingStore->path = meta->backingStoreRaw; > diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c > index fcec31f..cd8de85 100644 > --- a/src/storage/storage_backend_logical.c > +++ b/src/storage/storage_backend_logical.c > @@ -148,7 +148,7 @@ 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 (virStorageSourcePushBackingStore(&vol->target) == false) > goto cleanup; > > if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s", > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c > index ba38827..554aa8b 100644 > --- a/src/util/virstoragefile.c > +++ b/src/util/virstoragefile.c > @@ -1798,21 +1798,88 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) > } > > > +/** > + * virStorageSourceGetBackingStore: > + * @src: virStorageSourcePtr containing the backing stores > + * @pos: position of the backing store to get > + * > + * return the backingStore at the position of @pos > + */ > virStorageSourcePtr > -virStorageSourceGetBackingStore(const virStorageSource *src, > - size_t pos ATTRIBUTE_UNUSED) > +virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos) > +{ > + if (!src->backingStore || (pos > 1 && pos >= src->nBackingStores)) > + return NULL; > + if (src->nBackingStores < 2) > + return src->backingStore; > + return ((virStorageSourcePtr *)src->backingStore)[pos]; > +} > + > + > +/** > + * virStorageSourcePushBackingStore: > + * @src: virStorageSourcePtr to allocate the new backing store > + * > + * Allocate size for a new backingStorePtr in src->backingStore > + * and update src->nBackingStores > + * If we have less than 2 backing stores, we treat src->backingStore > + * as a pointer otherwise we treat it as an array of virStorageSourcePtr > + */ > +bool > +virStorageSourcePushBackingStore(virStorageSourcePtr src) > { > - return src->backingStore; > + virStorageSourcePtr tmp; > + virStorageSourcePtr *tmp2; > + > + if (!src) > + return false; > + if (src->nBackingStores == 1) { > + /* If we need more than one backing store we need an array > + * Because we don't want to lose our data from the old Backing Store > + * we copy the pointer from src->backingStore to src->backingStore[0] */ > + tmp = src->backingStore; > + if (VIR_ALLOC_N(tmp2, 1) < 0) > + return false; > + src->backingStore = (virStorageSourcePtr)tmp2; > + src->nBackingStores += 1; > + virStorageSourceSetBackingStore(src, tmp, 0); > + } else if (src->nBackingStores > 1) { > + tmp2 = ((virStorageSourcePtr *)src->backingStore); > + if (VIR_EXPAND_N(tmp2, src->nBackingStores, 1) < 0) > + return false; > + src->backingStore = (virStorageSourcePtr)tmp2; > + } else { > + /* Most of the time we use only one backingStore > + * So we don't need to allocate an array */ > + src->nBackingStores += 1; > + } > + return true; I must say I find this logic very hard to read. What's the problem with really using src->backingStore as an array? > } > > > +/** > + * virStorageSourceSetBackingStore: > + * @src: virStorageSourcePtr to hold @backingStore > + * @backingStore: backingStore to store > + * @pos: position of the backing store to store > + * > + * Set @backingStore at @pos in src->backingStore > + */ > virStorageSourcePtr > virStorageSourceSetBackingStore(virStorageSourcePtr src, > virStorageSourcePtr backingStore, > - size_t pos ATTRIBUTE_UNUSED) > + size_t pos) > { > - src->backingStore = backingStore; > - return src->backingStore; > + if (!src || (pos > 1 && pos >= src->nBackingStores)) > + goto error; > + if (src->nBackingStores > 1) { > + ((virStorageSourcePtr *)src->backingStore)[pos] = backingStore; > + } else { > + src->backingStore = backingStore; > + } > + return backingStore; > + error: > + return NULL; > } > > > @@ -2023,6 +2090,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src) > void > virStorageSourceBackingStoreClear(virStorageSourcePtr def) > { > + size_t i; > + > if (!def) > return; > > @@ -2030,7 +2099,13 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) > VIR_FREE(def->backingStoreRaw); > > /* recursively free backing chain */ > - virStorageSourceFree(virStorageSourceGetBackingStore(def, 0)); > + for (i = 0; i < def->nBackingStores; ++i) > + virStorageSourceFree(virStorageSourceGetBackingStore(def, i)); > + if (def->nBackingStores > 1) { > + /* in this case def->backingStores is treat as an array so we have to free it*/ > + VIR_FREE(def->backingStore); > + } > + def->nBackingStores = 0; > virStorageSourceSetBackingStore(def, NULL, 0); > } > > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h > index d5cf7e6..74c363b 100644 > --- a/src/util/virstoragefile.h > +++ b/src/util/virstoragefile.h > @@ -271,6 +271,7 @@ struct _virStorageSource { > > /* backing chain of the storage source */ > virStorageSourcePtr backingStore; > + size_t nBackingStores; How can this fly? In the code you're still accessing it as an array of pointers, but here it's defined as array of structs. So you're just lucky the struct is bigger than a pointer. This should rather be: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 74c363b..11fb96d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -270,8 +270,8 @@ struct _virStorageSource { bool shared; /* backing chain of the storage source */ - virStorageSourcePtr backingStore; - size_t nBackingStores; + virStorageSourcePtr *backingStore; + size_t nBackingStores; /* metadata for storage driver access to remote and local volumes */ virStorageDriverDataPtr drv; I'm stopping my review here until the time I get some clarification here. Michal