1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/6] Split the storage file driver impls from the storage driver backends
  4. View patch

[libvirt] [PATCH 5/6] util: refactor storage file checks to allow error reporting

Daniel P. Berrangé posted 6 patches 7 years ago
[libvirt] [PATCH 5/6] util: refactor storage file checks to allow error reporting
Posted by Daniel P. Berrangé 7 years ago 
The virStorageFileSupportsSecurityDriver and
virStorageFileSupportsAccess currently just return a boolean
value. This is ok because they don't have any failure scenarios
but a subsequent patch is going to introduce potential failure
scenario. This changes their return type from a boolean to an
int with values -1, 0, 1.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_domain.c    | 21 +++++++++------
 src/qemu/qemu_driver.c    |  6 +++--
 src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++----------------
 src/util/virstoragefile.h |  4 +--
 4 files changed, 63 insertions(+), 34 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 326c939c85..542e20c5e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7514,19 +7514,24 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
 
     /* skip to the end of the chain if there is any */
     while (virStorageSourceHasBacking(src)) {
-        if (report_broken &&
-            virStorageFileSupportsAccess(src)) {
+        if (report_broken) {
+            int rv = virStorageFileSupportsAccess(src);
 
-            if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
+            if (rv < 0)
                 goto cleanup;
 
-            if (virStorageFileAccess(src, F_OK) < 0) {
-                virStorageFileReportBrokenChain(errno, src, disk->src);
+            if (rv > 0) {
+                if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0)
+                    goto cleanup;
+
+                if (virStorageFileAccess(src, F_OK) < 0) {
+                    virStorageFileReportBrokenChain(errno, src, disk->src);
+                    virStorageFileDeinit(src);
+                    goto cleanup;
+                }
+
                 virStorageFileDeinit(src);
-                goto cleanup;
             }
-
-            virStorageFileDeinit(src);
         }
         src = src->backingStore;
     }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 672c5372eb..168a7c9ff3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -308,9 +308,11 @@ qemuSecurityChownCallback(const virStorageSource *src,
     struct stat sb;
     int save_errno = 0;
     int ret = -1;
+    int rv;
 
-    if (!virStorageFileSupportsSecurityDriver(src))
-        return 0;
+    rv = virStorageFileSupportsSecurityDriver(src);
+    if (rv <= 0)
+        return rv;
 
     if (virStorageSourceIsLocalStorage(src)) {
         /* use direct chmod for local files so that the file doesn't
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f09035cd4a..da13d51d32 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src)
 }
 
 
-static virStorageFileBackendPtr
-virStorageFileGetBackendForSupportCheck(const virStorageSource *src)
+static int
+virStorageFileGetBackendForSupportCheck(const virStorageSource *src,
+                                        virStorageFileBackendPtr *backend)
 {
     int actualType;
 
-    if (!src)
-        return NULL;
 
-    if (src->drv)
-        return src->drv->backend;
+    if (!src) {
+        *backend = NULL;
+        return 0;
+    }
+
+    if (src->drv) {
+        *backend = src->drv->backend;
+        return 0;
+    }
 
     actualType = virStorageSourceGetActualType(src);
 
-    return virStorageFileBackendForTypeInternal(actualType, src->protocol, false);
+    *backend = virStorageFileBackendForTypeInternal(actualType, src->protocol, false);
+    return 0;
 }
 
 
-static bool
+static int
 virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
 {
     virStorageFileBackendPtr backend;
+    int ret;
 
-    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
-        return false;
+    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
+    if (ret < 0)
+        return -1;
+
+    if (!backend)
+        return 0;
 
     return backend->storageFileGetUniqueIdentifier &&
-           backend->storageFileRead &&
-           backend->storageFileAccess;
+        backend->storageFileRead &&
+        backend->storageFileAccess ? 1 : 0;
 }
 
 
@@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
  * Check if a storage file supports operations needed by the security
  * driver to perform labelling
  */
-bool
+int
 virStorageFileSupportsSecurityDriver(const virStorageSource *src)
 {
     virStorageFileBackendPtr backend;
+    int ret;
 
-    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
-        return false;
+    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
+    if (ret < 0)
+        return -1;
+    if (backend == NULL)
+        return 0;
 
-    return !!backend->storageFileChown;
+    return backend->storageFileChown ? 1 : 0;
 }
 
 
@@ -4157,15 +4173,19 @@ virStorageFileSupportsSecurityDriver(const virStorageSource *src)
  * Check if a storage file supports checking if the storage source is accessible
  * for the given vm.
  */
-bool
+int
 virStorageFileSupportsAccess(const virStorageSource *src)
 {
     virStorageFileBackendPtr backend;
+    int ret;
 
-    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
-        return false;
+    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
+    if (ret < 0)
+        return -1;
+    if (backend == NULL)
+        return 0;
 
-    return !!backend->storageFileAccess;
+    return backend->storageFileAccess ? 1 : 0;
 }
 
 
@@ -4514,14 +4534,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
     ssize_t headerLen;
     virStorageSourcePtr backingStore = NULL;
     int backingFormat;
+    int rv;
 
     VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d",
               src->path, src->format,
               (unsigned int)uid, (unsigned int)gid, allow_probe);
 
     /* exit if we can't load information about the current image */
-    if (!virStorageFileSupportsBackingChainTraversal(src))
-        return 0;
+    rv = virStorageFileSupportsBackingChainTraversal(src);
+    if (rv <= 0)
+        return rv;
 
     if (virStorageFileInitAs(src, uid, gid) < 0)
         return -1;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index b92c1c47dd..0909fe212c 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -465,8 +465,8 @@ const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src);
 int virStorageFileAccess(virStorageSourcePtr src, int mode);
 int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid);
 
