[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3] storage: Check for invalid storage mode before opening



On 05/24/2010 05:36 PM, Cole Robinson wrote:
> If a directory pool contains pipes or sockets, a pool start can fail or hang:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=589577
> 
> We already try to avoid these special files, but only attempt after
> opening the path, which is where the problems lie. Unify volume opening
> into helper functions, which use the proper open() flags to avoid error,
> followed by fstat to validate storage mode.
> 
> Previously, virStorageBackendUpdateVolTargetInfoFD attempted to enforce the
> storage mode check, but allowed callers to detect this case and silently
> continue. In practice, only the FS backend was using this feature, the rest
> were treating unknown mode as an error condition. Unfortunately the InfoFD
> function wasn't raising an error message here, so error reporting was
> busted.
> 
> This patch adds 2 functions: virStorageBackendVolOpen, and
> virStorageBackendVolOpenModeSkip. The latter retains the original opt out
> semantics, the former now throws an explicit error.
> 
> This patch maintains the previous volume mode checks: allowing specific
> modes for specific pool types requires a bit of surgery, since VolOpen
> is called through several different helper functions.
> 
> v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with
>     O_NONBLOCK|O_NOCTTY.
> 
> v3: Move mode check logic back to VolOpen. Use 2 VolOpen functions with
>     different error semantics to improve error reporting.
> 

ping, not sure if anyone saw this.

- Cole

