[libvirt] [PATCH 00/11] Adjust build/start format checking for fs, disk, and logical backends
Two of these patches (2 and 10) are followups to no-overwrite patches previously posted (I left links in the patches for reference). Essentially this series works to unify the probing code from the file system backend using blkid and the parted format reading code from the disk backend in one common set of functions. In doing so, the disk backend will now pick up the ability to use the blkid 'partitions' APIs in order to "probe" the target device for a valid/known partition format type. Additionally, the file system probe will pick up a fallback to the parted mechanism from the disk backend if the 'blkid' APIs are not built into the system. A few extra bells/whistles were added for fs pool startup to ensure that the format in the startup XML matches the format on the target disk, which is essentially the mechanism the disk backend used (although it was slightly broken). Then in order to really pile on, the overwrite logic was added to the logical pool which didn't have any such checking (both build and start). Testing was done locally using an iSCSI device and laying down different formats and seeing what would happen on various usages of the format (with --overwrite and --no-overwrite). John Ferlan (11): storage: Introduce virStorageBackendDeviceProbeEmpty storage: Fix implementation of no-overwrite for file system backend storage: Add partition type checks for BLKID probing storage: Add writelabel bool for virStorageBackendDeviceProbe storage: For FS pool check for properly formatted target volume storage: Move and rename disk backend label checking storage: Adjust disk label found to match labels storage: Clean up logical pool devices on build failure storage: Extract logical device initialize into a helper storage: Add overwrite flag checking for logical pool storage: Validate the device formats at logical startup src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 396 ++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 4 + src/storage/storage_backend_disk.c | 150 ++----------- src/storage/storage_backend_fs.c | 118 ++-------- src/storage/storage_backend_fs.h | 5 - src/storage/storage_backend_logical.c | 180 ++++++++++------ tools/virsh-pool.c | 2 +- tools/virsh.pod | 42 ++-- 9 files changed, 585 insertions(+), 313 deletions(-) -- 2.7.4
Rename virStorageBackendFileSystemProbe and to virStorageBackendBLKIDProbe and move to the more common storage_backend module. Create a shim virStorageBackendDeviceProbeEmpty which will make the call to the virStorageBackendBLKIDProbeFS and check the return value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 120 +++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 90 +---------------------------- 3 files changed, 124 insertions(+), 89 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f2fc038..2432b54 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -42,6 +42,10 @@ # endif #endif +#if WITH_BLKID +# include <blkid/blkid.h> +#endif + #if WITH_SELINUX # include <selinux/selinux.h> #endif @@ -2632,3 +2636,119 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED, return 0; } #endif /* #ifdef GLUSTER_CLI */ + + +#if WITH_BLKID +/* + * @device: Path to device + * @format: Desired format + * + * 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. + */ +static virStoragePoolProbeResult +virStorageBackendBLKIDProbeFS(const char *device, + const char *format) +{ + + virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR; + 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"), + format); + goto error; + } + + probe = blkid_new_probe_from_filename(device); + if (probe == NULL) { + virReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Failed to create filesystem probe " + "for device %s"), + device); + goto error; + } + + 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 (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; + } + + error: + VIR_FREE(libblkid_format); + + if (probe != NULL) + blkid_free_probe(probe); + + return ret; +} + +#else /* #if WITH_BLKID */ + +static virStoragePoolProbeResult +virStorageBackendBLKIDProbeFS(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; +} + +#endif /* #if WITH_BLKID */ + + +/* virStorageBackendDeviceProbeEmpty: + * @devpath: Path to the device to check + * @format: Desired format string + * + * Check if the @devpath has some sort of known file system using the + * BLKID API if available. + * + * Returns true if the probe deems the device has nothing valid on it + * and returns false if the probe finds something + */ +bool +virStorageBackendDeviceProbeEmpty(const char *devpath, + const char *format) +{ + if (virStorageBackendBLKIDProbeFS(devpath, format) == + FILESYSTEM_PROBE_NOT_FOUND) + true; + + return false; +} diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 28e1a65..75f2ed6 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -158,6 +158,9 @@ int virStorageBackendVolWipeLocal(virConnectPtr conn, unsigned int algorithm, unsigned int flags); +bool virStorageBackendDeviceProbeEmpty(const char *devpath, + const char *format); + typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d5..c29791b 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -37,10 +37,6 @@ #include <libxml/tree.h> #include <libxml/xpath.h> -#if WITH_BLKID -# include <blkid/blkid.h> -#endif - #include "virerror.h" #include "storage_backend_fs.h" #include "storage_conf.h" @@ -617,89 +613,6 @@ virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, } #endif /* WITH_STORAGE_FS */ -#if WITH_BLKID -static virStoragePoolProbeResult -virStorageBackendFileSystemProbe(const char *device, - const char *format) -{ - - virStoragePoolProbeResult ret = FILESYSTEM_PROBE_ERROR; - 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"), - format); - goto error; - } - - probe = blkid_new_probe_from_filename(device); - if (probe == NULL) { - virReportError(VIR_ERR_STORAGE_PROBE_FAILED, - _("Failed to create filesystem probe " - "for device %s"), - device); - goto error; - } - - 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 (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; - } - - error: - VIR_FREE(libblkid_format); - - if (probe != NULL) - blkid_free_probe(probe); - - return ret; -} - -#else /* #if WITH_BLKID */ - -static virStoragePoolProbeResult -virStorageBackendFileSystemProbe(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; -} - -#endif /* #if WITH_BLKID */ /* some platforms don't support mkfs */ #ifdef MKFS @@ -780,8 +693,7 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { ok_to_mkfs = true; } else if (flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE && - virStorageBackendFileSystemProbe(device, format) == - FILESYSTEM_PROBE_NOT_FOUND) { + virStorageBackendDeviceProbeEmpty(device, format)) { ok_to_mkfs = true; } -- 2.7.4
On 12/15/2016 10:42 PM, John Ferlan wrote:
Rename virStorageBackendFileSystemProbe and to virStorageBackendBLKIDProbe and move to the more common storage_backend module.
Create a shim virStorageBackendDeviceProbeEmpty which will make the call to the virStorageBackendBLKIDProbeFS and check the return value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 120 +++++++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 3 + src/storage/storage_backend_fs.c | 90 +---------------------------- 3 files changed, 124 insertions(+), 89 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f2fc038..2432b54 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c
+/* virStorageBackendDeviceProbeEmpty: + * @devpath: Path to the device to check + * @format: Desired format string + * + * Check if the @devpath has some sort of known file system using the + * BLKID API if available. + * + * Returns true if the probe deems the device has nothing valid on it + * and returns false if the probe finds something + */ +bool +virStorageBackendDeviceProbeEmpty(const char *devpath, + const char *format) +{ + if (virStorageBackendBLKIDProbeFS(devpath, format) == + FILESYSTEM_PROBE_NOT_FOUND) + true;
return true; /* is what you want here. */ Or even better: return virStorageBackendBLKIDProbeFS() == FILESYSTEM_PROBE_NOT_FOUND;
+ + return false; +}
ACK with that change. Michal
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> --- v1: http://www.redhat.com/archives/libvir-list/2016-November/msg00784.html Changes aplenty since v1 - the subsequent patches are a result of the review comments from jtomko vis-a-vis using blkid for the partitions src/storage/storage_backend.c | 68 ++++++++++++++++++---------------------- src/storage/storage_backend_fs.c | 15 +++++---- src/storage/storage_backend_fs.h | 5 --- tools/virsh-pool.c | 2 +- tools/virsh.pod | 32 ++++++++++--------- 5 files changed, 58 insertions(+), 64 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2432b54..108f2b9 100644 --- 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 virStorageBackendBLKIDProbeFS(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 virStorageBackendBLKIDProbeFS(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,8 +2739,7 @@ bool virStorageBackendDeviceProbeEmpty(const char *devpath, const char *format) { - if (virStorageBackendBLKIDProbeFS(devpath, format) == - FILESYSTEM_PROBE_NOT_FOUND) + if (virStorageBackendBLKIDProbeFS(devpath, format) == 0) true; return false; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c29791b..c3986af 100644 --- 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 347ea9b..94fe111 100644 --- 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 8313be8..45b538e 100644 --- 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 74c05c9..b00fc8d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3452,20 +3452,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>]] -- 2.7.4
On 12/15/2016 10:42 PM, John Ferlan wrote:
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> ---
v1: http://www.redhat.com/archives/libvir-list/2016-November/msg00784.html
Changes aplenty since v1 - the subsequent patches are a result of the review comments from jtomko vis-a-vis using blkid for the partitions
src/storage/storage_backend.c | 68 ++++++++++++++++++---------------------- src/storage/storage_backend_fs.c | 15 +++++---- src/storage/storage_backend_fs.h | 5 --- tools/virsh-pool.c | 2 +- tools/virsh.pod | 32 ++++++++++--------- 5 files changed, 58 insertions(+), 64 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2432b54..108f2b9 100644 --- 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
Let me just say I find this very confusing. Why would ProbeFS() function make decisions whether it is possible to rewrite FS or not? I mean, I'd expect ProbeFS() function to fetch me the FS on @device so that I, caller, can make the decision myself. Was this something that was requested by Jan? Maybe it's a naming problem. MatchFS() sounds more like reflecting what this function actually does after this change.
*/ -static virStoragePoolProbeResult +static int virStorageBackendBLKIDProbeFS(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; }
Michal
On 01/09/2017 10:32 AM, Michal Privoznik wrote:
On 12/15/2016 10:42 PM, John Ferlan wrote:
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> ---
v1: http://www.redhat.com/archives/libvir-list/2016-November/msg00784.html
Changes aplenty since v1 - the subsequent patches are a result of the review comments from jtomko vis-a-vis using blkid for the partitions
src/storage/storage_backend.c | 68 ++++++++++++++++++---------------------- src/storage/storage_backend_fs.c | 15 +++++---- src/storage/storage_backend_fs.h | 5 --- tools/virsh-pool.c | 2 +- tools/virsh.pod | 32 ++++++++++--------- 5 files changed, 58 insertions(+), 64 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2432b54..108f2b9 100644 --- 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
Let me just say I find this very confusing. Why would ProbeFS() function make decisions whether it is possible to rewrite FS or not? I mean, I'd expect ProbeFS() function to fetch me the FS on @device so that I, caller, can make the decision myself.
Was this something that was requested by Jan?
History? I just shortened the "FileSystem" to "FS" and inserted that BLKID to indicate the tool being used to do the probe. AFAIK, BLKID and PARTED are the existing options - all I'm looking to do is combine them which was something Jan hinted at IIRC.
Maybe it's a naming problem. MatchFS() sounds more like reflecting what this function actually does after this change.
Changing a name is easy, but after reading your comment in 3, I think inserting a step between 1 and 2 would at least remove the concern regarding changing the ProbeEmpty function. If changing the name suffices - that's fine too. If you'd rather see other merges - I'm fine with that. John
*/ -static virStoragePoolProbeResult +static int virStorageBackendBLKIDProbeFS(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; }
Michal
On 01/10/2017 01:12 AM, John Ferlan wrote:
On 01/09/2017 10:32 AM, Michal Privoznik wrote:
On 12/15/2016 10:42 PM, John Ferlan wrote:
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> ---
v1: http://www.redhat.com/archives/libvir-list/2016-November/msg00784.html
Changes aplenty since v1 - the subsequent patches are a result of the review comments from jtomko vis-a-vis using blkid for the partitions
src/storage/storage_backend.c | 68 ++++++++++++++++++---------------------- src/storage/storage_backend_fs.c | 15 +++++---- src/storage/storage_backend_fs.h | 5 --- tools/virsh-pool.c | 2 +- tools/virsh.pod | 32 ++++++++++--------- 5 files changed, 58 insertions(+), 64 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 2432b54..108f2b9 100644 --- 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
Let me just say I find this very confusing. Why would ProbeFS() function make decisions whether it is possible to rewrite FS or not? I mean, I'd expect ProbeFS() function to fetch me the FS on @device so that I, caller, can make the decision myself.
Was this something that was requested by Jan?
History? I just shortened the "FileSystem" to "FS" and inserted that BLKID to indicate the tool being used to do the probe. AFAIK, BLKID and PARTED are the existing options - all I'm looking to do is combine them which was something Jan hinted at IIRC.
Maybe it's a naming problem. MatchFS() sounds more like reflecting what this function actually does after this change.
Changing a name is easy, but after reading your comment in 3, I think inserting a step between 1 and 2 would at least remove the concern regarding changing the ProbeEmpty function.
If changing the name suffices - that's fine too. If you'd rather see other merges - I'm fine with that.
Yeah, I think if this function was called MatchFS(), CheckFS() or something among those lines I'm okay with this patch. But ProbeFS() makes me think that the function is just probing for existing FSs on given device. Sorry for the bikeshedding :-). ACK if you make the function name reflect what the function actually does. Michal
[...]
Yeah, I think if this function was called MatchFS(), CheckFS() or something among those lines I'm okay with this patch. But ProbeFS() makes me think that the function is just probing for existing FSs on given device. Sorry for the bikeshedding :-).
No problem - I'd rather get the name right now instead of having a followup because someone didn't like the name. How about FindFS() - the function is looking to "find" if a FS exists on the disk/partition. That would mean ProbeEmpty could be FindNoFS or FindEmpty (and the fallout in the remaining patches to adjust the names). John
ACK if you make the function name reflect what the function actually does.
Michal
On 01/10/2017 01:36 PM, John Ferlan wrote:
[...]
Yeah, I think if this function was called MatchFS(), CheckFS() or something among those lines I'm okay with this patch. But ProbeFS() makes me think that the function is just probing for existing FSs on given device. Sorry for the bikeshedding :-).
No problem - I'd rather get the name right now instead of having a followup because someone didn't like the name.
How about FindFS() - the function is looking to "find" if a FS exists on the disk/partition.
That would mean ProbeEmpty could be FindNoFS or FindEmpty (and the fallout in the remaining patches to adjust the names).
Looks reasonable to me. Michal
A device may be formatted using some sort of disk partition format type. We can check that using the blkid_ API's as well - so alter the logic to allow checking the device for both a filesystem and a disk partition. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 177 ++++++++++++++++++++++++++++++++---------- 1 file changed, 138 insertions(+), 39 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 108f2b9..065f2ef 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2639,37 +2639,117 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED, #if WITH_BLKID + +typedef enum { + VIR_STORAGE_BLKID_PROBE_ERROR = -1, + VIR_STORAGE_BLKID_PROBE_UNDEFINED, /* Nothing found */ + VIR_STORAGE_BLKID_PROBE_UNKNOWN, /* Don't know libvirt fs/part type */ + VIR_STORAGE_BLKID_PROBE_MATCH, /* Matches the on disk format */ + VIR_STORAGE_BLKID_PROBE_DIFFERENT, /* Format doesn't match on disk format */ +} virStorageBackendBLKIDProbeResult; + +/* + * Utility function to probe for a file system on the device using the + * blkid "superblock" (e.g. default) APIs. + * + * NB: In general this helper will handle the virStoragePoolFormatFileSystem + * format types; however, if called from the Disk path, the initial fstype + * check will fail forcing the usage of the ProbePart helper. + * + * Returns virStorageBackendBLKIDProbeResult enum + */ +static virStorageBackendBLKIDProbeResult +virStorageBackendBLKIDProbeFS(blkid_probe probe, + const char *device, + const char *format) +{ + const char *fstype = NULL; + + /* Make sure we're doing a superblock probe from the start */ + blkid_probe_enable_superblocks(probe, true); + blkid_probe_reset_superblocks_filter(probe); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No filesystem found on device '%s'", device); + return VIR_STORAGE_BLKID_PROBE_UNDEFINED; + } + + if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { + if (STREQ(fstype, format)) + return VIR_STORAGE_BLKID_PROBE_MATCH; + + return VIR_STORAGE_BLKID_PROBE_DIFFERENT; + } + + if (blkid_known_fstype(format) == 0) + return VIR_STORAGE_BLKID_PROBE_UNKNOWN; + + return VIR_STORAGE_BLKID_PROBE_ERROR; +} + + +/* + * Utility function to probe for a partition on the device using the + * blkid "partitions" APIs. + * + * NB: In general, this API will be validating the virStoragePoolFormatDisk + * format types. + * + * Returns virStorageBackendBLKIDProbeResult enum + */ +static virStorageBackendBLKIDProbeResult +virStorageBackendBLKIDProbePart(blkid_probe probe, + const char *device, + const char *format) +{ + const char *pttype = NULL; + + /* Make sure we're doing a partitions probe from the start */ + blkid_probe_enable_partitions(probe, true); + blkid_probe_reset_partitions_filter(probe); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No partition found on device '%s'", device); + return VIR_STORAGE_BLKID_PROBE_UNDEFINED; + } + + if (blkid_probe_lookup_value(probe, "PTTYPE", &pttype, NULL) == 0) { + if (STREQ(pttype, format)) + return VIR_STORAGE_BLKID_PROBE_MATCH; + + return VIR_STORAGE_BLKID_PROBE_DIFFERENT; + } + + if (blkid_known_pttype(format) == 0) + return VIR_STORAGE_BLKID_PROBE_UNKNOWN; + + return VIR_STORAGE_BLKID_PROBE_ERROR; +} + + /* * @device: Path to device * @format: Desired format * * Use the blkid_ APIs in order to get details regarding whether a file - * system exists on the disk already. + * system or partition exists on the disk already. * * Returns: * -1: An error was encountered, with error message set * 0: No file system found */ static int -virStorageBackendBLKIDProbeFS(const char *device, - const char *format) +virStorageBackendBLKIDProbe(const char *device, + const char *format) { int ret = -1; + int rc; blkid_probe probe = NULL; - const char *fstype = NULL; - VIR_DEBUG("Probing for existing filesystem of type %s on device %s", + VIR_DEBUG("Probe for existing filesystem/partition format %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, requires --overwrite"), - format); - return -1; - } - if (!(probe = blkid_new_probe_from_filename(device))) { virReportError(VIR_ERR_STORAGE_PROBE_FAILED, _("Failed to create filesystem probe for device %s"), @@ -2677,34 +2757,53 @@ virStorageBackendBLKIDProbeFS(const char *device, return -1; } - if (blkid_do_probe(probe) != 0) { - VIR_INFO("No filesystem of type '%s' found on device '%s'", - format, device); - } else if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { - 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; + /* Look for something on FS, if it either doesn't recognize the + * format type as a valid FS format type or it doesn't find a valid + * format type on the device, then perform the same check using + * partition probing. */ + rc = virStorageBackendBLKIDProbeFS(probe, device, format); + if (rc == VIR_STORAGE_BLKID_PROBE_UNDEFINED || + rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) + rc = virStorageBackendBLKIDProbePart(probe, device, format); + + switch (rc) { + case VIR_STORAGE_BLKID_PROBE_UNDEFINED: + ret = 0; + break; + + case VIR_STORAGE_BLKID_PROBE_ERROR: + virReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Failed to probe for format type '%s'"), format); + 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); + break; + + case VIR_STORAGE_BLKID_PROBE_MATCH: + virReportError(VIR_ERR_STORAGE_POOL_BUILT, + _("Device '%s' already formatted using '%s'"), + device, format); + 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); + break; } - if (blkid_do_probe(probe) != 1) { + if (ret == 0 && blkid_do_probe(probe) != 1) { virReportError(VIR_ERR_STORAGE_PROBE_FAILED, "%s", - _("Found additional probes to run, " - "filesystem probing may be incorrect")); - goto cleanup; + _("Found additional probes to run, probing may " + "be incorrect")); + ret = -1; } - ret = 0; - - cleanup: blkid_free_probe(probe); return ret; @@ -2713,8 +2812,8 @@ virStorageBackendBLKIDProbeFS(const char *device, #else /* #if WITH_BLKID */ static int -virStorageBackendBLKIDProbeFS(const char *device ATTRIBUTE_UNUSED, - const char *format ATTRIBUTE_UNUSED) +virStorageBackendBLKIDProbe(const char *device ATTRIBUTE_UNUSED, + const char *format ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("probing for filesystems is unsupported " @@ -2739,7 +2838,7 @@ bool virStorageBackendDeviceProbeEmpty(const char *devpath, const char *format) { - if (virStorageBackendBLKIDProbeFS(devpath, format) == 0) + if (virStorageBackendBLKIDProbe(devpath, format) == 0) true; return false; -- 2.7.4
On 12/15/2016 10:42 PM, John Ferlan wrote:
A device may be formatted using some sort of disk partition format type. We can check that using the blkid_ API's as well - so alter the logic to allow checking the device for both a filesystem and a disk partition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 177 ++++++++++++++++++++++++++++++++---------- 1 file changed, 138 insertions(+), 39 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 108f2b9..065f2ef 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2639,37 +2639,117 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
#if WITH_BLKID + +typedef enum { + VIR_STORAGE_BLKID_PROBE_ERROR = -1, + VIR_STORAGE_BLKID_PROBE_UNDEFINED, /* Nothing found */ + VIR_STORAGE_BLKID_PROBE_UNKNOWN, /* Don't know libvirt fs/part type */ + VIR_STORAGE_BLKID_PROBE_MATCH, /* Matches the on disk format */ + VIR_STORAGE_BLKID_PROBE_DIFFERENT, /* Format doesn't match on disk format */ +} virStorageBackendBLKIDProbeResult; + +/* + * Utility function to probe for a file system on the device using the + * blkid "superblock" (e.g. default) APIs. + * + * NB: In general this helper will handle the virStoragePoolFormatFileSystem + * format types; however, if called from the Disk path, the initial fstype + * check will fail forcing the usage of the ProbePart helper. + * + * Returns virStorageBackendBLKIDProbeResult enum + */ +static virStorageBackendBLKIDProbeResult +virStorageBackendBLKIDProbeFS(blkid_probe probe, + const char *device, + const char *format) +{ + const char *fstype = NULL; + + /* Make sure we're doing a superblock probe from the start */ + blkid_probe_enable_superblocks(probe, true); + blkid_probe_reset_superblocks_filter(probe); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No filesystem found on device '%s'", device); + return VIR_STORAGE_BLKID_PROBE_UNDEFINED; + } + + if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { + if (STREQ(fstype, format)) + return VIR_STORAGE_BLKID_PROBE_MATCH; + + return VIR_STORAGE_BLKID_PROBE_DIFFERENT; + } + + if (blkid_known_fstype(format) == 0) + return VIR_STORAGE_BLKID_PROBE_UNKNOWN; + + return VIR_STORAGE_BLKID_PROBE_ERROR; +}
I'm confused. So before this patch set this function was returning something among these lines. Then in patch #2 you make it return -1 or 0, and here we go again. The code after this patch looks sane though. And I don't really care that much about partial steps. The result looks good, therefore you have my ACK. Michal
On 01/09/2017 10:32 AM, Michal Privoznik wrote:
On 12/15/2016 10:42 PM, John Ferlan wrote:
A device may be formatted using some sort of disk partition format type. We can check that using the blkid_ API's as well - so alter the logic to allow checking the device for both a filesystem and a disk partition.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 177 ++++++++++++++++++++++++++++++++---------- 1 file changed, 138 insertions(+), 39 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 108f2b9..065f2ef 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2639,37 +2639,117 @@ virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
#if WITH_BLKID + +typedef enum { + VIR_STORAGE_BLKID_PROBE_ERROR = -1, + VIR_STORAGE_BLKID_PROBE_UNDEFINED, /* Nothing found */ + VIR_STORAGE_BLKID_PROBE_UNKNOWN, /* Don't know libvirt fs/part type */ + VIR_STORAGE_BLKID_PROBE_MATCH, /* Matches the on disk format */ + VIR_STORAGE_BLKID_PROBE_DIFFERENT, /* Format doesn't match on disk format */ +} virStorageBackendBLKIDProbeResult; + +/* + * Utility function to probe for a file system on the device using the + * blkid "superblock" (e.g. default) APIs. + * + * NB: In general this helper will handle the virStoragePoolFormatFileSystem + * format types; however, if called from the Disk path, the initial fstype + * check will fail forcing the usage of the ProbePart helper. + * + * Returns virStorageBackendBLKIDProbeResult enum + */ +static virStorageBackendBLKIDProbeResult +virStorageBackendBLKIDProbeFS(blkid_probe probe, + const char *device, + const char *format) +{ + const char *fstype = NULL; + + /* Make sure we're doing a superblock probe from the start */ + blkid_probe_enable_superblocks(probe, true); + blkid_probe_reset_superblocks_filter(probe); + + if (blkid_do_probe(probe) != 0) { + VIR_INFO("No filesystem found on device '%s'", device); + return VIR_STORAGE_BLKID_PROBE_UNDEFINED; + } + + if (blkid_probe_lookup_value(probe, "TYPE", &fstype, NULL) == 0) { + if (STREQ(fstype, format)) + return VIR_STORAGE_BLKID_PROBE_MATCH; + + return VIR_STORAGE_BLKID_PROBE_DIFFERENT; + } + + if (blkid_known_fstype(format) == 0) + return VIR_STORAGE_BLKID_PROBE_UNKNOWN; + + return VIR_STORAGE_BLKID_PROBE_ERROR; +}
I'm confused. So before this patch set this function was returning something among these lines. Then in patch #2 you make it return -1 or 0, and here we go again.
IIRC, the diffs were really ugly with the 1 step approach. Maybe if I created a step between 1 and 2 which introduced and implemented the VIR_STORAGE_BLKID_PROBE_* constants that may make it easier going from 2 to 3. John
The code after this patch looks sane though. And I don't really care that much about partial steps. The result looks good, therefore you have my ACK.
Michal
It's possible that the API could be called from a startup path in order to check whether the label on the device matches what our format is. In order to handle that condition, add a 'writelabel' boolean to the API in order to indicate whether a write or just read is about to happen. This alters two "error" conditions that would care about knowing. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 26 +++++++++++++++++++------- src/storage/storage_backend.h | 3 ++- src/storage/storage_backend_fs.c | 2 +- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 065f2ef..834973d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2730,6 +2730,7 @@ virStorageBackendBLKIDProbePart(blkid_probe probe, /* * @device: Path to device * @format: Desired format + * @writelabel: True if desire to write the label * * Use the blkid_ APIs in order to get details regarding whether a file * system or partition exists on the disk already. @@ -2740,7 +2741,8 @@ virStorageBackendBLKIDProbePart(blkid_probe probe, */ static int virStorageBackendBLKIDProbe(const char *device, - const char *format) + const char *format, + bool writelabel) { int ret = -1; @@ -2768,7 +2770,12 @@ virStorageBackendBLKIDProbe(const char *device, switch (rc) { case VIR_STORAGE_BLKID_PROBE_UNDEFINED: - ret = 0; + if (writelabel) + ret = 0; + else + virReportError(VIR_ERR_STORAGE_PROBE_FAILED, + _("Device '%s' is unrecognized, requires build"), + device); break; case VIR_STORAGE_BLKID_PROBE_ERROR: @@ -2784,9 +2791,12 @@ virStorageBackendBLKIDProbe(const char *device, break; case VIR_STORAGE_BLKID_PROBE_MATCH: - virReportError(VIR_ERR_STORAGE_POOL_BUILT, - _("Device '%s' already formatted using '%s'"), - device, format); + if (writelabel) + virReportError(VIR_ERR_STORAGE_POOL_BUILT, + _("Device '%s' already formatted using '%s'"), + device, format); + else + ret = 0; break; case VIR_STORAGE_BLKID_PROBE_DIFFERENT: @@ -2827,6 +2837,7 @@ virStorageBackendBLKIDProbe(const char *device ATTRIBUTE_UNUSED, /* virStorageBackendDeviceProbeEmpty: * @devpath: Path to the device to check * @format: Desired format string + * @writelabel: True if the caller expects to write the label * * Check if the @devpath has some sort of known file system using the * BLKID API if available. @@ -2836,9 +2847,10 @@ virStorageBackendBLKIDProbe(const char *device ATTRIBUTE_UNUSED, */ bool virStorageBackendDeviceProbeEmpty(const char *devpath, - const char *format) + const char *format, + bool writelabel) { - if (virStorageBackendBLKIDProbe(devpath, format) == 0) + if (virStorageBackendBLKIDProbe(devpath, format, writelabel) == 0) true; return false; diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 75f2ed6..4adc075 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -159,7 +159,8 @@ int virStorageBackendVolWipeLocal(virConnectPtr conn, unsigned int flags); bool virStorageBackendDeviceProbeEmpty(const char *devpath, - const char *format); + const char *format, + bool writelabel); typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c3986af..db5b340 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -693,7 +693,7 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) { ok_to_mkfs = true; } else if (flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE && - virStorageBackendDeviceProbeEmpty(device, format)) { + virStorageBackendDeviceProbeEmpty(device, format, true)) { ok_to_mkfs = true; } -- 2.7.4
Prior to starting up, let's be sure the target volume device is formatted as we expect; otherwise, inhibit the start. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_fs.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index db5b340..936e472 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -605,8 +605,17 @@ static int virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - if (pool->def->type != VIR_STORAGE_POOL_DIR && - virStorageBackendFileSystemMount(pool) < 0) + const char *format = + virStoragePoolFormatFileSystemTypeToString(pool->def->source.format); + const char *path = pool->def->source.devices[0].path; + + if (pool->def->type == VIR_STORAGE_POOL_DIR) + return 0; + + if (!virStorageBackendDeviceProbeEmpty(path, format, false)) + return -1; + + if (virStorageBackendFileSystemMount(pool) < 0) return -1; return 0; -- 2.7.4
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: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 168 ++++++++++++++++++++++++++++++++++++- src/storage/storage_backend_disk.c | 150 ++++----------------------------- tools/virsh.pod | 14 ++-- 3 files changed, 187 insertions(+), 145 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 834973d..cc04d3a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2704,6 +2704,14 @@ virStorageBackendBLKIDProbePart(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 @@ virStorageBackendBLKIDProbePart(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 @@ virStorageBackendBLKIDProbe(const char *device, * partition probing. */ rc = virStorageBackendBLKIDProbeFS(probe, device, format); if (rc == VIR_STORAGE_BLKID_PROBE_UNDEFINED || - rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) + rc == VIR_STORAGE_BLKID_PROBE_UNKNOWN) { + rc = virStorageBackendBLKIDProbePart(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 @@ virStorageBackendBLKIDProbe(const char *device, ret = -1; } + cleanup: blkid_free_probe(probe); return ret; @@ -2828,12 +2844,151 @@ virStorageBackendBLKIDProbe(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; +} + + /* virStorageBackendDeviceProbeEmpty: * @devpath: Path to the device to check * @format: Desired format string @@ -2850,8 +3005,13 @@ virStorageBackendDeviceProbeEmpty(const char *devpath, const char *format, bool writelabel) { - if (virStorageBackendBLKIDProbe(devpath, format, writelabel) == 0) - true; + int ret; + + if ((ret = virStorageBackendBLKIDProbe(devpath, format, writelabel)) == -2) + ret = virStorageBackendPARTEDValidLabel(devpath, writelabel); + + if (ret == 0) + return true; return false; } diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 97e5a8f..1d4e996 100644 --- a/src/storage/storage_backend_disk.c +++ b/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 (!virStorageBackendDeviceProbeEmpty(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 (virStorageBackendDeviceProbeEmpty(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) diff --git a/tools/virsh.pod b/tools/virsh.pod index b00fc8d..fd0f7fa 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3463,13 +3463,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>]] -- 2.7.4
Currently as long as the disk is formatted using a known parted format type, the algorithm is happy to continue. However, that leaves a scenario whereby a disk formatted using "pc98" could be used by a pool that's defined using "dvh" (or vice versa). Alter the check to be match and different and adjust the caller. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index cc04d3a..6dd0fa9 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2852,7 +2852,8 @@ virStorageBackendBLKIDProbe(const char *device ATTRIBUTE_UNUSED, typedef enum { VIR_STORAGE_PARTED_ERROR = -1, - VIR_STORAGE_PARTED_FOUND, /* Valid label found */ + VIR_STORAGE_PARTED_MATCH, /* Valid label found and matches format */ + VIR_STORAGE_PARTED_DIFFERENT, /* Valid label found but not match format */ 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*/ @@ -2865,7 +2866,8 @@ typedef enum { * returns virStorageBackendPARTEDResult */ static virStorageBackendPARTEDResult -virStorageBackendPARTEDFindLabel(const char *device) +virStorageBackendPARTEDFindLabel(const char *device, + const char *format) { const char *const args[] = { device, "print", "--script", NULL, @@ -2913,7 +2915,11 @@ virStorageBackendPARTEDFindLabel(const char *device) goto cleanup; } - ret = VIR_STORAGE_PARTED_FOUND; + /* Does the on disk match what the pool desired? */ + if (STREQ(start, format)) + ret = VIR_STORAGE_PARTED_MATCH; + + ret = VIR_STORAGE_PARTED_DIFFERENT; cleanup: virCommandFree(cmd); @@ -2943,12 +2949,13 @@ virStorageBackendPARTEDFindLabel(const char *device) */ static int virStorageBackendPARTEDValidLabel(const char *device, + const char *format, bool writelabel) { int ret = -1; virStorageBackendPARTEDResult check; - check = virStorageBackendPARTEDFindLabel(device); + check = virStorageBackendPARTEDFindLabel(device, format); switch (check) { case VIR_STORAGE_PARTED_ERROR: virReportError(VIR_ERR_OPERATION_FAILED, "%s", @@ -2956,15 +2963,21 @@ virStorageBackendPARTEDValidLabel(const char *device, "disk partition information")); break; - case VIR_STORAGE_PARTED_FOUND: + case VIR_STORAGE_PARTED_MATCH: if (writelabel) - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Valid disk label already present, " - "requires --overwrite")); + virReportError(VIR_ERR_OPERATION_INVALID, + _("Disk label already formatted using '%s'"), + format); else ret = 0; break; + case VIR_STORAGE_PARTED_DIFFERENT: + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Known, but different label format present, " + "requires build --overwrite")); + break; + case VIR_STORAGE_PARTED_UNKNOWN: if (writelabel) ret = 0; @@ -3008,7 +3021,7 @@ virStorageBackendDeviceProbeEmpty(const char *devpath, int ret; if ((ret = virStorageBackendBLKIDProbe(devpath, format, writelabel)) == -2) - ret = virStorageBackendPARTEDValidLabel(devpath, writelabel); + ret = virStorageBackendPARTEDValidLabel(devpath, format, writelabel); if (ret == 0) return true; -- 2.7.4
If the build fails, then we need to ensure that we've run pvremove on any devices which we've run pvcreate on; otherwise, a subsequent build could fail since running pvcreate twice on a device requires special force arguments. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 42 ++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index ca05fe1..1e59bab 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -64,6 +64,23 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, } +/* + * @path: Path to the device + * + * Remove the pv device since we're done with it. This ensures a subsequent + * create won't require special arguments in order for force recreation. + */ +static void +virStorageBackendLogicalRemoveDevice(const char *path) +{ + virCommandPtr cmd = virCommandNewArgList(PVREMOVE, path, NULL); + + if (virCommandRun(cmd, NULL) < 0) + VIR_INFO("Failed to pvremove logical device '%s'", path); + virCommandFree(cmd); +} + + #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR "mirror" #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_RAID "raid" @@ -752,6 +769,15 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, cleanup: virCommandFree(vgcmd); + + /* On any failure, run through the devices that had pvcreate run in + * in order to run pvremove on the device; otherwise, subsequent build + * will fail if a pvcreate had been run already. */ + if (ret < 0) { + size_t j; + for (j = 0; j < i; j++) + virStorageBackendLogicalRemoveDevice(pool->def->source.devices[j].path); + } return ret; } @@ -845,22 +871,12 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn ATTRIBUTE_UNUSED, NULL); if (virCommandRun(cmd, NULL) < 0) goto cleanup; - virCommandFree(cmd); - cmd = NULL; /* now remove the pv devices and clear them out */ + for (i = 0; i < pool->def->source.ndevice; i++) + virStorageBackendLogicalRemoveDevice(pool->def->source.devices[i].path); + ret = 0; - for (i = 0; i < pool->def->source.ndevice; i++) { - cmd = virCommandNewArgList(PVREMOVE, - pool->def->source.devices[i].path, - NULL); - if (virCommandRun(cmd, NULL) < 0) { - ret = -1; - break; - } - virCommandFree(cmd); - cmd = NULL; - } cleanup: virCommandFree(cmd); -- 2.7.4
Make the remaining code a bit cleaner. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 111 +++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 48 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 1e59bab..2d8e288 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -81,6 +81,66 @@ virStorageBackendLogicalRemoveDevice(const char *path) } +/* + * @path: Path to the device + * + * Initialize and pvcreate the device. + * + * Returns 0 on success, -1 on failure with error message set + */ +static int +virStorageBackendLogicalInitializeDevice(const char *path) +{ + int fd = -1; + char zeros[PV_BLANK_SECTOR_SIZE] = {0}; + int ret = -1; + virCommandPtr pvcmd = NULL; + + /* + * LVM requires that the first sector is blanked if using + * a whole disk as a PV. So we just blank them out regardless + * rather than trying to figure out if we're a disk or partition + */ + if ((fd = open(path, O_WRONLY)) < 0) { + virReportSystemError(errno, _("cannot open device '%s'"), path); + return -1; + } + + if (safewrite(fd, zeros, sizeof(zeros)) < 0) { + virReportSystemError(errno, _("cannot clear device header of '%s'"), + path); + goto cleanup; + } + + if (fsync(fd) < 0) { + virReportSystemError(errno, _("cannot flush header of device'%s'"), + path); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("cannot close device '%s'"), path); + goto cleanup; + } + + /* + * Initialize the physical volume because vgcreate is not + * clever enough todo this for us :-( + */ + pvcmd = virCommandNewArgList(PVCREATE, path, NULL); + if (virCommandRun(pvcmd, NULL) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FORCE_CLOSE(fd); + virCommandFree(pvcmd); + + return ret; +} + + #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped" #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_MIRROR "mirror" #define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_RAID "raid" @@ -700,65 +760,20 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned int flags) { virCommandPtr vgcmd; - int fd; - char zeros[PV_BLANK_SECTOR_SIZE]; int ret = -1; size_t i; virCheckFlags(0, -1); - memset(zeros, 0, sizeof(zeros)); - vgcmd = virCommandNewArgList(VGCREATE, pool->def->source.name, NULL); for (i = 0; i < pool->def->source.ndevice; i++) { - virCommandPtr pvcmd; - /* - * LVM requires that the first sector is blanked if using - * a whole disk as a PV. So we just blank them out regardless - * rather than trying to figure out if we're a disk or partition - */ - if ((fd = open(pool->def->source.devices[i].path, O_WRONLY)) < 0) { - virReportSystemError(errno, - _("cannot open device '%s'"), - pool->def->source.devices[i].path); - goto cleanup; - } - if (safewrite(fd, zeros, sizeof(zeros)) < 0) { - virReportSystemError(errno, - _("cannot clear device header of '%s'"), - pool->def->source.devices[i].path); - VIR_FORCE_CLOSE(fd); - goto cleanup; - } - if (fsync(fd) < 0) { - virReportSystemError(errno, - _("cannot flush header of device'%s'"), - pool->def->source.devices[i].path); - VIR_FORCE_CLOSE(fd); - goto cleanup; - } - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot close device '%s'"), - pool->def->source.devices[i].path); - goto cleanup; - } + const char *path = pool->def->source.devices[i].path; - /* - * Initialize the physical volume because vgcreate is not - * clever enough todo this for us :-( - */ - pvcmd = virCommandNewArgList(PVCREATE, - pool->def->source.devices[i].path, - NULL); - if (virCommandRun(pvcmd, NULL) < 0) { - virCommandFree(pvcmd); + if (virStorageBackendLogicalInitializeDevice(path) < 0) goto cleanup; - } - virCommandFree(pvcmd); - virCommandAddArg(vgcmd, pool->def->source.devices[i].path); + virCommandAddArg(vgcmd, path); } /* Now create the volume group itself */ -- 2.7.4
https://bugzilla.redhat.com/show_bug.cgi?id=1373711 Add support and documentation for the [NO_]OVERWRITE flags for the logical backend. Update virsh.pod with a description of the process for usage of the flags and building of the pool's volume group. Signed-off-by: John Ferlan <jferlan@redhat.com> --- v2: http://www.redhat.com/archives/libvir-list/2016-November/msg00810.html Changes are too numerous to mention, but suffice to say this patch will add the overwrite checking much the same as the fs and disk backing storage makes the checks. The only really "odd" part is that I found using the 'blkid -p /dev/XXX' that the target devices for a logical pool are formatted and blkid knows them as "LVM2_member" - so I went with it. src/libvirt_private.syms | 1 + src/storage/storage_backend_logical.c | 18 +++++++++++++++--- tools/virsh.pod | 12 +++++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7beebbf..02f4c38 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -876,6 +876,7 @@ virStoragePoolFormatDiskTypeFromString; virStoragePoolFormatDiskTypeToString; virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; +virStoragePoolFormatLogicalTypeToString; virStoragePoolGetVhbaSCSIHostParent; virStoragePoolLoadAllConfigs; virStoragePoolLoadAllState; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 2d8e288..f128941 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -759,17 +759,29 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool, unsigned int flags) { - virCommandPtr vgcmd; + virCommandPtr vgcmd = NULL; int ret = -1; - size_t i; + size_t i = 0; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret); + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE, + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, + cleanup); vgcmd = virCommandNewArgList(VGCREATE, pool->def->source.name, NULL); for (i = 0; i < pool->def->source.ndevice; i++) { const char *path = pool->def->source.devices[i].path; + /* The blkid FS and Part probing code doesn't know "lvm2" (this + * pool's only format type), but it does know "LVM2_member", so + * we'll pass that here */ + if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && + !virStorageBackendDeviceProbeEmpty(path, "LVM2_member", true)) + goto cleanup; + if (virStorageBackendLogicalInitializeDevice(path) < 0) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index fd0f7fa..2813fe2 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3452,7 +3452,7 @@ 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. +B<pool-build> a filesystem, disk, or logical 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 @@ -3471,6 +3471,16 @@ 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. +For a logical pool, if neither of them is specified or I<--no-overwrite> +is specified, B<pool-build> will check the target volume devices for +existing filesystems or partitions before attempting to initialize +and format each device for usage by the logical pool. If any 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 devices prior to initializing and formatting each device. Once +all the target volume devices are properly formatted via pvcreate, +the volume group will be created using all the devices. + =item B<pool-create> I<file> [I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]] -- 2.7.4
At startup time, rather than blindly trusting the target devices are still properly formatted, let's check to make sure the pool's target devices are all properly formatted before attempting to start the pool. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index f128941..a433e61 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -743,6 +743,19 @@ static int virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { + size_t i; + + /* Let's make sure the pool's devices are properly formatted */ + for (i = 0; i < pool->def->source.ndevice; i++) { + const char *path = pool->def->source.devices[i].path; + + /* The blkid FS and Part probing code doesn't know "lvm2" (this + * pool's only format type), but it does know "LVM2_member", so + * we'll pass that here */ + if (!virStorageBackendDeviceProbeEmpty(path, "LVM2_member", false)) + return -1; + } + /* Let's make sure that the pool's name matches the pvs output and * that the pool's source devices match the pvs output. */ -- 2.7.4
On Thu, Dec 15, 2016 at 04:42:16PM -0500, John Ferlan wrote:
At startup time, rather than blindly trusting the target devices are still properly formatted, let's check to make sure the pool's target devices are all properly formatted before attempting to start the pool.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_logical.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index f128941..a433e61 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -743,6 +743,19 @@ static int virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { + size_t i; + + /* Let's make sure the pool's devices are properly formatted */ + for (i = 0; i < pool->def->source.ndevice; i++) { + const char *path = pool->def->source.devices[i].path; + + /* The blkid FS and Part probing code doesn't know "lvm2" (this + * pool's only format type), but it does know "LVM2_member", so + * we'll pass that here */ + if (!virStorageBackendDeviceProbeEmpty(path, "LVM2_member", false)) + return -1; + }
This seems pointless. Is there any chance where this check would fail but the (arguably also pointless) virStorageBackendLogicalMatchPoolSource check below would succeed? Jan
ping? Thanks - John (couple of bz's sprinkled in here too from previous postings - patch 2 and patch 10 dealing with NO_OVERWRITE) On 12/15/2016 04:42 PM, John Ferlan wrote:
Two of these patches (2 and 10) are followups to no-overwrite patches previously posted (I left links in the patches for reference).
Essentially this series works to unify the probing code from the file system backend using blkid and the parted format reading code from the disk backend in one common set of functions.
In doing so, the disk backend will now pick up the ability to use the blkid 'partitions' APIs in order to "probe" the target device for a valid/known partition format type. Additionally, the file system probe will pick up a fallback to the parted mechanism from the disk backend if the 'blkid' APIs are not built into the system.
A few extra bells/whistles were added for fs pool startup to ensure that the format in the startup XML matches the format on the target disk, which is essentially the mechanism the disk backend used (although it was slightly broken).
Then in order to really pile on, the overwrite logic was added to the logical pool which didn't have any such checking (both build and start).
Testing was done locally using an iSCSI device and laying down different formats and seeing what would happen on various usages of the format (with --overwrite and --no-overwrite).
John Ferlan (11): storage: Introduce virStorageBackendDeviceProbeEmpty storage: Fix implementation of no-overwrite for file system backend storage: Add partition type checks for BLKID probing storage: Add writelabel bool for virStorageBackendDeviceProbe storage: For FS pool check for properly formatted target volume storage: Move and rename disk backend label checking storage: Adjust disk label found to match labels storage: Clean up logical pool devices on build failure storage: Extract logical device initialize into a helper storage: Add overwrite flag checking for logical pool storage: Validate the device formats at logical startup
src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 396 ++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 4 + src/storage/storage_backend_disk.c | 150 ++----------- src/storage/storage_backend_fs.c | 118 ++-------- src/storage/storage_backend_fs.h | 5 - src/storage/storage_backend_logical.c | 180 ++++++++++------ tools/virsh-pool.c | 2 +- tools/virsh.pod | 42 ++-- 9 files changed, 585 insertions(+), 313 deletions(-)
On 12/15/2016 10:42 PM, John Ferlan wrote:
Two of these patches (2 and 10) are followups to no-overwrite patches previously posted (I left links in the patches for reference).
Essentially this series works to unify the probing code from the file system backend using blkid and the parted format reading code from the disk backend in one common set of functions.
In doing so, the disk backend will now pick up the ability to use the blkid 'partitions' APIs in order to "probe" the target device for a valid/known partition format type. Additionally, the file system probe will pick up a fallback to the parted mechanism from the disk backend if the 'blkid' APIs are not built into the system.
A few extra bells/whistles were added for fs pool startup to ensure that the format in the startup XML matches the format on the target disk, which is essentially the mechanism the disk backend used (although it was slightly broken).
Then in order to really pile on, the overwrite logic was added to the logical pool which didn't have any such checking (both build and start).
Testing was done locally using an iSCSI device and laying down different formats and seeing what would happen on various usages of the format (with --overwrite and --no-overwrite).
John Ferlan (11): storage: Introduce virStorageBackendDeviceProbeEmpty storage: Fix implementation of no-overwrite for file system backend storage: Add partition type checks for BLKID probing storage: Add writelabel bool for virStorageBackendDeviceProbe storage: For FS pool check for properly formatted target volume storage: Move and rename disk backend label checking storage: Adjust disk label found to match labels storage: Clean up logical pool devices on build failure storage: Extract logical device initialize into a helper storage: Add overwrite flag checking for logical pool storage: Validate the device formats at logical startup
src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 396 ++++++++++++++++++++++++++++++++++ src/storage/storage_backend.h | 4 + src/storage/storage_backend_disk.c | 150 ++----------- src/storage/storage_backend_fs.c | 118 ++-------- src/storage/storage_backend_fs.h | 5 - src/storage/storage_backend_logical.c | 180 ++++++++++------ tools/virsh-pool.c | 2 +- tools/virsh.pod | 42 ++-- 9 files changed, 585 insertions(+), 313 deletions(-)
ACK to all the patches but 02/11. I think a little discussion is needed there. Michal
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik