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

[Ján Tomko]

On 03/31/2014 08: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()
>   */
>  int
>  virStorageBackendVolOpen(const char *path, struct stat *sb,
> @@ -1284,22 +1285,30 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>  {
>      int fd, mode = 0;
>      char *base = last_component(path);
> +    bool raise_error = (flags & VIR_STORAGE_VOL_OPEN_ERROR);

We've been using !!(flags & FLAG) for this kind of assignment, but I can't
remember what that fixes.

>  
>      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;
>      }
>  
>      if (S_ISFIFO(sb->st_mode)) {
> +        if (raise_error) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Volume path '%s' is a FIFO"), path);
> +        }
>          VIR_WARN("ignoring FIFO '%s'", path);

We don't need both an error and a warning. And I think we should return -1 if
an error has been logged.

>          return -2;
>      } else if (S_ISSOCK(sb->st_mode)) {
> +        if (raise_error) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Volume path '%s' is a socket"), path);
> +        }
>          VIR_WARN("ignoring socket '%s'", path);
>          return -2;
>      }


> @@ -1366,13 +1386,11 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
>      if (!(mode & flags)) {
>          VIR_FORCE_CLOSE(fd);
>          VIR_INFO("Skipping volume '%s'", path);
> -
> -        if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {

Hmm, it seems this condition could never have been true before.

> +        if (raise_error) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("unexpected storage mode for '%s'"), path);
>              return -1;
>          }
> -
>          return -2;
>      }
>  

> @@ -1389,8 +1407,12 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
>      int ret, fd = -1;
>      struct stat sb;
>  
> -    if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0)
> +    if ((ret = virStorageBackendVolOpen(target->path, &sb, openflags)) < 0) {
> +        /* ret == -2 may be a non-fatal error, but if callers want that
> +         * behavior they should call VolOpen manually */
> +        ret = -1;
>          goto cleanup;

This and the snipped changes below would not be needed if we never returned -2
with VOL_OPEN_ERROR (or by default if we switch the flag to NOERROR as Eric
suggested.)

> +    }
>      fd = ret;
>  
>      if ((ret = virStorageBackendUpdateVolTargetInfoFD(target,

Jan


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