> Signed-off-by: Cole Robinson <crobinso redhat com>
> ---
>  src/storage/storage_backend.c         |   74 ++++++++++++++++++++++++++------
>  src/storage/storage_backend.h         |    5 ++
>  src/storage/storage_backend_fs.c      |   11 ++---
>  src/storage/storage_backend_logical.c |    9 +---
>  src/storage/storage_backend_mpath.c   |    9 +---
>  src/storage/storage_backend_scsi.c    |   17 ++++----
>  6 files changed, 83 insertions(+), 42 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index f4124df..702870a 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
>      return NULL;
>  }
>  
> +static int
> +virStorageBackendVolOpenInternal(const char *path)
> +{
> +    int fd;
> +    struct stat sb;
> +
> +    if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot open volume '%s'"),
> +                             path);
> +        return -1;
> +    }
> +
> +    if (fstat(fd, &sb) < 0) {
> +        virReportSystemError(errno,
> +                             _("cannot stat file '%s'"),
> +                             path);
> +        close(fd);
> +        return -1;
> +    }
> +
> +    if (!S_ISREG(sb.st_mode) &&
> +        !S_ISCHR(sb.st_mode) &&
> +        !S_ISBLK(sb.st_mode)) {
> +        close(fd);
> +        return -2;
> +    }
> +
> +    return fd;
> +}
> +
> +/*
> + * Allows caller to silently ignore files with improper mode
> + *
> + * Returns -1 on error, -2 if file mode is unexpected.
> + */
> +int virStorageBackendVolOpenModeSkip(const char *path)
> +{
> +    return virStorageBackendVolOpenInternal(path);
> +}
> +
> +int
> +virStorageBackendVolOpen(const char *path)
> +{
> +    int ret;
> +
> +    ret = virStorageBackendVolOpenInternal(path);
> +    if (ret == -2) {
> +        virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> +                              _("unexpected storage mode for '%s'"), path);
> +        return -1;
> +    }
> +
> +    return ret;
> +}
>  
>  int
>  virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
> @@ -880,13 +935,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
>  {
>      int ret, fd;
>  
> -    if ((fd = open(target->path, O_RDONLY)) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot open volume '%s'"),
> -                             target->path);
> -        return -1;
> -    }
> +    if ((ret = virStorageBackendVolOpen(target->path)) < 0)
> +        return ret;
>  
> +    fd = ret;
>      ret = virStorageBackendUpdateVolTargetInfoFD(target,
>                                                   fd,
>                                                   allocation,
> @@ -920,12 +972,11 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
>   * virStorageBackendUpdateVolTargetInfoFD:
>   * @conn: connection to report errors on
>   * @target: target definition ptr of volume to update
> - * @fd: fd of storage volume to update
> + * @fd: fd of storage volume to update, via virStorageBackendOpenVol*
>   * @allocation: If not NULL, updated allocation information will be stored
>   * @capacity: If not NULL, updated capacity info will be stored
>   *
> - * Returns 0 for success-1 on a legitimate error condition,
> - *    -2 if passed FD isn't a regular, char, or block file.
> + * Returns 0 for success, -1 on a legitimate error condition.
>   */
>  int
>  virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
> @@ -945,11 +996,6 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
>          return -1;
>      }
>  
> -    if (!S_ISREG(sb.st_mode) &&
> -        !S_ISCHR(sb.st_mode) &&
> -        !S_ISBLK(sb.st_mode))
> -        return -2;
> -
>      if (allocation) {
>          if (S_ISREG(sb.st_mode)) {
>  #ifndef WIN32
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 907c4bc..8f54dce 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -80,6 +80,11 @@ struct _virStorageBackend {
>  
>  virStorageBackendPtr virStorageBackendForType(int type);
>  
> +int virStorageBackendVolOpen(const char *path)
> +ATTRIBUTE_NONNULL(1);
> +
> +int virStorageBackendVolOpenModeSkip(const char *path)
> +ATTRIBUTE_NONNULL(1);
>  
>  int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
>                                     int withCapacity);
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index c96c4f3..5aceb39 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -61,18 +61,15 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
>      if (encryption)
>          *encryption = NULL;
>  
> -    if ((fd = open(target->path, O_RDONLY)) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot open volume '%s'"),
> -                             target->path);
> -        return -1;
> -    }
> +    if ((ret = virStorageBackendVolOpenModeSkip(target->path)) < 0)
> +        return ret; /* Take care to propagate ret, it is not always -1 */
> +    fd = ret;
>  
>      if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd,
>                                                        allocation,
>                                                        capacity)) < 0) {
>          close(fd);
> -        return ret; /* Take care to propagate ret, it is not always -1 */
> +        return -1;
>      }
>  
>      memset(&meta, 0, sizeof(meta));
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 06238d1..616ca1a 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -559,7 +559,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>                                    virStoragePoolObjPtr pool,
>                                    virStorageVolDefPtr vol)
>  {
> -    int fd = -1;
> +    int fdret, fd = -1;
>      char size[100];
>      const char *cmdargvnew[] = {
>          LVCREATE, "--name", vol->name, "-L", size,
> @@ -602,12 +602,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
>      if (virRun(cmdargv, NULL) < 0)
>          return -1;
>  
> -    if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot read path '%s'"),
> -                             vol->target.path);
> +    if ((fdret = virStorageBackendVolOpen(vol->target.path)) < 0)
>          goto cleanup;
> -    }
> +    fd = fdret;
>  
>      /* We can only chown/grp if root */
>      if (getuid() == 0) {
> diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c
> index 3a137eb..3d7dfcc 100644
> --- a/src/storage/storage_backend_mpath.c
> +++ b/src/storage/storage_backend_mpath.c
> @@ -44,14 +44,11 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target,
>                                            unsigned long long *capacity)
>  {
>      int ret = -1;
> -    int fd = -1;
> +    int fdret, fd = -1;
>  
> -    if ((fd = open(target->path, O_RDONLY)) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot open volume '%s'"),
> -                             target->path);
> +    if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
>          goto out;
> -    }
> +    fd = fdret;
>  
>      if (virStorageBackendUpdateVolTargetInfoFD(target,
>                                                 fd,
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 40f4fd8..71492cf 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -135,14 +135,12 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target,
>                                           unsigned long long *allocation,
>                                           unsigned long long *capacity)
>  {
> -    int fd, ret = -1;
> +    int fdret, fd = -1;
> +    int ret = -1;
>  
> -    if ((fd = open(target->path, O_RDONLY)) < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot open volume '%s'"),
> -                             target->path);
> -        return -1;
> -    }
> +    if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
> +        goto cleanup;
> +    fd = fdret;
>  
>      if (virStorageBackendUpdateVolTargetInfoFD(target,
>                                                 fd,
> @@ -155,8 +153,9 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target,
>  
>      ret = 0;
>  
> -  cleanup:
> -    close(fd);
> +cleanup:
> +    if (fd >= 0)
> +        close(fd);
>  
>      return ret;
>  }


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]