On 03/31/2014 08:50 PM, Cole Robinson wrote:
> 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);
We've been using !!(flags & FLAG) for this kind of assignment, but I can't
remember what that fixes.
>
> 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);
We don't need both an error and a warning. And I think we should return -1 if
an error has been logged.
> 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;
> }
> @@ -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) {
Hmm, it seems this condition could never have been true before.
> + 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;
This and the snipped changes below would not be needed if we never returned -2
with VOL_OPEN_ERROR (or by default if we switch the flag to NOERROR as Eric
suggested.)
> + }
> fd = ret;
>
> if ((ret = virStorageBackendUpdateVolTargetInfoFD(target,
Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list