1:50 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 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