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

Maybe it's a naming problem. MatchFS() sounds more like reflecting what
this function actually does after this change.

>   */
> -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