storage: Report error from VolOpen by default

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

@@ -1279,8 +1279,9 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target,
 /*
  * Allows caller to silently ignore files with improper mode
  *
- * Returns -1 on error, -2 if file mode is unexpected or the
+ * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we
- * volume is a dangling symbolic link.
+ * return -2 if file mode is unexpected or the volume is a dangling
+ * symbolic link.
  */
 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);
 
     if (lstat(path, sb) < 0) {
-        if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+        if (errno == ENOENT && noerror) {
             VIR_WARN("ignoring missing file '%s'", path);
             return -2;
         }
@@ -1301,11 +1303,21 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
     }
 
     if (S_ISFIFO(sb->st_mode)) {
-        VIR_WARN("ignoring FIFO '%s'", path);
+        if (noerror) {
-        return -2;
+            VIR_WARN("ignoring FIFO '%s'", path);
+            return -2;
+        }
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Volume path '%s' is a FIFO"), path);
+        return -1;
     } else if (S_ISSOCK(sb->st_mode)) {
-        VIR_WARN("ignoring socket '%s'", path);
+        if (noerror) {
-        return -2;
+            VIR_WARN("ignoring socket '%s'", path);
+            return -2;
+        }
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Volume path '%s' is a socket"), path);
+        return -1;
     }
 
     /* O_NONBLOCK should only matter during open() for fifos and
@@ -1316,25 +1328,21 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
      * race.  */
     if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
         if ((errno == ENOENT || errno == ELOOP) &&
-            S_ISLNK(sb->st_mode)) {
+            S_ISLNK(sb->st_mode) && noerror) {
             VIR_WARN("ignoring dangling symlink '%s'", path);
             return -2;
         }
-        if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
+        if (errno == ENOENT && noerror) {
             VIR_WARN("ignoring missing file '%s'", path);
             return -2;
         }
 
-        virReportSystemError(errno,
+        virReportSystemError(errno, _("cannot open volume '%s'"), path);
-                             _("cannot open volume '%s'"),
-                             path);
         return -1;
     }
 
     if (fstat(fd, sb) < 0) {
-        virReportSystemError(errno,
+        virReportSystemError(errno, _("cannot stat file '%s'"), path);
-                             _("cannot stat file '%s'"),
-                             path);
         VIR_FORCE_CLOSE(fd);
         return -1;
     }
@@ -1351,33 +1359,42 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
         if (STREQ(base, ".") ||
             STREQ(base, "..")) {
             VIR_FORCE_CLOSE(fd);
-            VIR_INFO("Skipping special dir '%s'", base);
+            if (noerror) {
-            return -2;
+                VIR_INFO("Skipping special dir '%s'", base);
+                return -2;
+            }
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Cannot use volume path '%s'"), path);
+            return -1;
         }
     } 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;
     }
 
     if (virSetBlocking(fd, true) < 0) {
+        VIR_FORCE_CLOSE(fd);
         virReportSystemError(errno, _("unable to set blocking mode for '%s'"),
                              path);
-        VIR_FORCE_CLOSE(fd);
+        return -1;
-        return -2;
     }
 
     if (!(mode & flags)) {
         VIR_FORCE_CLOSE(fd);
-        VIR_INFO("Skipping volume '%s'", path);
+        if (noerror) {
-
+            VIR_INFO("Skipping volume '%s'", path);
-        if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {
+            return -2;
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           _("unexpected storage mode for '%s'"), path);
-            return -1;
         }
 
-        return -2;
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("unexpected storage mode for '%s'"), path);
+        return -1;
     }
 
     return fd;

src/storage/storage_backend.h

@@ -120,16 +120,15 @@ virStorageBackendPtr virStorageBackendForType(int type);
 
 /* VolOpenCheckMode flags */
 enum {
-    VIR_STORAGE_VOL_OPEN_ERROR  = 1 << 0, /* warn if unexpected type
+    VIR_STORAGE_VOL_OPEN_NOERROR = 1 << 0, /* don't error if unexpected type
-                                           * encountered */
+                                            * encountered, just warn */
-    VIR_STORAGE_VOL_OPEN_REG    = 1 << 1, /* regular files okay */
+    VIR_STORAGE_VOL_OPEN_REG     = 1 << 1, /* regular files okay */
-    VIR_STORAGE_VOL_OPEN_BLOCK  = 1 << 2, /* block 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_CHAR    = 1 << 3, /* char files okay */
-    VIR_STORAGE_VOL_OPEN_DIR    = 1 << 4, /* directories okay */
+    VIR_STORAGE_VOL_OPEN_DIR     = 1 << 4, /* directories okay */
 };
 
-# define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR    |\
+# define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_REG      |\
-                                       VIR_STORAGE_VOL_OPEN_REG      |\
                                        VIR_STORAGE_VOL_OPEN_BLOCK)
 
 int virStorageBackendVolOpen(const char *path, struct stat *sb,

src/storage/storage_backend_fs.c

@@ -56,10 +56,10 @@
 
 VIR_LOG_INIT("storage.storage_backend_fs");
 
-#define VIR_STORAGE_VOL_FS_OPEN_FLAGS       (VIR_STORAGE_VOL_OPEN_DEFAULT   |\
+#define VIR_STORAGE_VOL_FS_OPEN_FLAGS    (VIR_STORAGE_VOL_OPEN_DEFAULT | \
-                                             VIR_STORAGE_VOL_OPEN_DIR)
+                                          VIR_STORAGE_VOL_OPEN_DIR)
-#define VIR_STORAGE_VOL_FS_REFRESH_FLAGS    (VIR_STORAGE_VOL_FS_OPEN_FLAGS  &\
+#define VIR_STORAGE_VOL_FS_PROBE_FLAGS   (VIR_STORAGE_VOL_FS_OPEN_FLAGS | \
-                                             ~VIR_STORAGE_VOL_OPEN_ERROR)
+                                          VIR_STORAGE_VOL_OPEN_NOERROR)
 
 static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
 virStorageBackendProbeTarget(virStorageSourcePtr target,
@@ -80,7 +80,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
         *encryption = NULL;
 
     if ((ret = virStorageBackendVolOpen(target->path, &sb,
-                                        VIR_STORAGE_VOL_FS_REFRESH_FLAGS)) < 0)
+                                        VIR_STORAGE_VOL_FS_PROBE_FLAGS)) < 0)
         goto error; /* Take care to propagate ret, it is not always -1 */
     fd = ret;
 
@@ -902,19 +902,12 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
             vol->backingStore.path = backingStore;
             vol->backingStore.format = backingStoreFormat;
 
-            if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore,
+            ignore_value(virStorageBackendUpdateVolTargetInfo(
-                                                     false,
+                                               &vol->backingStore, false,
-                                        VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
+                                               VIR_STORAGE_VOL_OPEN_DEFAULT));
-                /* The backing file is currently unavailable, the capacity,
+            /* If this failed, the backing file is currently unavailable,
-                 * allocation, owner, group and mode are unknown. Just log the
+             * the capacity, allocation, owner, group and mode are unknown.
-                 * error and continue.
+             * An error message was raised, but we just 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);
-            }
         }
 
 

src/storage/storage_backend_scsi.c

@@ -201,9 +201,6 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
 
     if (virStorageBackendUpdateVolInfo(vol, true,
                                        VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("Failed to update volume for '%s'"),
-                       devpath);
         retval = -1;
         goto free_vol;
     }