7:42 p.m.
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 always report an error, and fix the one caller that
was actually handling -2 to explicitly unset the raised error. Tweak
some of the other call sites to properly propagate the newly raised
error.
---
Unfortunately this makes libvirtd startup pretty noisy on stderr, since
logging is done at ErrorReport time, even if the error is never dispatched,
and every directory pool will try to probe the illegal volumes $target/. and
$target/.. . Suggestions welcome
src/storage/storage_backend.c | 34 ++++++++++++++++++++++------------
src/storage/storage_backend_fs.c | 24 ++++++++++++++----------
src/storage/storage_backend_logical.c | 6 +++++-
src/storage/storage_backend_scsi.c | 4 ++--
4 files changed, 43 insertions(+), 25 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 42bd445..b57d718 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,
@@ -1286,20 +1287,23 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
char *base = last_component(path);
if (lstat(path, sb) < 0) {
+ virReportSystemError(errno, _("cannot stat file '%s'"), path);
+
if (errno == ENOENT && !(flags & VIR_STORAGE_VOL_OPEN_ERROR)) {
VIR_WARN("ignoring missing file '%s'", path);
return -2;
}
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
- path);
return -1;
}
if (S_ISFIFO(sb->st_mode)) {
+ 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)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Volume path '%s' is a socket"), path);
VIR_WARN("ignoring socket '%s'", path);
return -2;
}
@@ -1311,6 +1315,8 @@ 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) {
+ virReportSystemError(errno, _("cannot open volume '%s'"),
path);
+
if ((errno == ENOENT || errno == ELOOP) &&
S_ISLNK(sb->st_mode)) {
VIR_WARN("ignoring dangling symlink '%s'", path);
@@ -1321,9 +1327,6 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
return -2;
}
- virReportSystemError(errno,
- _("cannot open volume '%s'"),
- path);
return -1;
}
@@ -1347,10 +1350,14 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
if (STREQ(base, ".") ||
STREQ(base, "..")) {
VIR_FORCE_CLOSE(fd);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot use volume path '%s'"), path);
VIR_INFO("Skipping special dir '%s'", base);
return -2;
}
} else {
+ 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;
@@ -1366,13 +1373,12 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
if (!(mode & flags)) {
VIR_FORCE_CLOSE(fd);
VIR_INFO("Skipping volume '%s'", path);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected storage mode for '%s'"), path);
if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected storage mode for '%s'"),
path);
return -1;
}
-
return -2;
}
@@ -1389,8 +1395,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..c6cad72 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -82,8 +82,14 @@ 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, so reset the error that was raised, and
+ ensure we propagate the return code since the caller handles it */
+ if (ret == -2) {
+ virResetLastError();
+ }
+ goto error;
+ }
fd = ret;
if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb,
@@ -904,8 +910,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 +1184,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