Return the storage source definition via a pointer in the arguments and
document the returned values. This will simplify the possibility to
ignore certain backing store types which are not representable by
libvirt.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/storage/storage_util.c |  2 +-
 src/util/virstoragefile.c  | 59 ++++++++++++++++++++++++--------------
 src/util/virstoragefile.h  |  4 ++-
 3 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index 62f857f9ea..24c5918aa3 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3391,7 +3391,7 @@ storageBackendProbeTarget(virStorageSourcePtr target,
         return -1;

     if (meta->backingStoreRaw) {
-        if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
+        if (virStorageSourceNewFromBacking(meta, &target->backingStore) < 0)
             return -1;

         target->backingStore->format = backingStoreFormat;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 6fff013e3a..4e57a0438e 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3679,42 +3679,57 @@ virStorageSourceNewFromBackingAbsolute(const char *path)
 }


-virStorageSourcePtr
-virStorageSourceNewFromBacking(virStorageSourcePtr parent)
+/**
+ * virStorageSourceNewFromBacking:
+ * @parent: storage source parent
+ * @backing: returned backing store definition
+ *
+ * Creates a storage source which describes the backing image of @parent and
+ * fills it into @backing depending on the 'backingStoreRaw' property of @parent
+ * and other data. Note that for local storage this function interrogates the
+ * actual type of the backing store.
+ *
+ * Returns 0 and fills @backing, or -1 on error (with appropriate error reported).
+ */
+int
+virStorageSourceNewFromBacking(virStorageSourcePtr parent,
+                               virStorageSourcePtr *backing)
 {
     struct stat st;
-    virStorageSourcePtr ret = NULL;
     VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;

+    *backing = NULL;
+
     if (virStorageIsRelative(parent->backingStoreRaw))
         def = virStorageSourceNewFromBackingRelative(parent,
                                                      parent->backingStoreRaw);
     else
         def = virStorageSourceNewFromBackingAbsolute(parent->backingStoreRaw);

-    if (def) {
-        /* possibly update local type */
-        if (def->type == VIR_STORAGE_TYPE_FILE) {
-            if (stat(def->path, &st) == 0) {
-                if (S_ISDIR(st.st_mode)) {
-                    def->type = VIR_STORAGE_TYPE_DIR;
-                    def->format = VIR_STORAGE_FILE_DIR;
-                } else if (S_ISBLK(st.st_mode)) {
-                    def->type = VIR_STORAGE_TYPE_BLOCK;
-                }
+    if (!def)
+        return -1;
+
+    /* possibly update local type */
+    if (def->type == VIR_STORAGE_TYPE_FILE) {
+        if (stat(def->path, &st) == 0) {
+            if (S_ISDIR(st.st_mode)) {
+                def->type = VIR_STORAGE_TYPE_DIR;
+                def->format = VIR_STORAGE_FILE_DIR;
+            } else if (S_ISBLK(st.st_mode)) {
+                def->type = VIR_STORAGE_TYPE_BLOCK;
             }
         }
+    }

-        /* copy parent's labelling and other top level stuff */
-        if (virStorageSourceInitChainElement(def, parent, true) < 0)
-            return NULL;
+    /* copy parent's labelling and other top level stuff */
+    if (virStorageSourceInitChainElement(def, parent, true) < 0)
+        return -1;

-        def->readonly = true;
-        def->detected = true;
-    }
+    def->readonly = true;
+    def->detected = true;

-    VIR_STEAL_PTR(ret, def);
-    return ret;
+    VIR_STEAL_PTR(*backing, def);
+    return 0;
 }


@@ -4899,7 +4914,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
         goto cleanup;

     if (src->backingStoreRaw) {
-        if (!(backingStore = virStorageSourceNewFromBacking(src)))
+        if (virStorageSourceNewFromBacking(src, &backingStore) < 0)
             goto cleanup;

         if (backingFormat == VIR_STORAGE_FILE_AUTO)
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 2882bacf3e..3a72c62ad7 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -444,7 +444,9 @@ int virStorageSourceUpdateCapacity(virStorageSourcePtr src,
                                    char *buf, ssize_t len,
                                    bool probe);

-virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent);
+int virStorageSourceNewFromBacking(virStorageSourcePtr parent,
+                                   virStorageSourcePtr *backing);
+
 virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src,
                                          bool backingChain)
     ATTRIBUTE_NONNULL(1);
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Aug 16, 2019 at 12:39:28PM +0200, Peter Krempa wrote:
>Return the storage source definition via a pointer in the arguments and
>document the returned values. This will simplify the possibility to
>ignore certain backing store types which are not representable by
>libvirt.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/storage/storage_util.c |  2 +-
> src/util/virstoragefile.c  | 59 ++++++++++++++++++++++++--------------
> src/util/virstoragefile.h  |  4 ++-
> 3 files changed, 41 insertions(+), 24 deletions(-)
>
>diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>index 6fff013e3a..4e57a0438e 100644
>--- a/src/util/virstoragefile.c
>+++ b/src/util/virstoragefile.c
>@@ -3679,42 +3679,57 @@ virStorageSourceNewFromBackingAbsolute(const char *path)
> }
>
>
>-virStorageSourcePtr
>-virStorageSourceNewFromBacking(virStorageSourcePtr parent)
>+/**
>+ * virStorageSourceNewFromBacking:
>+ * @parent: storage source parent
>+ * @backing: returned backing store definition
>+ *
>+ * Creates a storage source which describes the backing image of @parent and
>+ * fills it into @backing depending on the 'backingStoreRaw' property of @parent
>+ * and other data. Note that for local storage this function interrogates the

Not quite sure what 'interrogates' is supposed to mean in this context.
Perhaps you could use another verb?

>+ * actual type of the backing store.
>+ *
>+ * Returns 0 and fills @backing, or -1 on error (with appropriate error reported).
>+ */
>+int
>+virStorageSourceNewFromBacking(virStorageSourcePtr parent,
>+                               virStorageSourcePtr *backing)
> {

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list