[libvirt] [PATCH v3] 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 | 77 ++++++++++++++++++++++++-------------- src/storage/storage_backend.h | 7 ++-- src/storage/storage_backend_fs.c | 28 +++++--------- src/storage/storage_backend_scsi.c | 3 -- 4 files changed, 62 insertions(+), 53 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 8fe3687..f44e323 100644 --- a/src/storage/storage_backend.c +++ b/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 - * volume is a dangling symbolic link. + * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we + * 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); - return -2; + if (noerror) { + 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); - return -2; + if (noerror) { + 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, - _("cannot open volume '%s'"), - path); + 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; } @@ -1351,33 +1359,46 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, if (STREQ(base, ".") || STREQ(base, "..")) { VIR_FORCE_CLOSE(fd); - VIR_INFO("Skipping special dir '%s'", base); - return -2; + if (noerror) { + 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); + if (noerror) { + VIR_WARN("unable to set blocking mode for '%s'", path); + return -2; + } virReportSystemError(errno, _("unable to set blocking mode for '%s'"), path); - VIR_FORCE_CLOSE(fd); - return -2; + return -1; } if (!(mode & flags)) { VIR_FORCE_CLOSE(fd); - VIR_INFO("Skipping volume '%s'", path); - - if (mode & VIR_STORAGE_VOL_OPEN_ERROR) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected storage mode for '%s'"), path); - return -1; + if (noerror) { + VIR_INFO("Skipping volume '%s'", path); + return -2; } - return -2; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected storage mode for '%s'"), path); + return -1; } return fd; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 89511f8..c695ef7 100644 --- a/src/storage/storage_backend.h +++ b/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 - * encountered */ + VIR_STORAGE_VOL_OPEN_NOERROR = 1 << 0, /* don't error if unexpected type + * encountered, just warn */ VIR_STORAGE_VOL_OPEN_REG = 1 << 1, /* regular 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_DIR = 1 << 4, /* directories okay */ }; -# define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR |\ - VIR_STORAGE_VOL_OPEN_REG |\ +# define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_REG |\ VIR_STORAGE_VOL_OPEN_BLOCK) int virStorageBackendVolOpen(const char *path, struct stat *sb, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de6521c..9b958f0 100644 --- a/src/storage/storage_backend_fs.c +++ b/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 |\ - VIR_STORAGE_VOL_OPEN_DIR) -#define VIR_STORAGE_VOL_FS_REFRESH_FLAGS (VIR_STORAGE_VOL_FS_OPEN_FLAGS &\ - ~VIR_STORAGE_VOL_OPEN_ERROR) +#define VIR_STORAGE_VOL_FS_OPEN_FLAGS (VIR_STORAGE_VOL_OPEN_DEFAULT &\ + VIR_STORAGE_VOL_OPEN_DIR) +#define VIR_STORAGE_VOL_FS_PROBE_FLAGS (VIR_STORAGE_VOL_FS_OPEN_FLAGS &\ + 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,11 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.path = backingStore; vol->backingStore.format = backingStoreFormat; - if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { - /* The backing file is currently unavailable, the capacity, - * allocation, owner, group and mode are unknown. Just log the - * error and 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); - } + virStorageBackendUpdateVolTargetInfo(&vol->backingStore, false, + VIR_STORAGE_VOL_OPEN_DEFAULT); + /* If this failed, the backing file is currently unavailable, + * the capacity, allocation, owner, group and mode are unknown. + * An error message was raised, but we just continue. */ } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index cf546fb..c448d7f 100644 --- a/src/storage/storage_backend_scsi.c +++ b/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; } -- 1.9.0
On 04/02/2014 09:54 AM, Cole Robinson wrote:
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 | 77 ++++++++++++++++++++++++-------------- src/storage/storage_backend.h | 7 ++-- src/storage/storage_backend_fs.c | 28 +++++--------- src/storage/storage_backend_scsi.c | 3 -- 4 files changed, 62 insertions(+), 53 deletions(-)
+ * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we + * return -2 if file mode is unexpected or the volume is a dangling + * symbolic link.
Seems fair; let's see how it works below...
*/ 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);
Whoops[1] - that returns true for any non-zero flags value. I think you meant s/*/&/
} 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;
...so far, so good (all cases of 'return -2' are guarded by noerror, and an error is issued before returning -1).
}
if (virSetBlocking(fd, true) < 0) { + VIR_FORCE_CLOSE(fd); + if (noerror) { + VIR_WARN("unable to set blocking mode for '%s'", path); + return -2; + }
But this one feels wrong[2]. If we get here, we KNOW the fd has the expected type, and we successfully opened it. This is a case where the system is hosed, and we should loudly and unconditionally fail with -1, rather than returning -2 on noerror.
/* VolOpenCheckMode flags */ enum { - VIR_STORAGE_VOL_OPEN_ERROR = 1 << 0, /* warn if unexpected type - * encountered */ + VIR_STORAGE_VOL_OPEN_NOERROR = 1 << 0, /* don't error if unexpected type + * encountered, just warn */ VIR_STORAGE_VOL_OPEN_REG = 1 << 1, /* regular 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_DIR = 1 << 4, /* directories okay */
Cosmetic, but alignment of = is now off [3].
- if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { - /* The backing file is currently unavailable, the capacity, - * allocation, owner, group and mode are unknown. Just log the - * error and 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); - } + virStorageBackendUpdateVolTargetInfo(&vol->backingStore, false, + VIR_STORAGE_VOL_OPEN_DEFAULT); + /* If this failed, the backing file is currently unavailable, + * the capacity, allocation, owner, group and mode are unknown. + * An error message was raised, but we just continue. */
You may need an ignore_value() here to keep Coverity quiet about an unchecked error return [4]. This touches code that I'm also working on, so I'm interested in getting it in sooner to avoid rebase churn over repeated iterations. ACK if you fix [1] and [2] above; and up to you whether changes are needed at [3] and [4]. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/02/2014 12:36 PM, Eric Blake wrote:
On 04/02/2014 09:54 AM, Cole Robinson wrote:
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 | 77 ++++++++++++++++++++++++-------------- src/storage/storage_backend.h | 7 ++-- src/storage/storage_backend_fs.c | 28 +++++--------- src/storage/storage_backend_scsi.c | 3 -- 4 files changed, 62 insertions(+), 53 deletions(-)
+ * Returns -1 on error. If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we + * return -2 if file mode is unexpected or the volume is a dangling + * symbolic link.
Seems fair; let's see how it works below...
*/ 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);
Whoops[1] - that returns true for any non-zero flags value. I think you meant s/*/&/
Doh, sorry bout that. Fixed.
} 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;
...so far, so good (all cases of 'return -2' are guarded by noerror, and an error is issued before returning -1).
}
if (virSetBlocking(fd, true) < 0) { + VIR_FORCE_CLOSE(fd); + if (noerror) { + VIR_WARN("unable to set blocking mode for '%s'", path); + return -2; + }
But this one feels wrong[2]. If we get here, we KNOW the fd has the expected type, and we successfully opened it. This is a case where the system is hosed, and we should loudly and unconditionally fail with -1, rather than returning -2 on noerror.
Agreed, I was just preserving the original behavior. Fixed to unconditionally raise error and return -1.
/* VolOpenCheckMode flags */ enum { - VIR_STORAGE_VOL_OPEN_ERROR = 1 << 0, /* warn if unexpected type - * encountered */ + VIR_STORAGE_VOL_OPEN_NOERROR = 1 << 0, /* don't error if unexpected type + * encountered, just warn */ VIR_STORAGE_VOL_OPEN_REG = 1 << 1, /* regular 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_DIR = 1 << 4, /* directories okay */
Cosmetic, but alignment of = is now off [3].
Fixed.
- if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { - /* The backing file is currently unavailable, the capacity, - * allocation, owner, group and mode are unknown. Just log the - * error and 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); - } + virStorageBackendUpdateVolTargetInfo(&vol->backingStore, false, + VIR_STORAGE_VOL_OPEN_DEFAULT); + /* If this failed, the backing file is currently unavailable, + * the capacity, allocation, owner, group and mode are unknown. + * An error message was raised, but we just continue. */
You may need an ignore_value() here to keep Coverity quiet about an unchecked error return [4].
This touches code that I'm also working on, so I'm interested in getting it in sooner to avoid rebase churn over repeated iterations. ACK if you fix [1] and [2] above; and up to you whether changes are needed at [3] and [4].
Fixed with ignore_value() and pushed now. Thanks Eric! - Cole
participants (2)
-
Cole Robinson -
Eric Blake