[libvirt] [PATCH v2 0/5] Add read error ignore to storage backend vol info APIs
v1 took a simplistic approach: http://www.redhat.com/archives/libvir-list/2015-October/msg00919.html but not specific enough according to the review. So adding some extra complexity of checking for specific errors and flag bits to "ignore" the error similar to the catch-all openflags VIR_STORAGE_VOL_OPEN_NOERROR for all the virStorageBackendUpdateVol{Info|TargetInfo|TargetInfoFD} API's which are used to get the volume capacity, allocation, etc. data. Along the way an inavertent bug was found and fixed in patch 3 in virStorageBackendUpdateVolTargetInfo where an error could have been reported, but not propagated back to the user since 'ret' would have been 0 after the virStorageBackendUpdateVolTargetInfoFD call. John Ferlan (5): storage: Add readflags for backend error processing storage: Add comments for backend APIs storage: Handle readflags errors storage: Add debug message storage: Ignore block devices that fail format detection src/storage/storage_backend.c | 151 +++++++++++++++++++++++++++------- src/storage/storage_backend.h | 20 ++++- src/storage/storage_backend_disk.c | 5 +- src/storage/storage_backend_fs.c | 8 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 6 +- 8 files changed, 153 insertions(+), 43 deletions(-) -- 2.5.0
Similar to the openflags which allow VIR_STORAGE_VOL_OPEN_NOERROR to be passed to avoid open errors, add a 'readflags' variable so that in the future various read failures could also be ignored. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 24 ++++++++++++++++-------- src/storage/storage_backend.h | 9 ++++++--- src/storage/storage_backend_disk.c | 5 +++-- src/storage/storage_backend_fs.c | 8 ++++---- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 194736b..38ef9fc 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1391,7 +1391,8 @@ static struct diskType const disk_types[] = { static int virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, - int fd) + int fd, + unsigned int readflags ATTRIBUTE_UNUSED) { size_t i; off_t start; @@ -1580,7 +1581,8 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, bool withBlockVolFormat, - unsigned int openflags) + unsigned int openflags, + unsigned int readflags) { int ret, fd = -1; struct stat sb; @@ -1592,7 +1594,8 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, + readflags)) < 0) goto cleanup; if (target->type == VIR_STORAGE_VOL_FILE && @@ -1622,7 +1625,8 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } if (withBlockVolFormat) { - if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd)) < 0) + if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd, + readflags)) < 0) goto cleanup; } @@ -1636,20 +1640,22 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, - unsigned int openflags) + unsigned int openflags, + unsigned int readflags) { int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, withBlockVolFormat, - openflags)) < 0) + openflags, readflags)) < 0) return ret; if (vol->target.backingStore && (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT | - VIR_STORAGE_VOL_OPEN_NOERROR) < 0)) + VIR_STORAGE_VOL_OPEN_NOERROR, + readflags) < 0)) return ret; return 0; @@ -1660,13 +1666,15 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, via virStorageBackendOpenVol*, or -1 * @sb: details about file (must match @fd, if that is provided) + * @readflags: unused * * Returns 0 for success, -1 on a legitimate error condition. */ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb) + struct stat *sb, + unsigned int readflags ATTRIBUTE_UNUSED) { #if WITH_SELINUX security_context_t filecon = NULL; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 96b5f39..aa9008e 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -192,13 +192,16 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, - unsigned int openflags); + unsigned int openflags, + unsigned int readflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, bool withBlockVolFormat, - unsigned int openflags); + unsigned int openflags, + unsigned int readflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb); + struct stat *sb, + unsigned int readflags); bool virStorageBackendPoolPathIsStable(const char *path); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 7baecc1..a83e340 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -154,14 +154,15 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT | - VIR_STORAGE_VOL_OPEN_NOERROR) == -1) + VIR_STORAGE_VOL_OPEN_NOERROR, + 0) == -1) return -1; vol->target.allocation = 0; vol->target.capacity = (vol->source.extents[0].end - vol->source.extents[0].start); } else { if (virStorageBackendUpdateVolInfo(vol, false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) return -1; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 99ea394..38c7805 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -80,7 +80,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, return rc; /* Take care to propagate rc, it is not always -1 */ fd = rc; - if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, 0) < 0) goto cleanup; if (S_ISDIR(sb.st_mode)) { @@ -921,7 +921,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.backingStore) { ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore, false, - VIR_STORAGE_VOL_OPEN_DEFAULT)); + VIR_STORAGE_VOL_OPEN_DEFAULT, 0)); /* 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. */ @@ -953,7 +953,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &statbuf) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &statbuf, 0) < 0) goto cleanup; /* VolTargetInfoFD doesn't update capacity correctly for the pool case */ @@ -1245,7 +1245,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, /* Refresh allocation / capacity / permissions info in case its changed */ ret = virStorageBackendUpdateVolInfo(vol, false, - VIR_STORAGE_VOL_FS_OPEN_FLAGS); + VIR_STORAGE_VOL_FS_OPEN_FLAGS, 0); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index d2e79bc..b40db27 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -268,7 +268,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup; - if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, 0) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 536e617..96bc4c6 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -162,7 +162,7 @@ virStorageBackendLogicalMakeVol(char **const groups, goto cleanup; if (virStorageBackendUpdateVolInfo(vol, false, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) goto cleanup; nextents = 1; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index ca9a62f..b5b4bb6 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -61,7 +61,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, goto cleanup; if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { + VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 7dd7674..cc2c5d7 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -225,7 +225,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, } if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) + VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) goto cleanup; if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) -- 2.5.0
Just so it's clearer what to expect upon input and what types of return values could be generated. These were loosely copied from existing virStorageBackendUpdateVolTargetInfoFD. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 38ef9fc..c870380 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1389,6 +1389,14 @@ static struct diskType const disk_types[] = { }; +/* + * virStorageBackendDetectBlockVolFormatFD + * @target: target definition ptr of volume to update + * @fd: fd of storage volume to update, + * @readflags: unused + * + * Returns 0 for success, -1 on a legitimate error condition + */ static int virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, int fd, @@ -1578,6 +1586,17 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return fd; } +/* + * virStorageBackendUpdateVolTargetInfo + * @target: target definition ptr of volume to update + * @withBlockVolFormat: true if caller determined a block file + * @openflags: various VolOpenCheckMode flags to handle errors on open + * @readflags: unused + * + * Returns 0 for success, -1 on a legitimate error condition, and -2 + * if the openflags used VIR_STORAGE_VOL_OPEN_NOERROR and some sort of + * open error occurred. It is up to the caller to handle. + */ int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, bool withBlockVolFormat, @@ -1637,6 +1656,17 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, return ret; } +/* + * virStorageBackendUpdateVolInfo + * @vol: Pointer to a volume storage definition + * @withBlockVolFormat: true if the caller determined a block file + * @openflags: various VolOpenCheckMode flags to handle errors on open + * @readflags: unused + * + * Returns 0 for success, -1 on a legitimate error condition, and -2 + * if the openflags used VIR_STORAGE_VOL_OPEN_NOERROR and some sort of + * open error occurred. It is up to the caller to handle. + */ int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withBlockVolFormat, -- 2.5.0
Similar to the openflags VIR_STORAGE_VOL_OPEN_NOERROR processing, if some read processing operation fails, check the readflags for the corresponding error flag being set. If so, rather then causing an error - use VIR_WARN to flag the error, but return -2 which some callers can use to perform specific actions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 107 +++++++++++++++++++++++++++++++----------- src/storage/storage_backend.h | 11 +++++ 2 files changed, 90 insertions(+), 28 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c870380..da830d6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1393,14 +1393,17 @@ static struct diskType const disk_types[] = { * virStorageBackendDetectBlockVolFormatFD * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, - * @readflags: unused + * @readflags: various VolReadErrorMode flags to handle errors after open + * is successful, but some other access/read is not. * - * Returns 0 for success, -1 on a legitimate error condition + * Returns 0 for success, -1 on a legitimate error condition, -2 if + * some sort of error is desired to be ignored (along with appropriate + * VIR_WARN of the issue). */ static int virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, int fd, - unsigned int readflags ATTRIBUTE_UNUSED) + unsigned int readflags) { size_t i; off_t start; @@ -1412,17 +1415,29 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, start = lseek(fd, 0, SEEK_SET); if (start < 0) { - virReportSystemError(errno, - _("cannot seek to beginning of file '%s'"), - target->path); - return -1; + if (readflags & VIR_STORAGE_VOL_SEEK_ERROR) { + VIR_WARN("ignoring failed beginning of file lseek for '%s'", + target->path); + return -2; + } else { + virReportSystemError(errno, + _("cannot seek to beginning of file '%s'"), + target->path); + return -1; + } } bytes = saferead(fd, buffer, sizeof(buffer)); if (bytes < 0) { - virReportSystemError(errno, - _("cannot read beginning of file '%s'"), - target->path); - return -1; + if (readflags & VIR_STORAGE_VOL_READ_ERROR) { + VIR_WARN("ignoring failed saferead of file '%s'", + target->path); + return -2; + } else { + virReportSystemError(errno, + _("cannot read beginning of file '%s'"), + target->path); + return -1; + } } for (i = 0; disk_types[i].part_table_type != -1; i++) { @@ -1591,11 +1606,13 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, * @target: target definition ptr of volume to update * @withBlockVolFormat: true if caller determined a block file * @openflags: various VolOpenCheckMode flags to handle errors on open - * @readflags: unused + * @readflags: various VolReadErrorMode flags to handle errors after open + * is successful, but some other access/read is not. * * Returns 0 for success, -1 on a legitimate error condition, and -2 * if the openflags used VIR_STORAGE_VOL_OPEN_NOERROR and some sort of - * open error occurred. It is up to the caller to handle. + * open error occurred. It is up to the caller to handle. A -2 may also + * be returned if the caller passed a specific readflagsflag. */ int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, @@ -1625,17 +1642,36 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { - virReportSystemError(errno, _("cannot seek to start of '%s'"), target->path); + if (readflags & VIR_STORAGE_VOL_SEEK_ERROR) { + VIR_WARN("ignoring failed beginning of file lseek for '%s'", + target->path); + ret = -2; + } else { + virReportSystemError(errno, + _("cannot seek to start of '%s'"), + target->path); + ret = -1; + } goto cleanup; } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), target->path); + if (readflags & VIR_STORAGE_VOL_READ_ERROR) { + VIR_WARN("ignoring failed header read for '%s'", + target->path); + ret = -2; + } else { + virReportSystemError(errno, + _("cannot read header '%s'"), + target->path); + ret = -1; + } goto cleanup; } - if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, - NULL))) { + if (!(meta = virStorageFileGetMetadataFromBuf(target->path, + buf, len, + target->format, NULL))) { goto cleanup; } @@ -1696,15 +1732,18 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * @target: target definition ptr of volume to update * @fd: fd of storage volume to update, via virStorageBackendOpenVol*, or -1 * @sb: details about file (must match @fd, if that is provided) - * @readflags: unused + * @readflags: various VolReadErrorMode flags to handle errors after open + * is successful, but some other access/read is not. * - * Returns 0 for success, -1 on a legitimate error condition. + * Returns 0 for success, -1 on a legitimate error condition, -2 if + * some sort of error is desired to be ignored (along with appropriate + * VIR_WARN of the issue). */ int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, struct stat *sb, - unsigned int readflags ATTRIBUTE_UNUSED) + unsigned int readflags) { #if WITH_SELINUX security_context_t filecon = NULL; @@ -1733,10 +1772,16 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, */ end = lseek(fd, 0, SEEK_END); if (end == (off_t)-1) { - virReportSystemError(errno, - _("cannot seek to end of file '%s'"), - target->path); - return -1; + if (readflags & VIR_STORAGE_VOL_SEEK_ERROR) { + VIR_WARN("Ignoring failed end of file lseek for '%s'", + target->path); + return -2; + } else { + virReportSystemError(errno, + _("cannot seek to end of file '%s'"), + target->path); + return -1; + } } target->allocation = end; target->capacity = end; @@ -1762,10 +1807,16 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, if (fd >= 0) { if (fgetfilecon_raw(fd, &filecon) == -1) { if (errno != ENODATA && errno != ENOTSUP) { - virReportSystemError(errno, - _("cannot get file context of '%s'"), - target->path); - return -1; + if (readflags & VIR_STORAGE_VOL_FILECON_ERROR) { + VIR_WARN("ignore failed get file context of '%s'", + target->path); + return -2; + } else { + virReportSystemError(errno, + _("cannot get file context of '%s'"), + target->path); + return -1; + } } } else { if (VIR_STRDUP(target->perms->label, filecon) < 0) { diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index aa9008e..e3ff306 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,17 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ }; +/* VolReadErrorMode flags + * If flag is present, then operation won't cause fatal error for + * specified operation, rather a VIR_WARN will be issued and a -2 returned + * for function call + */ +enum { + VIR_STORAGE_VOL_SEEK_ERROR = 1 << 0, /* don't error on (l)seek */ + VIR_STORAGE_VOL_READ_ERROR = 1 << 1, /* don't error on *read */ + VIR_STORAGE_VOL_FILECON_ERROR = 1 << 2, /* don't error on (f)filecon* */ +}; + # define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_REG |\ VIR_STORAGE_VOL_OPEN_BLOCK) -- 2.5.0
On Tue, Nov 24, 2015 at 03:57:14PM -0500, John Ferlan wrote:
Similar to the openflags VIR_STORAGE_VOL_OPEN_NOERROR processing, if some read processing operation fails, check the readflags for the corresponding error flag being set. If so, rather then causing an error - use VIR_WARN to flag the error, but return -2 which some callers can use to perform specific actions.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 107 +++++++++++++++++++++++++++++++----------- src/storage/storage_backend.h | 11 +++++ 2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index aa9008e..e3ff306 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,17 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ };
+/* VolReadErrorMode flags + * If flag is present, then operation won't cause fatal error for + * specified operation, rather a VIR_WARN will be issued and a -2 returned + * for function call + */ +enum { + VIR_STORAGE_VOL_SEEK_ERROR = 1 << 0, /* don't error on (l)seek */
+ VIR_STORAGE_VOL_READ_ERROR = 1 << 1, /* don't error on *read */
This is the only flag used in this series. Also, naming it VIR_STORAGE_VOL_READ_NOERROR or VIR_STORAGE_VOL_READ_IGNORE_ERROR would make its meaning more obvious. ACK with the unused flags dropped. Jan
On 12/04/2015 08:46 AM, Ján Tomko wrote:
On Tue, Nov 24, 2015 at 03:57:14PM -0500, John Ferlan wrote:
Similar to the openflags VIR_STORAGE_VOL_OPEN_NOERROR processing, if some read processing operation fails, check the readflags for the corresponding error flag being set. If so, rather then causing an error - use VIR_WARN to flag the error, but return -2 which some callers can use to perform specific actions.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 107 +++++++++++++++++++++++++++++++----------- src/storage/storage_backend.h | 11 +++++ 2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index aa9008e..e3ff306 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,17 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ };
+/* VolReadErrorMode flags + * If flag is present, then operation won't cause fatal error for + * specified operation, rather a VIR_WARN will be issued and a -2 returned + * for function call + */ +enum { + VIR_STORAGE_VOL_SEEK_ERROR = 1 << 0, /* don't error on (l)seek */
+ VIR_STORAGE_VOL_READ_ERROR = 1 << 1, /* don't error on *read */
This is the only flag used in this series.
Also, naming it VIR_STORAGE_VOL_READ_NOERROR or VIR_STORAGE_VOL_READ_IGNORE_ERROR would make its meaning more obvious.
I can rename flags to be: VIR_STORAGE_VOL_xxx_IGNORE_ERROR or VIR_STORAGE_VOL_IGNORE_xxx_ERROR Do you have a preference on order? I personally didn't find the *_NOERROR to be that obvious, but I agree adding IGNORE at least does make it obvious.
ACK with the unused flags dropped.
Is it really that important to remove the SEEK and FILECON failure checks? I added them mainly to be "complete". Sure having them is overkill; however, it was pointed out the v1 was too broad. Keeping them just means a change in the future won't have to add them. I'm not sure I see the harm, but I'm ambivalent over having to remove them for an ACK. Tks - John
On Fri, Dec 04, 2015 at 09:44:20AM -0500, John Ferlan wrote:
On 12/04/2015 08:46 AM, Ján Tomko wrote:
On Tue, Nov 24, 2015 at 03:57:14PM -0500, John Ferlan wrote:
Similar to the openflags VIR_STORAGE_VOL_OPEN_NOERROR processing, if some read processing operation fails, check the readflags for the corresponding error flag being set. If so, rather then causing an error - use VIR_WARN to flag the error, but return -2 which some callers can use to perform specific actions.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 107 +++++++++++++++++++++++++++++++----------- src/storage/storage_backend.h | 11 +++++ 2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index aa9008e..e3ff306 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,17 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ };
+/* VolReadErrorMode flags + * If flag is present, then operation won't cause fatal error for + * specified operation, rather a VIR_WARN will be issued and a -2 returned + * for function call + */ +enum { + VIR_STORAGE_VOL_SEEK_ERROR = 1 << 0, /* don't error on (l)seek */
+ VIR_STORAGE_VOL_READ_ERROR = 1 << 1, /* don't error on *read */
This is the only flag used in this series.
Also, naming it VIR_STORAGE_VOL_READ_NOERROR or VIR_STORAGE_VOL_READ_IGNORE_ERROR would make its meaning more obvious.
I can rename flags to be:
VIR_STORAGE_VOL_xxx_IGNORE_ERROR
or
VIR_STORAGE_VOL_IGNORE_xxx_ERROR
Do you have a preference on order?
VIR_STORAGE_VOL_READ_xxx for VolReadErrorMode flags, similar to VIR_STORAGE_VOL_OPEN_xxx for VolOpenCheckMode flags.
I personally didn't find the *_NOERROR to be that obvious, but I agree adding IGNORE at least does make it obvious.
ACK with the unused flags dropped.
Is it really that important to remove the SEEK and FILECON failure checks? I added them mainly to be "complete".
Yes, not introducing unused code means there is less code to read when trying to figure out what the code does.
Sure having them is overkill; however, it was pointed out the v1 was too broad. Keeping them just means a change in the future won't have to add them. I'm not sure I see the harm, but I'm ambivalent over having to remove them for an ACK.
There is also a chance that there might never be a change that needs them. Jan
On 12/04/2015 09:55 AM, Ján Tomko wrote:
On Fri, Dec 04, 2015 at 09:44:20AM -0500, John Ferlan wrote:
On 12/04/2015 08:46 AM, Ján Tomko wrote:
On Tue, Nov 24, 2015 at 03:57:14PM -0500, John Ferlan wrote:
Similar to the openflags VIR_STORAGE_VOL_OPEN_NOERROR processing, if some read processing operation fails, check the readflags for the corresponding error flag being set. If so, rather then causing an error - use VIR_WARN to flag the error, but return -2 which some callers can use to perform specific actions.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 107 +++++++++++++++++++++++++++++++----------- src/storage/storage_backend.h | 11 +++++ 2 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index aa9008e..e3ff306 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,17 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ };
+/* VolReadErrorMode flags + * If flag is present, then operation won't cause fatal error for + * specified operation, rather a VIR_WARN will be issued and a -2 returned + * for function call + */ +enum { + VIR_STORAGE_VOL_SEEK_ERROR = 1 << 0, /* don't error on (l)seek */
+ VIR_STORAGE_VOL_READ_ERROR = 1 << 1, /* don't error on *read */
This is the only flag used in this series.
Also, naming it VIR_STORAGE_VOL_READ_NOERROR or VIR_STORAGE_VOL_READ_IGNORE_ERROR would make its meaning more obvious.
I can rename flags to be:
VIR_STORAGE_VOL_xxx_IGNORE_ERROR
or
VIR_STORAGE_VOL_IGNORE_xxx_ERROR
Do you have a preference on order?
VIR_STORAGE_VOL_READ_xxx for VolReadErrorMode flags, similar to VIR_STORAGE_VOL_OPEN_xxx for VolOpenCheckMode flags.
OK, removing the flags causes changes to: virStorageBackendUpdateVolTargetInfoFD effecting src/storage/storage_backend.{c,h} src/storage/storage_backend_{fs,gluster}.c This affects patch 1 (to remove readflags from the API) and patch 3 to remove the -2 return value comments from the API as well as adjustments to other API's to touch up the 'readflags'. I also needed to insert a new patch 2.5 which would handle errors in virStorageBackendUpdateVolTargetInfo while processing the fd. If you look at the code before my changes, failures from calls to lseek, virFileReadHeaderFD, and virStorageFileGetMetadataFromBuf would result in a virReportSystemError; however, the "ret" would remain what was returned from the virStorageBackendUpdateVolTargetInfoFD (eg. 0 - zero). Adding those "ignored" errors had also served the duplicate purpose of actually returning an error code. So would you like to see the whole series? John
I found this useful while processing a volume that wouldn't end up showing up in the resulting list of block volumes. In this case, the partition type wasn't found in the disk_types table. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index da830d6..d7ba916 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1450,6 +1450,10 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } } + if (target->format == VIR_STORAGE_POOL_DISK_UNKNOWN) + VIR_DEBUG("cannot determine the target format for '%s'", + target->path); + return 0; } -- 2.5.0
https://bugzilla.redhat.com/show_bug.cgi?id=1276198 Prior to commit id '98322052' failure to saferead the block device would cause an error to be logged and the device to be skipped while attempting to discover/create a stable target path for a new LUN (NPIV). This was because virStorageBackendSCSIFindLUs ignored errors from processLU and virStorageBackendSCSINewLun. Ignoring the failure allowed a multipath device with an "active" and "ghost" to be present on the host with the "ghost" block device being ignored. This patch will return a -2 to the caller indicating the desire to ignore the block device since it cannot be used directly rather than fail the pool startup. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index cc2c5d7..27c90f6 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -224,8 +224,10 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto cleanup; } - if (virStorageBackendUpdateVolInfo(vol, true, - VIR_STORAGE_VOL_OPEN_DEFAULT, 0) < 0) + /* Allow a volume read failure to ignore or skip this block file */ + if ((retval = virStorageBackendUpdateVolInfo(vol, true, + VIR_STORAGE_VOL_OPEN_DEFAULT, + VIR_STORAGE_VOL_READ_ERROR)) < 0) goto cleanup; if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) -- 2.5.0
On Tue, Nov 24, 2015 at 03:57:11PM -0500, John Ferlan wrote:
v1 took a simplistic approach:
http://www.redhat.com/archives/libvir-list/2015-October/msg00919.html
but not specific enough according to the review. So adding some extra complexity of checking for specific errors and flag bits to "ignore" the error similar to the catch-all openflags VIR_STORAGE_VOL_OPEN_NOERROR for all the virStorageBackendUpdateVol{Info|TargetInfo|TargetInfoFD} API's which are used to get the volume capacity, allocation, etc. data.
Along the way an inavertent bug was found and fixed in patch 3 in virStorageBackendUpdateVolTargetInfo where an error could have been reported, but not propagated back to the user since 'ret' would have been 0 after the virStorageBackendUpdateVolTargetInfoFD call.
John Ferlan (5): storage: Add readflags for backend error processing storage: Add comments for backend APIs storage: Handle readflags errors storage: Add debug message storage: Ignore block devices that fail format detection
src/storage/storage_backend.c | 151 +++++++++++++++++++++++++++------- src/storage/storage_backend.h | 20 ++++- src/storage/storage_backend_disk.c | 5 +- src/storage/storage_backend_fs.c | 8 +- src/storage/storage_backend_gluster.c | 2 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 6 +- 8 files changed, 153 insertions(+), 43 deletions(-)
ACK series, with one comment on patch 3/5. Jan
participants (2)
-
John Ferlan -
Ján Tomko