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

[Daniel P . Berrangé]

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é 
> > ---
> >  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, );
> > +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, );
> > +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

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

[Peter Krempa]

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é 
> ---
>  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, );
> +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, );
> +if (ret < 0)
> +return -1;
> +if (backend == NULL)
> +return 0;
>  
> -return !!backend->storageFileChown;
> +return backend->storageFileChown ? 1 : 0;

ACK


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list