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

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

 



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);
 
     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);
         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;
     }
@@ -1311,26 +1320,25 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
      * blocking fd, so fix it up after verifying we avoided a
      * race.  */
     if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
+        if (raise_error) {
+            virReportSystemError(errno, _("cannot open volume '%s'"), path);
+        }
+
         if ((errno == ENOENT || errno == ELOOP) &&
             S_ISLNK(sb->st_mode)) {
             VIR_WARN("ignoring dangling symlink '%s'", path);
             return -2;
         }
-        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 open volume '%s'"),
-                             path);
         return -1;
     }
 
     if (fstat(fd, sb) < 0) {
-        virReportSystemError(errno,
-                             _("cannot stat file '%s'"),
-                             path);
+        virReportSystemError(errno, _("cannot stat file '%s'"), path);
         VIR_FORCE_CLOSE(fd);
         return -1;
     }
@@ -1347,18 +1355,30 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
         if (STREQ(base, ".") ||
             STREQ(base, "..")) {
             VIR_FORCE_CLOSE(fd);
+            if (raise_error) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("Cannot use volume path '%s'"), path);
+            }
             VIR_INFO("Skipping special dir '%s'", base);
             return -2;
         }
     } else {
+        if (raise_error) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("unexpected type for file '%s'"), path);
+        }
         VIR_WARN("ignoring unexpected type for file '%s'", path);
         VIR_FORCE_CLOSE(fd);
         return -2;
     }
 
     if (virSetBlocking(fd, true) < 0) {
-        virReportSystemError(errno, _("unable to set blocking mode for '%s'"),
-                             path);
+        if (raise_error) {
+           virReportSystemError(errno,
+                                _("unable to set blocking mode for '%s'"),
+                                path);
+        }
+        VIR_WARN("Unable to set blocking mode for '%s'", path);
         VIR_FORCE_CLOSE(fd);
         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) {
+        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;
+    }
     fd = ret;
 
     if ((ret = virStorageBackendUpdateVolTargetInfoFD(target,
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index e02d17f..4d44897 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -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
+         * can handle it */
+        goto error;
+    }
     fd = ret;
 
     if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb,
@@ -904,8 +907,9 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
                  * 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);
+                               _("cannot probe backing volume path '%s': %s"),
+                               vol->backingStore.path,
+                               virGetLastErrorMessage());
             }
         }
 
@@ -1177,13 +1181,10 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn,
                                       virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
                                       virStorageVolDefPtr vol)
 {
-    int ret;
-
     /* Refresh allocation / permissions info in case its changed */
-    ret = virStorageBackendUpdateVolInfo(vol, false, false,
-                                         VIR_STORAGE_VOL_FS_OPEN_FLAGS);
-    if (ret < 0)
-        return ret;
+    if (virStorageBackendUpdateVolInfo(vol, false, false,
+                                       VIR_STORAGE_VOL_FS_OPEN_FLAGS) < 0)
+        return -1;
 
     /* Load any secrets if posible */
     if (vol->target.encryption &&
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index 7893626..4699dfb 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -762,8 +762,12 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
     cmd = NULL;
 
     if ((fd = virStorageBackendVolOpen(vol->target.path, &sb,
-                                       VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0)
+                                       VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) {
+        /* This returns -2 for potentially skipable errors, but we
+         * want to report them all. */
+        fd = -1;
         goto error;
+    }
 
     /* We can only chown/grp if root */
     if (geteuid() == 0) {
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 4c2484d..51404ff 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -202,8 +202,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
     if (virStorageBackendUpdateVolInfo(vol, true, true,
                                        VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Failed to update volume for '%s'"),
-                       devpath);
+                       _("Failed to update volume for '%s': %s"),
+                       devpath, virGetLastErrorMessage());
         retval = -1;
         goto free_vol;
     }
-- 
1.8.5.3

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]