openkylin
/
libvirt
mirror of https://gitee.com/openkylin/libvirt.git
9
0
Fork 0
Code Issues Projects Releases Wiki Activity

storage: Fix implementation of no-overwrite for file system backend 

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@redhat.com>
This commit is contained in:
John Ferlan 2016-12-14 15:14:19 -05:00
parent 553d21da6c
commit f23d4bbce3
5 changed files with 58 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
src/storage/storage_backend.c
View File

@ -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
*/
static virStoragePoolProbeResult
static int
virStorageBackendBLKIDFindFS(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;
}
#else /* #if WITH_BLKID */
static virStoragePoolProbeResult
static int
virStorageBackendBLKIDFindFS(const char *device ATTRIBUTE_UNUSED,
const char *format ATTRIBUTE_UNUSED)
{
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("probing for filesystems is unsupported "
"by this build"));
return FILESYSTEM_PROBE_ERROR;
return -1;
}
#endif /* #if WITH_BLKID */
@ -2746,6 +2739,5 @@ bool
virStorageBackendDeviceIsEmpty(const char *devpath,
const char *format)
{
return virStorageBackendBLKIDFindFS(devpath, format) ==
FILESYSTEM_PROBE_NOT_FOUND;
return virStorageBackendBLKIDFindFS(devpath, format) == 0;
}

15
src/storage/storage_backend_fs.c
View File

@ -712,12 +712,15 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool,
*
* Build a directory or FS based storage pool.
*
* If no flag is set, it only makes the directory; If
* VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it probes to determine if
* filesystem already exists on the target device, renurning an error
* if exists, or using mkfs to format the target device if not; If
* VIR_STORAGE_POOL_BUILD_OVERWRITE is set, mkfs is always executed,
* any existed data on the target device is overwritten unconditionally.
* If no flag is set, it only makes the directory.
*
* If VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it probes to determine if
* any filesystem already exists on the target device, returning an error
* if one exists. If no filesystem already exists, use mkfs to format the
* target device.
*
* If VIR_STORAGE_POOL_BUILD_OVERWRITE is set, mkfs is always executed and
* any existing data on the target device is overwritten unconditionally.
*
* The underlying source device is mounted for FS based pools.
*

5
src/storage/storage_backend_fs.h
View File

@ -31,11 +31,6 @@ extern virStorageBackend virStorageBackendFileSystem;
extern virStorageBackend virStorageBackendNetFileSystem;
# endif
typedef enum {
FILESYSTEM_PROBE_FOUND,
FILESYSTEM_PROBE_NOT_FOUND,
FILESYSTEM_PROBE_ERROR,
} virStoragePoolProbeResult;
extern virStorageBackend virStorageBackendDirectory;
extern virStorageFileBackend virStorageFileBackendFile;

2
tools/virsh-pool.c
View File

@ -48,7 +48,7 @@
{.name = "no-overwrite", \
.type = VSH_OT_BOOL, \
.flags = 0, \
.help = N_("do not overwrite an existing pool of this type") \
.help = N_("do not overwrite any existing data") \
} \
#define VIRSH_COMMON_OPT_POOL_OVERWRITE \

32
tools/virsh.pod
View File

@ -3466,20 +3466,24 @@ Configure whether I<pool> should automatically start at boot.
Build a given pool.
Options I<--overwrite> and I<--no-overwrite> can only be used for
B<pool-build> a filesystem or disk pool. For a file system pool if
neither of them is specified, B<pool-build> makes the directory. If
I<--no-overwrite> is specified, it probes to determine if a
filesystem already exists on the target device, returning an error
if exists, or using mkfs to format the target device if not. If
I<--overwrite> is specified, mkfs is always executed and any existing
data on the target device is overwritten unconditionally. For a disk
pool, if neither of them is specified or I<--no-overwrite> is specified,
B<pool-build> will use 'parted --print' in order to determine if the
disk already has a label before attempting to create one. Only if a disk
does not already have one will a label be created. If I<--overwrite> is
specified or it's been determined that the disk doesn't already have one,
'parted mklabel' will be used to create a label of the format specified
by the pool source format type or "dos" if not specified for the pool.
B<pool-build> a filesystem or disk pool.
For a file system pool if neither flag is specified, then B<pool-build>
just makes the target path directory and no attempt to run mkfs on the
target volume device. If I<--no-overwrite> is specified, it probes to
determine if a filesystem already exists on the target device, returning
an error if one exists or using mkfs to format the target device if not.
If I<--overwrite> is specified, mkfs is always executed and any existing
data on the target device is overwritten unconditionally.
For a disk pool, if neither of them is specified or I<--no-overwrite>
is specified, B<pool-build> will use 'parted --print' in order to
determine if the disk already has a label before attempting to create
one. Only if a disk does not already have one will a label be created.
If I<--overwrite> is specified or it's been determined that the disk
doesn't already have one, 'parted mklabel' will be used to create a
label of the format specified by the pool source format type or "dos"
if not specified for the pool.
=item B<pool-create> I<file>
[I<--build>] [[I<--overwrite>] | [I<--no-overwrite>]]