[libvirt] [PATCH v3 0/6] Add read error ignore to storage backend vol info APIs
v2 here: http://www.redhat.com/archives/libvir-list/2015-November/thread.html Based on review and well just the amount of change I figured I'd generate a v3. The patches were mostly ACK'd, but patch 3 required removal of some flags which resulted in patch 1 needing updates... Then I found that what I'd done in patch 3 regarding returning errors needed to generate a patch 2.5... So I just bit the bullet and am posting the patches again. Patches 2, 4, and 5 of this series are unchanged. While patch 1 removes 'readflags' from an API, patch 3 is new, and patch 6 uses the new name. John Ferlan (6): storage: Add readflags for backend error processing storage: Add comments for backend APIs storage: Set ret = -1 on failures in virStorageBackendUpdateVolTargetInfo storage: Handle readflags errors storage: Add debug message storage: Ignore block devices that fail format detection src/storage/storage_backend.c | 83 ++++++++++++++++++++++++++++++----- src/storage/storage_backend.h | 15 ++++++- src/storage/storage_backend_disk.c | 5 ++- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 6 ++- 7 files changed, 96 insertions(+), 21 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 read failures could also be ignored. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 17 +++++++++++------ src/storage/storage_backend.h | 6 ++++-- src/storage/storage_backend_disk.c | 5 +++-- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 7 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 194736b..94b0b3f 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; @@ -1622,7 +1624,8 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } if (withBlockVolFormat) { - if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd)) < 0) + if ((ret = virStorageBackendDetectBlockVolFormatFD(target, fd, + readflags)) < 0) goto cleanup; } @@ -1636,20 +1639,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; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 96b5f39..64c46ee 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -192,10 +192,12 @@ 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); 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..c71c724 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -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. */ @@ -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_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 94b0b3f..1ee83aa 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, @@ -1636,6 +1655,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
While processing the volume for lseek, virFileReadHeaderFD, and virStorageFileGetMetadataFromBuf - failure would cause an error, but ret would not be set. That would result in an error message being sent, but successful status being returned. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 1ee83aa..8eb5b04 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1625,16 +1625,19 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { 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); + ret = -1; goto cleanup; } if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, NULL))) { + ret = -1; goto cleanup; } -- 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. Use a new VIR_STORAGE_VOL_READ_NOERROR flag in a new VolReadErrorMode enum. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 43 +++++++++++++++++++++++++++++++------------ src/storage/storage_backend.h | 9 +++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 8eb5b04..c29b162 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: VolReadErrorMode flags to handle read error after open + * is successful, but 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 + * the read 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; @@ -1419,10 +1422,16 @@ virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, } 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_NOERROR) { + 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 +1600,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: VolReadErrorMode flags to handle read error after open + * is successful, but 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 readflagsflag. */ int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, @@ -1630,8 +1641,16 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } if ((len = virFileReadHeaderFD(fd, len, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%s'"), target->path); - ret = -1; + if (readflags & VIR_STORAGE_VOL_READ_NOERROR) { + 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; } @@ -1663,7 +1682,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, * @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 + * @readflags: various VolReadErrorMode flags to handle errors on read * * 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 diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 64c46ee..20e6079 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -179,6 +179,15 @@ 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_READ_NOERROR = 1 << 0, /* ignore *read errors */ +}; + # define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_REG |\ VIR_STORAGE_VOL_OPEN_BLOCK) -- 2.5.0
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 c29b162..6915b8a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1444,6 +1444,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..670cc4d 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_NOERROR)) < 0) goto cleanup; if (!(vol->key = virStorageBackendSCSISerial(vol->target.path))) -- 2.5.0
On Fri, Dec 04, 2015 at 05:23:47PM -0500, John Ferlan wrote:
v2 here: http://www.redhat.com/archives/libvir-list/2015-November/thread.html
Based on review and well just the amount of change I figured I'd generate a v3. The patches were mostly ACK'd, but patch 3 required removal of some flags which resulted in patch 1 needing updates... Then I found that what I'd done in patch 3 regarding returning errors needed to generate a patch 2.5... So I just bit the bullet and am posting the patches again. Patches 2, 4, and 5 of this series are unchanged. While patch 1 removes 'readflags' from an API, patch 3 is new, and patch 6 uses the new name.
John Ferlan (6): storage: Add readflags for backend error processing storage: Add comments for backend APIs storage: Set ret = -1 on failures in virStorageBackendUpdateVolTargetInfo storage: Handle readflags errors storage: Add debug message storage: Ignore block devices that fail format detection
src/storage/storage_backend.c | 83 ++++++++++++++++++++++++++++++----- src/storage/storage_backend.h | 15 ++++++- src/storage/storage_backend_disk.c | 5 ++- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 6 ++- 7 files changed, 96 insertions(+), 21 deletions(-)
ACK series Jan
participants (2)
-
John Ferlan -
Ján Tomko