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

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(a)redhat.com&gt; > --- > > 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