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

2015-05-12 Thread 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.ga...@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

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

2015-04-23 Thread Matthias Gatto 
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.ga...@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/storage/storage_backend.c b/src/storage/storage_backend.c
index 269a93b..b0eb976 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -487,7 +487,7 @@ virStorageBackendCreateRaw(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-if (vol-target.backingStore) {
+if (vol-target.backingStores) {
 virReportError(VIR_ERR_NO_SUPPORT, %s,
_(backing storage not supported for raw volumes));
 goto cleanup;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index bab2569..dd9ccb5 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1048,7 +1048,7 @@ static int createFileDir(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 return -1;
 }
 
-if (vol-target.backingStore) {
+if (vol-target.backingStores) {
 virReportError(VIR_ERR_NO_SUPPORT, %s,
_(backing storage not supported for directories 
volumes));
 return -1;
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)
+goto allocation_failed;
+} else {
+if (VIR_ALLOC_N(src-backingStores, nbr)  0)
+goto allocation_failed;
+src-nBackingStores += nbr;
+}
+return true;
+ allocation_failed:
+virReportOOMError();
+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;
 }
 
 
@@ -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));
+