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@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

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@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.

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@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