-bool virStorageFileSupportsSecurityDriver(const virStorageSource *src);
-bool virStorageFileSupportsAccess(const virStorageSource *src);
+int virStorageFileSupportsSecurityDriver(const virStorageSource *src);
+int virStorageFileSupportsAccess(const virStorageSource *src);
 
 int virStorageFileGetMetadata(virStorageSourcePtr src,
                               uid_t uid, gid_t gid,
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] util: refactor storage file checks to allow error reporting
Posted by Peter Krempa 7 years ago 
On Wed, Apr 25, 2018 at 16:52:42 +0100, Daniel Berrange wrote:
> The virStorageFileSupportsSecurityDriver and
> virStorageFileSupportsAccess currently just return a boolean
> value. This is ok because they don't have any failure scenarios
> but a subsequent patch is going to introduce potential failure
> scenario. This changes their return type from a boolean to an
> int with values -1, 0, 1.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/qemu/qemu_domain.c    | 21 +++++++++------
>  src/qemu/qemu_driver.c    |  6 +++--
>  src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++----------------
>  src/util/virstoragefile.h |  4 +--
>  4 files changed, 63 insertions(+), 34 deletions(-)

[...]

> index f09035cd4a..da13d51d32 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src)

[...]

> -static bool
> +static int
>  virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
>  {
>      virStorageFileBackendPtr backend;
> +    int ret;

Hmm, isn't 'rv' better in the case when this variable actually is not
returned?

>  
> -    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
> -        return false;
> +    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
> +    if (ret < 0)
> +        return -1;
> +
> +    if (!backend)
> +        return 0;
>  
>      return backend->storageFileGetUniqueIdentifier &&
> -           backend->storageFileRead &&
> -           backend->storageFileAccess;
> +        backend->storageFileRead &&
> +        backend->storageFileAccess ? 1 : 0;

Alignment looks off

>  }
>  
>  
> @@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
>   * Check if a storage file supports operations needed by the security
>   * driver to perform labelling
>   */
> -bool
> +int
>  virStorageFileSupportsSecurityDriver(const virStorageSource *src)
>  {
>      virStorageFileBackendPtr backend;
> +    int ret;

As in above hunk.

>  
> -    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
> -        return false;
> +    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
> +    if (ret < 0)
> +        return -1;
> +    if (backend == NULL)
> +        return 0;
>  
> -    return !!backend->storageFileChown;
> +    return backend->storageFileChown ? 1 : 0;

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] util: refactor storage file checks to allow error reporting
Posted by Daniel P. Berrangé 7 years ago 
On Thu, Apr 26, 2018 at 11:15:42AM +0200, Peter Krempa wrote:
> On Wed, Apr 25, 2018 at 16:52:42 +0100, Daniel Berrange wrote:
> > The virStorageFileSupportsSecurityDriver and
> > virStorageFileSupportsAccess currently just return a boolean
> > value. This is ok because they don't have any failure scenarios
> > but a subsequent patch is going to introduce potential failure
> > scenario. This changes their return type from a boolean to an
> > int with values -1, 0, 1.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  src/qemu/qemu_domain.c    | 21 +++++++++------
> >  src/qemu/qemu_driver.c    |  6 +++--
> >  src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++----------------
> >  src/util/virstoragefile.h |  4 +--
> >  4 files changed, 63 insertions(+), 34 deletions(-)
> 
> [...]
> 
> > index f09035cd4a..da13d51d32 100644
> > --- a/src/util/virstoragefile.c
> > +++ b/src/util/virstoragefile.c
> > @@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src)
> 
> [...]
> 
> > -static bool
> > +static int
> >  virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
> >  {
> >      virStorageFileBackendPtr backend;
> > +    int ret;
> 
> Hmm, isn't 'rv' better in the case when this variable actually is not
> returned?

Sure will change it.

> 
> >  
> > -    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
> > -        return false;
> > +    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
> > +    if (ret < 0)
> > +        return -1;
> > +
> > +    if (!backend)
> > +        return 0;
> >  
> >      return backend->storageFileGetUniqueIdentifier &&
> > -           backend->storageFileRead &&
> > -           backend->storageFileAccess;
> > +        backend->storageFileRead &&
> > +        backend->storageFileAccess ? 1 : 0;
> 
> Alignment looks off

Depends on your POV - this is standard indentation after a new
line - it would only line up following lines if there was a
opening round bracket on first line. That said I'll change it
to avoid affecting pre-existing code alignment.

> 
> >  }
> >  
> >  
> > @@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
> >   * Check if a storage file supports operations needed by the security
> >   * driver to perform labelling
> >   */
> > -bool
> > +int
> >  virStorageFileSupportsSecurityDriver(const virStorageSource *src)
> >  {
> >      virStorageFileBackendPtr backend;
> > +    int ret;
> 
> As in above hunk.
> 
> >  
> > -    if (!(backend = virStorageFileGetBackendForSupportCheck(src)))
> > -        return false;
> > +    ret = virStorageFileGetBackendForSupportCheck(src, &backend);
> > +    if (ret < 0)
> > +        return -1;
> > +    if (backend == NULL)
> > +        return 0;
> >  
> > -    return !!backend->storageFileChown;
> > +    return backend->storageFileChown ? 1 : 0;
> 
> ACK



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.