storage: Fix implementation of no-overwrite for file system backend

https://bugzilla.redhat.com/show_bug.cgi?id=1363586

Commit id '27758859' introduced the "NO_OVERWRITE" flag check for
file system backends; however, the implementation, documentation,
and algorithm was inconsistent. For the "flag" description for the
API the flag was described as "Do not overwrite existing pool";
however, within the storage backend code the flag is described
as "it probes to determine if filesystem already exists on the
target device, renurning an error if exists".

The code itself was implemented using the paradigm to set up the
superblock probe by creating a filter that would cause the code
to only search for the provided format type. If that type wasn't
found, then the algorithm would return success allowing the caller
to format the device. If the format type already existed on the
device, then the code would fail indicating that the a filesystem
of the same type existed on the device.

The result is that if someone had a file system of one type on the
device, it was possible to overwrite it if a different format type
was specified in updated XML effectively trashing whatever was on
the device already.

This patch alters what NO_OVERWRITE does for a file system backend
to be more realistic and consistent with what should be expected when
the caller requests to not overwrite the data on the disk.

Rather than filter results based on the expected format type, the
code will allow success/failure be determined solely on whether the
blkid_do_probe calls finds some known format on the device. This
adjustment also allows removal of the virStoragePoolProbeResult
enum that was under utilized.

If it does find a formatted file system different errors will be
generated indicating a file system of a specific type already exists
or a file system of some other type already exists.

In the original virsh support commit id 'ddcd5674', the description
for '--no-overwrite' within the 'pool-build' command help output
has an ambiguous "of this type" included in the short description.
Compared to the longer description within the "Build a given pool."
section of the virsh.pod file it's more apparent that the meaning
of this flag would cause failure if a probe of the target already
has a filesystem.

So this patch also modifies the short description to just be the
antecedent of the 'overwrite' flag, which matches the API description.
This patch also modifies the grammar in virsh.pod for no-overwrite
as well as reworking the paragraph formats to make it easier to read.

Signed-off-by: John Ferlan <jferlan@redhat.com>

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index a112ccd680c04d68e7bdff8b4fb9f59bc1acafcd..ed31efc39934609c080c2133f9750ffc4e591de3 100644 (file)
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2646,87 +2646,80 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
  * Use the blkid_ APIs in order to get details regarding whether a file
  * system exists on the disk already.
  *
- * Returns @virStoragePoolProbeResult value, where any error will also
- * set the error message.
+ * Returns:
+ *   -1: An error was encountered, with error message set
+ *    0: No file system found
  */
-static virStoragePoolProbeResult
+static int
 virStorageBackendBLKIDFindFS(const char *device,
                              const char *format)
 {
 
-    virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR;
+    int ret = -1;
     blkid_probe probe = NULL;
     const char *fstype = NULL;
-    char *names[2], *libblkid_format = NULL;
 
     VIR_DEBUG("Probing for existing filesystem of type %s on device %s",
               format, device);
 
     if (blkid_known_fstype(format) == 0) {
         virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
-                       _("Not capable of probing for "
-                         "filesystem of type %s"),
+                       _("Not capable of probing for filesystem of "
+                         "type %s, requires --overwrite"),
                        format);
-        goto error;
+        return -1;
     }
 
-    probe = blkid_new_probe_from_filename(device);
-    if (probe == NULL) {
+    if (!(probe = blkid_new_probe_from_filename(device))) {
         virReportError(VIR_ERR_STORAGE_PROBE_FAILED,
-                       _("Failed to create filesystem probe "
-                         "for device %s"),
+                       _("Failed to create filesystem probe for device %s"),
                        device);
-        goto error;
+        return -1;
     }
 
-    if (VIR_STRDUP(libblkid_format, format) < 0)
-        goto error;
-
-    names[0] = libblkid_format;
-    names[1] = NULL;
-
-    blkid_probe_filter_superblocks_type(probe,
-                                        BLKID_FLTR_ONLYIN,
-                                        names);
-
     if (blkid_do_probe(probe) != 0) {
         VIR_INFO("No filesystem of type '%s' found on device '%s'",
                  format, device);
-        ret = FILESYSTEM_PROBE_NOT_FOUND;
     } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) {
-        virReportError(VIR_ERR_STORAGE_POOL_BUILT,
-                       _("Existing filesystem of type '%s' found on "
-                         "device '%s'"),
-                       fstype, device);
-        ret = FILESYSTEM_PROBE_FOUND;
+        if (STREQ(fstype, format)) {
+            virReportError(VIR_ERR_STORAGE_POOL_BUILT,
+                           _("Device '%s' already contains a filesystem "
+                             "of type '%s'"),
+                           device, fstype);
+        } else {
+            virReportError(VIR_ERR_STORAGE_POOL_BUILT,
+                           _("Existing filesystem of type '%s' found on "
+                             "device '%s', requires --overwrite"),
+                           fstype, device);
+        }
+        goto cleanup;
     }
 
     if (blkid_do_probe(probe) != 1) {
         virReportError(VIR_ERR_STORAGE_PROBE_FAILED, "%s",
                        _("Found additional probes to run, "
                          "filesystem probing may be incorrect"));
-        ret = FILESYSTEM_PROBE_ERROR;
+        goto cleanup;
     }
 
