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@xxxxxxxxxx> >> --- >> >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
