[libvirt] [PATCH v5 5/9] virstoragefile: change backingStore to backingStores.

[Peter Krempa]

On Thu, Apr 23, 2015 at 14:41:17 +0200, Matthias Gatto wrote:
> The backingStore field was a virStorageSourcePtr.
> because a quorum can contain more that one backingStore at the same level
> it's now a 'virStorageSourcePtr *'.
> 
> This patch rename  src->backingStore to src->backingStores,
> add a static function virStorageSourceExpandBackingStore
> (virStorageSourcePushBackingStore in the V2) and made the necessary modification to
> virStorageSourceSetBackingStore and virStorageSourceGetBackingStore.
> virStorageSourceSetBackingStore can now expand size of src->backingStores
> by calling virStorageSourceExpandBackingStore if necessary.
> 
> Signed-off-by: Matthias Gatto <matthias.gatto at outscale.com>
> ---
>  src/storage/storage_backend.c    |  2 +-
>  src/storage/storage_backend_fs.c |  2 +-
>  src/util/virstoragefile.c        | 75 +++++++++++++++++++++++++++++++++++-----
>  src/util/virstoragefile.h        |  3 +-
>  4 files changed, 71 insertions(+), 11 deletions(-)
> 

...

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 234a72b..f0450b5 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1809,21 +1809,72 @@ 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)
>  {
> -    return src->backingStore;
> +    if (!src || !src->backingStores || pos >= src->nBackingStores)
> +        return NULL;
> +    return src->backingStores[pos];
>  }
>  
>  
> +/**
> + * virStorageSourcePushBackingStore:
> + *
> + * Expand src->backingStores and update src->nBackingStores
> + */
> +static bool
> +virStorageSourceExpandBackingStore(virStorageSourcePtr src, size_t nbr)
> +{
> +    if (!src) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("src is NULL"));
> +        return false;
> +    }
> +    if (src->nBackingStores > 0) {
> +        if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0)


This internally reallocates the memory even if the original pointer is
NULL, so there's no need for the if statement)


> +            goto allocation_failed;
> +    } else {
> +        if (VIR_ALLOC_N(src->backingStores, nbr) < 0)
> +            goto allocation_failed;
> +        src->nBackingStores += nbr;


> +    }
> +    return true;
> + allocation_failed:
> +    virReportOOMError();

Almost every libvirt memory allocation function reports the OOM error
internally, so there's no need to do it here.

> +    return false;
> +}
> +
> +
> +/**
> + * virStorageSourceSetBackingStore:
> + * @src: virStorageSourcePtr to hold @backingStore
> + * @backingStore: backingStore to store
> + * @pos: position of the backing store to store
> + *
> + * Set @backingStore at @pos in src->backingStores.
> + * If src->backingStores don't have the space to contain
> + * @backingStore, we expand src->backingStores
> + */
>  bool
>  virStorageSourceSetBackingStore(virStorageSourcePtr src,
>                                  virStorageSourcePtr backingStore,
> -                                size_t pos ATTRIBUTE_UNUSED)
> +                                size_t pos)
>  {
> -    src->backingStore = backingStore;
> -    return !!src->backingStore;
> +    if (!src)
> +        return false;
> +    if (pos >= src->nBackingStores &&
> +        !virStorageSourceExpandBackingStore(src, pos - src->nBackingStores + 1))

> +        return false;
> +    src->backingStores[pos] = backingStore;
> +    return true;

In general virStorageSourceExpandBackingStore is almost useless. It
could be folded in this function.

>  }
>  
>  
> @@ -2038,6 +2089,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src)
>  void
>  virStorageSourceBackingStoreClear(virStorageSourcePtr def)
>  {
> +    size_t i;
> +
>      if (!def)
>          return;
>  
> @@ -2045,8 +2098,14 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
>      VIR_FREE(def->backingStoreRaw);
>  
>      /* recursively free backing chain */
> -    virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
> -    virStorageSourceSetBackingStore(def, NULL, 0);
> +    for (i = 0; i < def->nBackingStores; ++i)
> +        virStorageSourceFree(virStorageSourceGetBackingStore(def, i));
> +    if (def->nBackingStores > 0) {
> +        /* in this case def->backingStores is treat as an array so we have to free it*/

s/treat/treated/ ... or drop the comment

> +        VIR_FREE(def->backingStores);
> +    }
> +    def->nBackingStores = 0;
> +    def->backingStores = NULL;

VIR_FREE sets the pointer to NULL already.

>  }
>  
>  

In general, the code can be done simpler. I'll have to look at other
patches to see how this will be used and if it makes sense. Also the
commit message wording is a bit awkward.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150512/fa843b68/attachment-0001.sig>