On 12/15/2016 10:42 PM, John Ferlan wrote: 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. Michal

On 01/10/2017 01:12 AM, 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 :-). ACK if you make the function name reflect what the function actually does. Michal

On 12/15/2016 10:42 PM, John Ferlan wrote: 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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

On Thu, Dec 15, 2016 at 04:42:16PM -0500, John Ferlan wrote: This seems pointless. Is there any chance where this check would fail but the (arguably also pointless) virStorageBackendLogicalMatchPoolSource check below would succeed? Jan