6:59 a.m.
Adjustments based on recent activity...
The !writelabel path of patch 3 is probably a moot point since patches
to check at FS and Logical startup for a valid data on the device(s)
were reverted.
John Ferlan (3):
storage: Alter logic when both BLKID and PARTED unavailable
storage: Clean up return value checking
storage: Alter error message in probe/empty checks
src/storage/storage_backend.c | 44 ++++++++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 19 deletions(-)
--
2.7.4
6:59 a.m.
New subject: [libvirt] [PATCH 1/3] storage: Alter logic when both BLKID and PARTED unavailable
If neither BLKID or PARTED is available and we're not writing, then
just return 0 which allows the underlying storage pool to generate
a failur. If both are unavailable and we're writing, then generate
a more generic error message.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 18433e9..6bdfbf1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2842,9 +2842,6 @@ virStorageBackendBLKIDFindEmpty(const char *device
ATTRIBUTE_UNUSED,
const char *format ATTRIBUTE_UNUSED,
bool writelabel ATTRIBUTE_UNUSED)
{
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("probing for filesystems is unsupported "
- "by this build"));
return -2;
}
@@ -2868,11 +2865,10 @@ virStorageBackendPARTEDValidLabel(const char *device
ATTRIBUTE_UNUSED,
const char *format ATTRIBUTE_UNUSED,
bool writelabel ATTRIBUTE_UNUSED)
{
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("PARTED is unsupported by this build"));
- return -1;
+ return -2;
}
+
#endif /* #if WITH_STORAGE_DISK */
@@ -2898,5 +2894,17 @@ virStorageBackendDeviceIsEmpty(const char *devpath,
writelabel)) == -2)
ret = virStorageBackendPARTEDValidLabel(devpath, format, writelabel);
+ /* Neither BLKID nor PARTED available, but we're not writing,
+ * so no mechanism to check, so allow a lower layer to reject. */
+ if (ret == -2 && !writelabel)
+ return 0;
+
+ if (ret == -2) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Unable to probe '%s' for existing data, "
+ "requires create/build using overwrite"),
+ devpath);
+ }
+
return ret == 0;
}
--
2.7.4
9:01 a.m.
New subject: [libvirt] [PATCH 1/3] storage: Alter logic when both BLKID and PARTED unavailable
On Fri, Jan 13, 2017 at 07:59:08 -0500, John Ferlan wrote:
If neither BLKID or PARTED is available and we're not writing,
then
just return 0 which allows the underlying storage pool to generate
a failur. If both are unavailable and we're writing, then generate
failure
a more generic error message.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 18433e9..6bdfbf1 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2842,9 +2842,6 @@ virStorageBackendBLKIDFindEmpty(const char *device
ATTRIBUTE_UNUSED,
const char *format ATTRIBUTE_UNUSED,
bool writelabel ATTRIBUTE_UNUSED)
{
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("probing for filesystems is unsupported "
- "by this build"));
return -2;
}
@@ -2868,11 +2865,10 @@ virStorageBackendPARTEDValidLabel(const char *device
ATTRIBUTE_UNUSED,
const char *format ATTRIBUTE_UNUSED,
bool writelabel ATTRIBUTE_UNUSED)
{
- virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("PARTED is unsupported by this build"));
- return -1;
+ return -2;
}
+
#endif /* #if WITH_STORAGE_DISK */
@@ -2898,5 +2894,17 @@ virStorageBackendDeviceIsEmpty(const char *devpath,
writelabel)) == -2)
ret = virStorageBackendPARTEDValidLabel(devpath, format, writelabel);
+ /* Neither BLKID nor PARTED available, but we're not writing,
+ * so no mechanism to check, so allow a lower layer to reject. */
I think you removed too many words here when compared to the commit
message so it stopped making sense.
+ if (ret == -2 && !writelabel)
+ return 0;
+
+ if (ret == -2) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Unable to probe '%s' for existing data, "
+ "requires create/build using overwrite"),
I'd state "forced overwrite is necessary" or something along that points
to the flag even for direct API users.
ACK
6:59 a.m.
New subject: [libvirt] [PATCH 2/3] storage: Clean up return value checking
Rather than special casing the VIR_STORAGE_BLKID_PROBE_UNKNOWN after
calling virStorageBackendBLKIDFindPart, just allow the switch statement
handle setting ret = -2.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 6bdfbf1..eebf039 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2777,10 +2777,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
rc = virStorageBackendBLKIDFindPart(probe, device, format);
- if (rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
- ret = -2;
- goto cleanup;
- }
}
switch (rc) {
@@ -2799,10 +2795,7 @@ virStorageBackendBLKIDFindEmpty(const char *device,
break;
case VIR_STORAGE_BLKID_PROBE_UNKNOWN:
- virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
- _("Not capable of probing for format type '%s',
"
- "requires build --overwrite"),
- format);
+ ret = -2;
break;
case VIR_STORAGE_BLKID_PROBE_MATCH:
@@ -2829,7 +2822,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
ret = -1;
}
- cleanup:
blkid_free_probe(probe);
return ret;
--
2.7.4
9:02 a.m.
New subject: [libvirt] [PATCH 2/3] storage: Clean up return value checking
On Fri, Jan 13, 2017 at 07:59:09 -0500, John Ferlan wrote:
Rather than special casing the VIR_STORAGE_BLKID_PROBE_UNKNOWN after
calling virStorageBackendBLKIDFindPart, just allow the switch statement
handle setting ret = -2.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 6bdfbf1..eebf039 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2777,10 +2777,6 @@ virStorageBackendBLKIDFindEmpty(const char *device,
rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
rc = virStorageBackendBLKIDFindPart(probe, device, format);
- if (rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
- ret = -2;
- goto cleanup;
- }
}
switch (rc) {
@@ -2799,10 +2795,7 @@ virStorageBackendBLKIDFindEmpty(const char *device,
break;
case VIR_STORAGE_BLKID_PROBE_UNKNOWN:
- virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
- _("Not capable of probing for format type '%s',
"
- "requires build --overwrite"),
- format);
+ ret = -2;
ACK, nice to see the virshism to go.
6:59 a.m.
New subject: [libvirt] [PATCH 3/3] storage: Alter error message in probe/empty checks
For case VIR_STORAGE_BLKID_PROBE_DIFFERENT, clean up the message to
avoid using the virsh like --overwrite syntax. Additionally provide
a different error message when not writing the label to avoid confusion.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index eebf039..c6a08eb 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2808,10 +2808,16 @@ virStorageBackendBLKIDFindEmpty(const char *device,
break;
case VIR_STORAGE_BLKID_PROBE_DIFFERENT:
- virReportError(VIR_ERR_STORAGE_POOL_BUILT,
- _("Device '%s' formatted cannot overwrite using
'%s', "
- "requires build --overwrite"),
- device, format);
+ if (writelabel)
+ virReportError(VIR_ERR_STORAGE_POOL_BUILT,
+ _("Device '%s' formatted cannot overwrite using
"
+ "'%s', requires create/build with overwrite
flag"),
+ device, format);
+ else
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Format of device '%s' does not match expected
"
+ "format '%s', requires rebuild"),
+ device, format);
break;
}
--
2.7.4
9:03 a.m.
New subject: [libvirt] [PATCH 3/3] storage: Alter error message in probe/empty checks
On Fri, Jan 13, 2017 at 07:59:10 -0500, John Ferlan wrote:
For case VIR_STORAGE_BLKID_PROBE_DIFFERENT, clean up the message to
avoid using the virsh like --overwrite syntax. Additionally provide
a different error message when not writing the label to avoid confusion.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index eebf039..c6a08eb 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2808,10 +2808,16 @@ virStorageBackendBLKIDFindEmpty(const char *device,
break;
case VIR_STORAGE_BLKID_PROBE_DIFFERENT:
- virReportError(VIR_ERR_STORAGE_POOL_BUILT,
- _("Device '%s' formatted cannot overwrite using
'%s', "
- "requires build --overwrite"),
- device, format);
+ if (writelabel)
+ virReportError(VIR_ERR_STORAGE_POOL_BUILT,
+ _("Device '%s' formatted cannot overwrite using
"
The beginning of the error message does not make sense.
+ "'%s', requires
create/build with overwrite flag"),
+ device, format);
+ else
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Format of device '%s' does not match expected
"
+ "format '%s', requires rebuild"),
I'd drop the "requires rebuild" part.
ACK
2870
days inactive
2870
days old
6 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa