[libvirt] [PATCH v2 4/4] storage: Report error from VolOpen if proper flag is passed

[Eric Blake]

On 03/31/2014 12:50 PM, Cole Robinson wrote:
> VolOpen notifies the user of a potentially non-fatal failure by
> returning -2 and logging a VIR_WARN or VIR_INFO. Unfortunately most
> callers treat -2 as fatal but don't actually report any message with
> the error APIs.
> 
> Change VolOpen to report an error if the VOL_OPEN_ERROR flag is passed.
> The ony caller that doesn't use this flag is the one that is explicitly
> handling -2 return code, so it fits the pattern.
> 
> Tweak some of the other call sites to propagate the newly raised error.
> ---
>  src/storage/storage_backend.c         | 60 ++++++++++++++++++++++++-----------
>  src/storage/storage_backend_fs.c      | 21 ++++++------
>  src/storage/storage_backend_logical.c |  6 +++-
>  src/storage/storage_backend_scsi.c    |  4 +--
>  4 files changed, 59 insertions(+), 32 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 42bd445..7c1220e 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1273,10 +1273,11 @@ virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target,
>  
>  
>  /*
> - * Allows caller to silently ignore files with improper mode
> - *
>   * Returns -1 on error, -2 if file mode is unexpected or the
>   * volume is a dangling symbolic link.
> + *
> + * -2 can be an ignorable error, but callers have to make sure to
> + * virResetLastError()
>   */

Is this comment still accurate?

>      char *base = last_component(path);
> +    bool raise_error = (flags & VIR_STORAGE_VOL_OPEN_ERROR);

Hmm - are the semantics of this flag backwards?  If most users always
want an error, and only one specific caller wants the error suppressed
because it is prepared to deal with a -2 return, shouldn't the default
be errors when flags==0, and the one caller pass a non-zero flag for
VIR_STORAGE_VOL_OPEN_NOERROR?

>  
>      if (lstat(path, sb) < 0) {
> -        if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
> +        if (errno == ENOENT && !raise_error) {
>              VIR_WARN("ignoring missing file '%s'", path);
>              return -2;
>          }
> -        virReportSystemError(errno,
> -                             _("cannot stat file '%s'"),
> -                             path);
> +
> +        virReportSystemError(errno, _("cannot stat file '%s'"), path);
>          return -1;

This is an unconditional error, even when the flags say the user doesn't
want errors; do we need to tweak the docs to mention that we are only
suppressing SOME errors, not all?

> @@ -82,8 +82,11 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>          *encryption = NULL;
>  
>      if ((ret = virStorageBackendVolOpen(target->path, &sb,
> -                                        VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0)
> -        goto error; /* Take care to propagate ret, it is not always -1 */
> +                                      VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0) {
> +        /* ret == -2 is non-fatal, propage the return code so the caller

s/propage/propagate/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140331/7b413346/attachment-0001.sig>