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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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 12/04/2015 08:46 AM, Ján Tomko wrote: 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. 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 12/04/2015 09:55 AM, Ján Tomko wrote: 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

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(a)redhat.com&gt; --- 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