Re: [libvirt] [PATCH v3] storage: Report error from VolOpen by default

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].