storage: Move and rename disk backend label checking

Rather than have the Disk code having to use PARTED to determine if
there's something on the device, let's use the virStorageBackendDeviceProbe.
and only fallback to the PARTED probing if the BLKID code isn't built in.

This will also provide a mechanism for the other current caller (File
System Backend) to utilize a PARTED parsing algorithm in the event that
BLKID isn't built in to at least see if *something* exists on the disk
before blindly trying to use. The PARTED error checking will not find
file system types, but if there is a partition table set on the device,
it will at least cause a failure.

Move virStorageBackendDiskValidLabel and virStorageBackendDiskFindLabel
to storage_backend and rename/rework the code to fit the new model.

Update the virsh.pod description to provide a more generic description
of the process since we could now use either blkid or parted to find
data on the target device.
Signed-off-by: NJohn Ferlan <jferlan@redhat.com>

src/storage/storage_backend.c

@@ -2704,6 +2704,14 @@ virStorageBackendBLKIDFindPart(blkid_probe probe,
 {
     const char *pttype = NULL;
 
+    /* A blkid_known_pttype on "dvh" and "pc98" returns a failure;
+     * however, the blkid_do_probe for "dvh" returns "sgi" and
+     * for "pc98" it returns "dos". So since those will cause problems
+     * with startup comparison, let's just treat them as UNKNOWN causing
+     * the caller to fallback to using PARTED */
+    if (STREQ(format, "dvh") || STREQ(format, "pc98"))
+        return VIR_STORAGE_BLKID_PROBE_UNKNOWN;
+
     /* Make sure we're doing a partitions probe from the start */
     blkid_probe_enable_partitions(probe, true);
     blkid_probe_reset_partitions_filter(probe);
@@ -2736,6 +2744,7 @@ virStorageBackendBLKIDFindPart(blkid_probe probe,
  * system or partition exists on the disk already.
  *
  * Returns:
+ *   -2: Force usage of PARTED for unknown types
  *   -1: An error was encountered, with error message set
  *    0: No file system found
  */
@@ -2765,8 +2774,14 @@ virStorageBackendBLKIDFindEmpty(const char *device,
      * partition probing. */
     rc = virStorageBackendBLKIDFindFS(probe, device, format);
     if (rc == VIR_STORAGE_BLKID_PROBE_UNDEFINED ||
-        rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN)
+        rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
+
         rc = virStorageBackendBLKIDFindPart(probe, device, format);
+        if (rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) {
+            ret = -2;
+            goto cleanup;
+        }
+    }
 
     switch (rc) {
     case VIR_STORAGE_BLKID_PROBE_UNDEFINED:
@@ -2814,6 +2829,7 @@ virStorageBackendBLKIDFindEmpty(const char *device,
         ret = -1;
     }
 
+ cleanup:
     blkid_free_probe(probe);
 
     return ret;
@@ -2829,12 +2845,151 @@ virStorageBackendBLKIDFindEmpty(const char *device ATTRIBUTE_UNUSED,
     virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                    _("probing for filesystems is unsupported "
                      "by this build"));
-    return -1;
+    return -2;
 }
 
 #endif /* #if WITH_BLKID */
 
 
+typedef enum {
+    VIR_STORAGE_PARTED_ERROR = -1,
+    VIR_STORAGE_PARTED_FOUND,       /* Valid label found */
+    VIR_STORAGE_PARTED_UNKNOWN,     /* No or unrecognized label */
+    VIR_STORAGE_PARTED_NOPTTYPE,    /* Did not find the Partition Table type */
+    VIR_STORAGE_PARTED_PTTYPE_UNK,  /* Partition Table type unknown*/
+} virStorageBackendPARTEDResult;
+
+/**
+ * Check for a valid disk label (partition table) on device using
+ * the PARTED command
+ *
+ * returns virStorageBackendPARTEDResult
+ */
+static virStorageBackendPARTEDResult
+virStorageBackendPARTEDFindLabel(const char *device)
+{
+    const char *const args[] = {
+        device, "print", "--script", NULL,
+    };
+    virCommandPtr cmd = virCommandNew(PARTED);
+    char *output = NULL;
+    char *error = NULL;
+    char *start, *end;
+    int ret = VIR_STORAGE_PARTED_ERROR;
+
+    virCommandAddArgSet(cmd, args);
+    virCommandAddEnvString(cmd, "LC_ALL=C");
+    virCommandSetOutputBuffer(cmd, &output);
+    virCommandSetErrorBuffer(cmd, &error);
+
+    /* if parted succeeds we have a valid partition table */
+    ret = virCommandRun(cmd, NULL);
+    if (ret < 0) {
+        if ((output && strstr(output, "unrecognised disk label")) ||
+            (error && strstr(error, "unrecognised disk label"))) {
+            ret = VIR_STORAGE_PARTED_UNKNOWN;
+        }
+        goto cleanup;
+    }
+
+    /* Search for "Partition Table:" in the output. If not present,
+     * then we cannot validate the partition table type.
+     */
+    if (!(start = strstr(output, "Partition Table: ")) ||
+        !(end = strstr(start, "\n"))) {
+        VIR_DEBUG("Unable to find tag in output: %s", output);
+        ret = VIR_STORAGE_PARTED_NOPTTYPE;
+        goto cleanup;
+    }
+    start += strlen("Partition Table: ");
+    *end = '\0';
+
+    /* on disk it's "msdos", but we document/use "dos" so deal with it here */
+    if (STREQ(start, "msdos"))
+        start += 2;
+
+    /* Make sure we know about this type */
+    if (virStoragePoolFormatDiskTypeFromString(start) < 0) {
+        ret = VIR_STORAGE_PARTED_PTTYPE_UNK;
+        goto cleanup;
+    }
+
+    ret = VIR_STORAGE_PARTED_FOUND;
+
+ cleanup:
+    virCommandFree(cmd);
+    VIR_FREE(output);
+    VIR_FREE(error);
+    return ret;
+}
+
+
+/**
+ * Determine whether the label on the disk is valid or in a known format
+ * for the purpose of rewriting the label during build or being able to
+ * start a pool on a device.
+ *
+ * When 'writelabel' is true, if we find a valid disk label on the device,
+ * then we shouldn't be attempting to write as the volume may contain
+ * data. Force the usage of the overwrite flag to the build command in
+ * order to be certain. When the disk label is unrecognized, then it
+ * should be safe to write.
+ *
+ * When 'writelabel' is false, only if we find a valid disk label on the
+ * device should we allow the start since for this path we won't be
+ * rewriting the label.
+ *
+ * Return: 0 if it's OK
+ *         -1 if something's wrong
+ */
+static int
+virStorageBackendPARTEDValidLabel(const char *device,
+                                  bool writelabel)
+{
+    int ret = -1;
+    virStorageBackendPARTEDResult check;
+
+    check = virStorageBackendPARTEDFindLabel(device);
+    switch (check) {
+    case VIR_STORAGE_PARTED_ERROR:
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("Error checking for disk label, failed to get "
+                         "disk partition information"));
+        break;
+
+    case VIR_STORAGE_PARTED_FOUND:
+        if (writelabel)
+            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                           _("Valid disk label already present, "
+                             "requires --overwrite"));
+        else
+            ret = 0;
+        break;
+
+    case VIR_STORAGE_PARTED_UNKNOWN:
+        if (writelabel)
+            ret = 0;
+        else
+            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                           _("Unrecognized disk label found, requires build"));
+        break;
+
+    case VIR_STORAGE_PARTED_NOPTTYPE:
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("Unable to determine Partition Type, "
+                         "requires build --overwrite"));
+        break;
+
+    case VIR_STORAGE_PARTED_PTTYPE_UNK:
+        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                       _("Unknown Partition Type, requires build --overwrite"));
+        break;
+    }
+
+    return ret;
+}
+
+
 /* virStorageBackendDeviceIsEmpty:
  * @devpath: Path to the device to check
  * @format: Desired format string
@@ -2851,5 +3006,11 @@ virStorageBackendDeviceIsEmpty(const char *devpath,
                                const char *format,
                                bool writelabel)
 {
-    return virStorageBackendBLKIDFindEmpty(devpath, format, writelabel) == 0;
+    int ret;
+
+    if ((ret = virStorageBackendBLKIDFindEmpty(devpath, format,
+                                               writelabel)) == -2)
+        ret = virStorageBackendPARTEDValidLabel(devpath, writelabel);
+
+    return ret == 0;
 }

src/storage/storage_backend_disk.c

@@ -418,143 +418,23 @@ virStorageBackendDiskRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
 }
 
 
-/**
- * Check for a valid disk label (partition table) on device
- *
- * return: 0 - valid disk label found
- *         1 - no or unrecognized disk label
- *         2 - did not find the Partition Table type
- *         3 - Partition Table type unknown
- *        <0 - error finding the disk label
- */
-static int
-virStorageBackendDiskFindLabel(const char* device)
-{
-    const char *const args[] = {
-        device, "print", "--script", NULL,
-    };
-    virCommandPtr cmd = virCommandNew(PARTED);
-    char *output = NULL;
-    char *error = NULL;
-    char *start, *end;
-    int ret = -1;
-
-    virCommandAddArgSet(cmd, args);
-    virCommandAddEnvString(cmd, "LC_ALL=C");
-    virCommandSetOutputBuffer(cmd, &output);
-    virCommandSetErrorBuffer(cmd, &error);
-
-    /* if parted succeeds we have a valid partition table */
-    ret = virCommandRun(cmd, NULL);
-    if (ret < 0) {
-        if ((output && strstr(output, "unrecognised disk label")) ||
-            (error && strstr(error, "unrecognised disk label"))) {
-            ret = 1;
-        }
-        goto cleanup;
-    }
-
-    /* Search for "Partition Table:" in the output. If not present,
-     * then we cannot validate the partition table type.
-     */
-    if (!(start = strstr(output, "Partition Table: ")) ||
-        !(end = strstr(start, "\n"))) {
-        VIR_DEBUG("Unable to find tag in output: %s", output);
-        ret = 2;
-        goto cleanup;
-    }
-    start += strlen("Partition Table: ");
-    *end = '\0';
-
-    /* on disk it's "msdos", but we document/use "dos" so deal with it here */
-    if (STREQ(start, "msdos"))
-        start += 2;
-
-    /* Make sure we know about this type */
-    if (virStoragePoolFormatDiskTypeFromString(start) < 0) {
-        ret = 3;
-        goto cleanup;
-    }
-
-    ret = 0;
-
- cleanup:
-    virCommandFree(cmd);
-    VIR_FREE(output);
-    VIR_FREE(error);
-    return ret;
-}
-
-/**
- * Determine whether the label on the disk is valid or in a known format
- * for the purpose of rewriting the label during build or being able to
- * start a pool on a device.
- *
- * When 'writelabel' is true, if we find a valid disk label on the device,
- * then we shouldn't be attempting to write as the volume may contain
- * data. Force the usage of the overwrite flag to the build command in
- * order to be certain. When the disk label is unrecognized, then it
- * should be safe to write.
- *
- * When 'writelabel' is false, only if we find a valid disk label on the
- * device should we allow the start since for this path we won't be
- * rewriting the label.
- *
- * Return: True if it's OK
- *         False if something's wrong
- */
-static bool
-virStorageBackendDiskValidLabel(const char *device,
-                                bool writelabel)
-{
-    bool valid = false;
-    int check;
-
-    check = virStorageBackendDiskFindLabel(device);
-    if (check == 1) {
-        if (writelabel)
-            valid = true;
-        else
-            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                           _("Unrecognized disk label found, requires build"));
-    } else if (check == 2) {
-        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                       _("Unable to determine Partition Type, "
-                         "requires build --overwrite"));
-    } else if (check == 3) {
-        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                       _("Unknown Partition Type, requires build --overwrite"));
-    } else if (check < 0) {
-        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-                       _("Error checking for disk label, failed to get "
-                         "disk partition information"));
-    } else {
-        if (writelabel)
-            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-                           _("Valid disk label already present, "
-                             "requires --overwrite"));
-        else
-            valid = true;
-    }
-    return valid;
-}
-
-
 static int
 virStorageBackendDiskStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virStoragePoolObjPtr pool)
 {
+    const char *format =
+        virStoragePoolFormatDiskTypeToString(pool->def->source.format);
+    const char *path = pool->def->source.devices[0].path;
+
     virFileWaitForDevices();
 
-    if (!virFileExists(pool->def->source.devices[0].path)) {
+    if (!virFileExists(path)) {
         virReportError(VIR_ERR_INVALID_ARG,
-                       _("device path '%s' doesn't exist"),
-                       pool->def->source.devices[0].path);
+                       _("device path '%s' doesn't exist"), path);
         return -1;
     }
 
-    if (!virStorageBackendDiskValidLabel(pool->def->source.devices[0].path,
-                                         false))
+    if (!virStorageBackendDeviceIsEmpty(path, format, false))
         return -1;
 
     return 0;
@@ -569,6 +449,8 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virStoragePoolObjPtr pool,
                                unsigned int flags)
 {
+    int format = pool->def->source.format;
+    const char *fmt;
     bool ok_to_mklabel = false;
     int ret = -1;
     virCommandPtr cmd = NULL;
@@ -580,17 +462,17 @@ virStorageBackendDiskBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                              VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
                              error);
 
