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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux