11:36 a.m.
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