-    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE)
+    fmt = virStoragePoolFormatDiskTypeToString(format);
+    if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) {
         ok_to_mklabel = true;
-    else
-        ok_to_mklabel = virStorageBackendDiskValidLabel(
-                                            pool->def->source.devices[0].path,
-                                            true);
+    } else {
+        if (virStorageBackendDeviceIsEmpty(pool->def->source.devices[0].path,
+                                              fmt, true))
+            ok_to_mklabel = true;
+    }
 
     if (ok_to_mklabel) {
         /* eg parted /dev/sda mklabel --script msdos */
-        int format = pool->def->source.format;
-        const char *fmt;
         if (format == VIR_STORAGE_POOL_DISK_UNKNOWN)
             format = pool->def->source.format = VIR_STORAGE_POOL_DISK_DOS;
         if (format == VIR_STORAGE_POOL_DISK_DOS)

tools/virsh.pod

@@ -3477,13 +3477,13 @@ If I<--overwrite> is specified, mkfs is always executed and any existing
 data on the target device is overwritten unconditionally.
 
 For a disk pool, if neither of them is specified or I<--no-overwrite>
-is specified, B<pool-build> will use 'parted --print' in order to
-determine if the disk already has a label before attempting to create
-one. Only if a disk does not already have one will a label be created.
-If I<--overwrite> is specified or it's been determined that the disk
-doesn't already have one, 'parted mklabel' will be used to create a
-label of the format specified by the pool source format type or "dos"
-if not specified for the pool.
+is specified, B<pool-build> will check the target volume device for
+existing filesystems or partitions before attempting to write a new
+label on the target volume device. If the target volume device already
+has a label, the command will fail. If I<--overwrite> is specified,
+then no check will be made on the target volume device prior to writing
+a new label. Writing of the label uses the pool source format type
+or "dos" if not specified.
 
 =item B<pool-create> I<file>
 [I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]]