4:23 p.m.
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
4:23 p.m.
New subject: [libvirt] [PATCH v3 1/6] storage: Add readflags for backend error processing
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH v3 2/6] storage: Add comments for backend APIs
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH v3 3/6] storage: Set ret = -1 on failures in virStorageBackendUpdateVolTargetInfo
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH v3 4/6] storage: Handle readflags errors
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH v3 5/6] storage: Add debug message
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(a)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
4:23 p.m.
New subject: [libvirt] [PATCH v3 6/6] storage: Ignore block devices that fail format detection
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>
---
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
5:44 a.m.
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
3301
days inactive
3306
days old
7 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko