[libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend
John Ferlan
jferlan at redhat.com
Tue Jan 10 00:12:20 UTC 2017
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.
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
>
More information about the libvir-list
mailing list