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

[Michal Privoznik]

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 at 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