[libvirt] [PATCH v3] storage: Report error from VolOpen by default

[Eric Blake]

On 04/02/2014 09:54 AM, Cole Robinson wrote:
> Currently 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.
> 
> Rename the VOL_OPEN_ERROR flag to VOL_OPEN_NOERROR. If NOERROR is specified,
> we preserve the current behavior of returning -2 (there's only one caller
> that wants this).
> 
> However in the default case, only return -1, and actually use the error
> APIs. Fix up a couple callers as a result.
> ---
>  src/storage/storage_backend.c      | 77 ++++++++++++++++++++++++--------------
>  src/storage/storage_backend.h      |  7 ++--
>  src/storage/storage_backend_fs.c   | 28 +++++---------
>  src/storage/storage_backend_scsi.c |  3 --
>  4 files changed, 62 insertions(+), 53 deletions(-)

> + * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we
> + * return -2 if file mode is unexpected or the volume is a dangling
> + * symbolic link.

Seems fair; let's see how it works below...

>   */
>  int
>  virStorageBackendVolOpen(const char *path, struct stat *sb,
> @@ -1288,9 +1289,10 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>  {
>      int fd, mode = 0;
>      char *base = last_component(path);
> +    bool noerror = (flags * VIR_STORAGE_VOL_OPEN_NOERROR);

Whoops[1] - that returns true for any non-zero flags value.  I think you
meant s/*/&/

>      } else {
> -        VIR_WARN("ignoring unexpected type for file '%s'", path);
>          VIR_FORCE_CLOSE(fd);
> -        return -2;
> +        if (noerror) {
> +            VIR_WARN("ignoring unexpected type for file '%s'", path);
> +            return -2;
> +        }
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("unexpected type for file '%s'"), path);
> +        return -1;

...so far, so good (all cases of 'return -2' are guarded by noerror, and
an error is issued before returning -1).

>      }
>  
>      if (virSetBlocking(fd, true) < 0) {
> +        VIR_FORCE_CLOSE(fd);
> +        if (noerror) {
> +            VIR_WARN("unable to set blocking mode for '%s'", path);
> +            return -2;
> +        }

But this one feels wrong[2].  If we get here, we KNOW the fd has the
expected type, and we successfully opened it.  This is a case where the
system is hosed, and we should loudly and unconditionally fail with -1,
rather than returning -2 on noerror.


>  /* VolOpenCheckMode flags */
>  enum {
> -    VIR_STORAGE_VOL_OPEN_ERROR  = 1 << 0, /* warn if unexpected type
> -                                           * encountered */
> +    VIR_STORAGE_VOL_OPEN_NOERROR  = 1 << 0, /* don't error if unexpected type
> +                                             * encountered, just warn */
>      VIR_STORAGE_VOL_OPEN_REG    = 1 << 1, /* regular files okay */
>      VIR_STORAGE_VOL_OPEN_BLOCK  = 1 << 2, /* block files okay */
>      VIR_STORAGE_VOL_OPEN_CHAR   = 1 << 3, /* char files okay */
>      VIR_STORAGE_VOL_OPEN_DIR    = 1 << 4, /* directories okay */

Cosmetic, but alignment of = is now off [3].

> -            if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore,
> -                                                     false,
> -                                        VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
> -                /* The backing file is currently unavailable, the capacity,
> -                 * allocation, owner, group and mode are unknown. Just log the
> -                 * error and continue.
> -                 * Unfortunately virStorageBackendProbeTarget() might already
> -                 * have logged a similar message for the same problem, but only
> -                 * if AUTO format detection was used. */
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("cannot probe backing volume info: %s"),
> -                               vol->backingStore.path);
> -            }
> +            virStorageBackendUpdateVolTargetInfo(&vol->backingStore, false,
> +                                                 VIR_STORAGE_VOL_OPEN_DEFAULT);
> +            /* If this failed, the backing file is currently unavailable,
> +             * the capacity, allocation, owner, group and mode are unknown.
> +             * An error message was raised, but we just continue. */

You may need an ignore_value() here to keep Coverity quiet about an
unchecked error return [4].

This touches code that I'm also working on, so I'm interested in getting
it in sooner to avoid rebase churn over repeated iterations.  ACK if you
fix [1] and [2] above; and up to you whether changes are needed at [3]
and [4].

-- 
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/20140402/ddca5e03/attachment-0001.sig>