- error:
-    VIR_FREE(libblkid_format);
+    ret = 0;
 
-    if (probe != NULL)
-        blkid_free_probe(probe);
+ cleanup:
+    blkid_free_probe(probe);
 
     return ret;
 }
 
 #else /* #if WITH_BLKID */
 
-static virStoragePoolProbeResult
+static int
 virStorageBackendBLKIDFindFS(const char *device ATTRIBUTE_UNUSED,
                              const char *format ATTRIBUTE_UNUSED)
 {
     virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                    _("probing for filesystems is unsupported "
                      "by this build"));
-    return FILESYSTEM_PROBE_ERROR;
+    return -1;
 }
 
 #endif /* #if WITH_BLKID */
@@ -2746,6 +2739,5 @@ bool
 virStorageBackendDeviceIsEmpty(const char *devpath,
                                const char *format)
 {
-    return virStorageBackendBLKIDFindFS(devpath, format) ==
-        FILESYSTEM_PROBE_NOT_FOUND;
+    return virStorageBackendBLKIDFindFS(devpath, format) == 0;
 }

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index a85e39c54734b1b27b395101606cc91ea89e6805..ba4f598975960fa0ff7617109a0c64a7306a4734 100644 (file)
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -712,12 +712,15 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
  *
  * Build a directory or FS based storage pool.
  *
- * If no flag is set, it only makes the directory; If
- * VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it probes to determine if
- * filesystem already exists on the target device, renurning an error
- * if exists, or using mkfs to format the target device if not; If
- * VIR_STORAGE_POOL_BUILD_OVERWRITE is set, mkfs is always executed,
- * any existed data on the target device is overwritten unconditionally.
+ * If no flag is set, it only makes the directory.
+ *
+ * If VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it probes to determine if
+ * any filesystem already exists on the target device, returning an error
+ * if one exists. If no filesystem already exists, use mkfs to format the
+ * target device.
+ *
+ * If VIR_STORAGE_POOL_BUILD_OVERWRITE is set, mkfs is always executed and
+ * any existing data on the target device is overwritten unconditionally.
  *
  * The underlying source device is mounted for FS based pools.
  *

diff --git a/src/storage/storage_backend_fs.h b/src/storage/storage_backend_fs.h
index 347ea9be5c7f33a1cfe116e126cd6b1a3daaab71..94fe11138c0b3549dc294729bbf9336dceaa99f9 100644 (file)
--- a/src/storage/storage_backend_fs.h
+++ b/src/storage/storage_backend_fs.h
@@ -31,11 +31,6 @@ extern virStorageBackend virStorageBackendFileSystem;
 extern virStorageBackend virStorageBackendNetFileSystem;
 # endif
 
-typedef enum {
-    FILESYSTEM_PROBE_FOUND,
-    FILESYSTEM_PROBE_NOT_FOUND,
-    FILESYSTEM_PROBE_ERROR,
-} virStoragePoolProbeResult;
 extern virStorageBackend virStorageBackendDirectory;
 
 extern virStorageFileBackend virStorageFileBackendFile;

diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 8313be8684421e48d91ca3b48abd7d95c137092d..45b538efacf997f7f9a5c904e9198312b2fe0fa9 100644 (file)
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -48,7 +48,7 @@
     {.name = "no-overwrite",                                      \
      .type = VSH_OT_BOOL,                                         \
      .flags = 0,                                                  \
-     .help = N_("do not overwrite an existing pool of this type") \
+     .help = N_("do not overwrite any existing data")             \
     }                                                             \
 
 #define VIRSH_COMMON_OPT_POOL_OVERWRITE                           \

diff --git a/tools/virsh.pod b/tools/virsh.pod
index c29f16dca74a16af2d34fcd95b50fd68f6f7192d..6af319e6be95d7eb8dc6c60ab8a3063a702c6ff8 100644 (file)
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3466,20 +3466,24 @@ Configure whether I<pool> should automatically start at boot.
 Build a given pool.
 
 Options I<--overwrite> and I<--no-overwrite> can only be used for
-B<pool-build> a filesystem or disk pool. For a file system pool if
-neither of them is specified, B<pool-build> makes the directory. If
-I<--no-overwrite> is specified, it probes to determine if a
-filesystem already exists on the target device, returning an error
-if exists, or using mkfs to format the target device if not. 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.
+B<pool-build> a filesystem or disk pool.
+
+For a file system pool if neither flag is specified, then B<pool-build>
+just makes the target path directory and no attempt to run mkfs on the
+target volume device. If I<--no-overwrite> is specified, it probes to
+determine if a filesystem already exists on the target device, returning
+an error if one exists or using mkfs to format the target device if not.
+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.
 
 =item B<pool-create> I<file>
 [I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]]