Re: [libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend

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