[libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend
Michal Privoznik
mprivozn at redhat.com
Mon Jan 9 15:32:56 UTC 2017
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?
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
More information about the libvir-list
mailing list