8:24 a.m.
Few recent patches broke stuff. Revert/fix them.
Peter Krempa (3):
Revert "storage: For FS pool check for properly formatted target
volume"
Revert "storage: Validate the device formats at logical startup"
storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
src/storage/storage_backend_fs.c | 13 ++-----------
src/storage/storage_backend_logical.c | 15 +--------------
2 files changed, 3 insertions(+), 25 deletions(-)
--
2.11.0
8:24 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
The check does not work properly (crashes) with netfs filesystems and
also checking that a device is not empty when attempting to mount a
filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really
see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for
blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
---
src/storage/storage_backend_fs.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index f4341f32c..f0ef07b2f 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -605,17 +605,8 @@ static int
virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool)
{
- const char *format =
- virStoragePoolFormatFileSystemTypeToString(pool->def->source.format);
- const char *path = pool->def->source.devices[0].path;
-
- if (pool->def->type == VIR_STORAGE_POOL_DIR)
- return 0;
-
- if (!virStorageBackendDeviceIsEmpty(path, format, false))
- return -1;
-
- if (virStorageBackendFileSystemMount(pool) < 0)
+ if (pool->def->type != VIR_STORAGE_POOL_DIR &&
+ virStorageBackendFileSystemMount(pool) < 0)
return -1;
return 0;
--
2.11.0
8:36 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check does not work properly (crashes) with netfs filesystems
and
also checking that a device is not empty when attempting to mount a
filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really
see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for
blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
---
src/storage/storage_backend_fs.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
Instead of reverting why not just fix the issue.
John
diff --git a/src/storage/storage_backend_fs.c
b/src/storage/storage_backend_fs.c
index f4341f32c..f0ef07b2f 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -605,17 +605,8 @@ static int
virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool)
{
- const char *format =
- virStoragePoolFormatFileSystemTypeToString(pool->def->source.format);
- const char *path = pool->def->source.devices[0].path;
-
- if (pool->def->type == VIR_STORAGE_POOL_DIR)
- return 0;
-
- if (!virStorageBackendDeviceIsEmpty(path, format, false))
- return -1;
-
- if (virStorageBackendFileSystemMount(pool) < 0)
+ if (pool->def->type != VIR_STORAGE_POOL_DIR &&
+ virStorageBackendFileSystemMount(pool) < 0)
return -1;
return 0;
8:41 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
> The check does not work properly (crashes) with netfs filesystems and
> also checking that a device is not empty when attempting to mount a
> filesystem is not very usefull since the mount will fail anyways.
>
> As the code would improve only a very minor corner case I don't really
> see a reason to have this code at all.
>
> This code would also fail if libvirt is compiled without support for
> blkid and without parted.
>
> This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
> ---
> src/storage/storage_backend_fs.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
Instead of reverting why not just fix the issue.
Because I think the whole check is pointless. It's valid only when
mounting a local filesystem, and in that case basically the same check
is done when mounting the filesystem by the kernel.
6:45 p.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
On 01/12/2017 09:41 AM, Peter Krempa wrote:
On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
>
>
> On 01/12/2017 09:24 AM, Peter Krempa wrote:
>> The check does not work properly (crashes) with netfs filesystems and
>> also checking that a device is not empty when attempting to mount a
>> filesystem is not very usefull since the mount will fail anyways.
>>
>> As the code would improve only a very minor corner case I don't really
>> see a reason to have this code at all.
>>
>> This code would also fail if libvirt is compiled without support for
>> blkid and without parted.
>>
>> This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
>> ---
>> src/storage/storage_backend_fs.c | 13 ++-----------
>> 1 file changed, 2 insertions(+), 11 deletions(-)
>>
>
> Instead of reverting why not just fix the issue.
Because I think the whole check is pointless. It's valid only when
mounting a local filesystem, and in that case basically the same check
is done when mounting the filesystem by the kernel.
Here's the difference between the check and removing the check (not
withstanding the no PARTED and no BLKID available)...
Create a pool using ext4, start it - life is happy. Destroy the pool.
Define a pool using xfs. Start it
w/ my change:
# virsh pool-start fs
error: Failed to start pool fs
error: Storage pool already built: Device '/dev/sde' formatted cannot
overwrite using 'xfs', requires build --overwrite
w/o my change (e.g. if the change is reverted):
# virsh pool-start fs
error: Failed to start pool fs
error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde
/home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad
option, bad superblock on /dev/sde,
missing codepage or helper program, or other error
In some cases useful info is found in syslog - try
dmesg | tail or so.
----
I personally like the former, but I do understand/agree that the latter
will also work.
John
1:34 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
On Thu, Jan 12, 2017 at 19:45:52 -0500, John Ferlan wrote:
Here's the difference between the check and removing the check
(not
withstanding the no PARTED and no BLKID available)...
Create a pool using ext4, start it - life is happy. Destroy the pool.
Define a pool using xfs. Start it
w/ my change:
# virsh pool-start fs
error: Failed to start pool fs
error: Storage pool already built: Device '/dev/sde' formatted cannot
overwrite using 'xfs', requires build --overwrite
This error message doesn't make any sense to me. You are starting a
pool, but libvirt is complaining that it is already built (well, it must
be, otherwise you cannot start it) and suggesting to use --overwrite,
which is specific to virsh (API users would be confused) and gives the
feeling the command is trying to format the device while it only tries
to mount it. In other words, the check makes sense when a user tries to
built (i.e., format) the pool, but it is pretty misleading here.
w/o my change (e.g. if the change is reverted):
# virsh pool-start fs
error: Failed to start pool fs
error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde
/home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad
option, bad superblock on /dev/sde,
On the other hand, this error is pretty clear.
Jirka
2:06 a.m.
New subject: [libvirt] [PATCH 1/3] Revert "storage: For FS pool check for properly formatted target volume"
On Thu, Jan 12, 2017 at 19:45:52 -0500, John Ferlan wrote:
On 01/12/2017 09:41 AM, Peter Krempa wrote:
> On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
>>
>>
>> On 01/12/2017 09:24 AM, Peter Krempa wrote:
>>> The check does not work properly (crashes) with netfs filesystems and
>>> also checking that a device is not empty when attempting to mount a
>>> filesystem is not very usefull since the mount will fail anyways.
>>>
>>> As the code would improve only a very minor corner case I don't really
>>> see a reason to have this code at all.
>>>
>>> This code would also fail if libvirt is compiled without support for
>>> blkid and without parted.
>>>
>>> This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4.
>>> ---
>>> src/storage/storage_backend_fs.c | 13 ++-----------
>>> 1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>
>> Instead of reverting why not just fix the issue.
>
> Because I think the whole check is pointless. It's valid only when
> mounting a local filesystem, and in that case basically the same check
> is done when mounting the filesystem by the kernel.
>
Here's the difference between the check and removing the check (not
withstanding the no PARTED and no BLKID available)...
Create a pool using ext4, start it - life is happy. Destroy the pool.
Define a pool using xfs. Start it
w/ my change:
# virsh pool-start fs
error: Failed to start pool fs
error: Storage pool already built: Device '/dev/sde' formatted cannot
overwrite using 'xfs', requires build --overwrite
I think that the error code and the message are in fact misleading here.
You are starting the pool not building it, so both the message comming
from the error code and the free form string state nonsensical facts.
w/o my change (e.g. if the change is reverted):
# virsh pool-start fs
error: Failed to start pool fs
error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde
/home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad
option, bad superblock on /dev/sde,
missing codepage or helper program, or other error
In some cases useful info is found in syslog - try
dmesg | tail or so.
While this is a bit too verbose, but in my opinion describes the problem
better.
Peter
8:24 a.m.
New subject: [libvirt] [PATCH 2/3] Revert "storage: Validate the device formats at logical startup"
The check is pointless since LVM is capable to detect it's own members
and the check is flawed as it would fail if neither libblkid nor parted
is installed.
We don't really need to babysit LVM in this way.
This reverts commit cb38b6cbc7e35e7ee92a7f54828f21261227d17a.
---
src/storage/storage_backend_logical.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 85e080bf9..6a6720e22 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -743,19 +743,6 @@ static int
virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool)
{
- size_t i;
-
- /* Let's make sure the pool's devices are properly formatted */
- for (i = 0; i < pool->def->source.ndevice; i++) {
- const char *path = pool->def->source.devices[i].path;
-
- /* The blkid FS and Part probing code doesn't know "lvm2" (this
- * pool's only format type), but it does know "LVM2_member", so
- * we'll pass that here */
- if (!virStorageBackendDeviceIsEmpty(path, "LVM2_member", false))
- return -1;
- }
-
/* Let's make sure that the pool's name matches the pvs output and
* that the pool's source devices match the pvs output.
*/
--
2.11.0
8:41 a.m.
New subject: [libvirt] [PATCH 2/3] Revert "storage: Validate the device formats at logical startup"
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check is pointless since LVM is capable to detect it's own
members
and the check is flawed as it would fail if neither libblkid nor parted
is installed.
We don't really need to babysit LVM in this way.
This reverts commit cb38b6cbc7e35e7ee92a7f54828f21261227d17a.
---
src/storage/storage_backend_logical.c | 13 -------------
1 file changed, 13 deletions(-)
Ambivalent on this one - the virStorageBackendDeviceIsEmpty was added
here and in the previous patch for FS mainly because Disk pools had a
similar check. Since each uses some sort of device backend onto which a
specific format (as defined by the pool) should be written - it just
seem logical to check.
You call it babysitting - I call it validation.
John
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 85e080bf9..6a6720e22 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -743,19 +743,6 @@ static int
virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool)
{
- size_t i;
-
- /* Let's make sure the pool's devices are properly formatted */
- for (i = 0; i < pool->def->source.ndevice; i++) {
- const char *path = pool->def->source.devices[i].path;
-
- /* The blkid FS and Part probing code doesn't know "lvm2" (this
- * pool's only format type), but it does know "LVM2_member", so
- * we'll pass that here */
- if (!virStorageBackendDeviceIsEmpty(path, "LVM2_member", false))
- return -1;
- }
-
/* Let's make sure that the pool's name matches the pvs output and
* that the pool's source devices match the pvs output.
*/
8:24 a.m.
New subject: [libvirt] [PATCH 3/3] storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
Commit f573f84eb7 introduced flawed logic which would cause a regression
in creating of lvm volumes when neither libblkid nor parted are
installed.
Fix the logic so that it triggers only if NO_OVERWRITE was specified
explicitly.
---
src/storage/storage_backend_logical.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 6a6720e22..77fc0d3c2 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -778,7 +778,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
/* The blkid FS and Part probing code doesn't know "lvm2" (this
* pool's only format type), but it does know "LVM2_member", so
* we'll pass that here */
- if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
+ if ((flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE) &&
!virStorageBackendDeviceIsEmpty(path, "LVM2_member", true))
goto cleanup;
--
2.11.0
9:12 a.m.
New subject: [libvirt] [PATCH 3/3] storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
On 01/12/2017 09:24 AM, Peter Krempa wrote:
Commit f573f84eb7 introduced flawed logic which would cause a
regression
in creating of lvm volumes when neither libblkid nor parted are
installed.
Fix the logic so that it triggers only if NO_OVERWRITE was specified
explicitly.
---
src/storage/storage_backend_logical.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
So if no flag is provided, this change would essentially allow an
overwrite of data on the target device.
If the target host doesn't have blkid and/or parted, then not only does
this code fail, so does the fs and disk code - even prior to any of the
patches from the series. For both disk/fs - if someone doesn't have
blkid/parted, then they must provide the overwrite flag to indicate they
know what they're doing; otherwise, we just fail because we either
cannot or did not validate that the device had nothing on it and we
don't want to overwrite something that may be important.
What makes logical "special" so that we just ignore the check? History?
- that is since we've never checked, thus we have to keep that going
forward? From a data integrity viewpoint, that doesn't feel right.
John
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 6a6720e22..77fc0d3c2 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -778,7 +778,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn
ATTRIBUTE_UNUSED,
/* The blkid FS and Part probing code doesn't know "lvm2" (this
* pool's only format type), but it does know "LVM2_member", so
* we'll pass that here */
- if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) &&
+ if ((flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE) &&
!virStorageBackendDeviceIsEmpty(path, "LVM2_member", true))
goto cleanup;
12:08 p.m.
New subject: [libvirt] [PATCH 3/3] storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
On Thu, Jan 12, 2017 at 10:12:39 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
> Commit f573f84eb7 introduced flawed logic which would cause a regression
> in creating of lvm volumes when neither libblkid nor parted are
> installed.
>
> Fix the logic so that it triggers only if NO_OVERWRITE was specified
> explicitly.
> ---
> src/storage/storage_backend_logical.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
So if no flag is provided, this change would essentially allow an
overwrite of data on the target device.
If the target host doesn't have blkid and/or parted, then not only does
this code fail, so does the fs and disk code - even prior to any of the
Disk code is guaranteed to have at least 'parted'. It would not be built
otherwise
patches from the series. For both disk/fs - if someone doesn't
have
blkid/parted, then they must provide the overwrite flag to indicate they
know what they're doing; otherwise, we just fail because we either
cannot or did not validate that the device had nothing on it and we
don't want to overwrite something that may be important.
Umm, you know that the code checks only wheter there's a different LVM
physical volume? If there's a filesystem or whatever else it still nukes
it.
What makes logical "special" so that we just ignore the
check? History?
I was motivated by history and the strange tristateness of the flags.
Also one of the big problems were the name and semantics of
'virStorageBackendDeviceIsEmpty'. The negative semantics and name
containing "Empty" distracted me.
At any rate, the usage as is makes sense, so I'll drop this patch.
On the other hand I figured out that the usage of parted together with
blkid is broken:
In virStorageBackendPARTEDValidLabel which calls 'parted' looks for
'Partition table' and then feeds it to
'virStoragePoolFormatDiskTypeFromString'
This type implements the following values:
VIR_ENUM_IMPL(virStoragePoolFormatDisk,
VIR_STORAGE_POOL_DISK_LAST,
"unknown", "dos", "dvh", "gpt",
"mac", "bsd", "pc98", "sun",
"lvm2")
The code paths in :
virStorageBackendLogicalStartPool
virStorageBackendLogicalBuildPool
virStorageBackendMakeFileSystem
virStorageBackendFileSystemStart
feed values from 'virStoragePoolFormatFileSystem' or the value
'LVM2_member'. The virStoragePoolFormatFileSystem implements the
following strings:
VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
VIR_STORAGE_POOL_FS_LAST,
"auto", "ext2", "ext3",
"ext4", "ufs", "iso9660", "udf",
"gfs", "gfs2", "vfat", "hfs+",
"xfs", "ocfs2")
There's no intersection of those two thus if you don't have blkid
installed the code always fails to match the type.
virStorageBackendPARTEDValidLabel is not a replacement for
virStorageBackendBLKIDFindEmpty.
The patchset wrongly joins the cases for disks, where the disklabel
(partition table) is relevant, with other cases where the type of the
filesystem itself is relevant.
The above facts also result into the fact that when liblkid is not
installed starting of logical and filesystem pools will fail all the
time. This is the justification for the two revert patches.
In addition the intergation of virStorageBackendBLKIDFindFS and
virStorageBackendBLKIDFindPart in virStorageBackendBLKIDFindEmpty is
weird.
There are two separate cases:
1) you need to look up the disklabel (partition table) type
2) you need to look up the filesystem type
I don't see any sane reason for combining the two.
7:13 p.m.
New subject: [libvirt] [PATCH 3/3] storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
On 01/12/2017 01:08 PM, Peter Krempa wrote:
On Thu, Jan 12, 2017 at 10:12:39 -0500, John Ferlan wrote:
>
>
> On 01/12/2017 09:24 AM, Peter Krempa wrote:
>> Commit f573f84eb7 introduced flawed logic which would cause a regression
>> in creating of lvm volumes when neither libblkid nor parted are
>> installed.
>>
>> Fix the logic so that it triggers only if NO_OVERWRITE was specified
>> explicitly.
>> ---
>> src/storage/storage_backend_logical.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> So if no flag is provided, this change would essentially allow an
> overwrite of data on the target device.
>
> If the target host doesn't have blkid and/or parted, then not only does
> this code fail, so does the fs and disk code - even prior to any of the
Disk code is guaranteed to have at least 'parted'. It would not be built
otherwise
True... and the Disk code ended up being the "model" to a degree.
> patches from the series. For both disk/fs - if someone
doesn't have
> blkid/parted, then they must provide the overwrite flag to indicate they
> know what they're doing; otherwise, we just fail because we either
> cannot or did not validate that the device had nothing on it and we
> don't want to overwrite something that may be important.
Umm, you know that the code checks only wheter there's a different LVM
physical volume? If there's a filesystem or whatever else it still nukes
it.
> What makes logical "special" so that we just ignore the check? History?
I was motivated by history and the strange tristateness of the flags.
Also one of the big problems were the name and semantics of
'virStorageBackendDeviceIsEmpty'. The negative semantics and name
containing "Empty" distracted me.
Well the "Probe" name was disliked by review and "Empty" while not
perfect was deemed OK. Is there a name that you believe would be better?
At any rate, the usage as is makes sense, so I'll drop this
patch.
On the other hand I figured out that the usage of parted together with
blkid is broken:
In virStorageBackendPARTEDValidLabel which calls 'parted' looks for
'Partition table' and then feeds it to
'virStoragePoolFormatDiskTypeFromString'
This type implements the following values:
VIR_ENUM_IMPL(virStoragePoolFormatDisk,
VIR_STORAGE_POOL_DISK_LAST,
"unknown", "dos", "dvh", "gpt",
"mac", "bsd", "pc98", "sun",
"lvm2")
The code paths in :
virStorageBackendLogicalStartPool
virStorageBackendLogicalBuildPool
virStorageBackendMakeFileSystem
virStorageBackendFileSystemStart
feed values from 'virStoragePoolFormatFileSystem' or the value
'LVM2_member'. The virStoragePoolFormatFileSystem implements the
following strings:
VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
VIR_STORAGE_POOL_FS_LAST,
"auto", "ext2", "ext3",
"ext4", "ufs", "iso9660", "udf",
"gfs", "gfs2", "vfat", "hfs+",
"xfs", "ocfs2")
There's no intersection of those two thus if you don't have blkid
installed the code always fails to match the type.
virStorageBackendPARTEDValidLabel is not a replacement for
virStorageBackendBLKIDFindEmpty.
The patchset wrongly joins the cases for disks, where the disklabel
(partition table) is relevant, with other cases where the type of the
filesystem itself is relevant.
The intent was being able to use BLKID if available instead of PARTED.
Combining blkid (fsid/partid) and parted was a suggestion from a review
of the FS overwrite issue. See "[C]" towards the end of the following:
http://www.redhat.com/archives/libvir-list/2016-November/msg00858.html
For the FS pool the BLKID should be the same... Being able to have a
fall back to have PARTED run just in case there was something that
PARTED knew about felt like a bonus. Being able to use BLKID for disk
in order to determine if "something" was on the target device (e.g. ends
up being the partfs calls) seemed like a better mechanism than spawning
off PARTED and all that comes with that.
Taking it the extra step for the logical backend seemed like a logical
(sic) step to me.
I'll need to think a bit longer on the rest of what you wrote and this
is not something that doesn't need to hold up 3.0.0, so do what you must
and I'll put this on a list of things to get back to.
The above facts also result into the fact that when liblkid is not
installed starting of logical and filesystem pools will fail all the
time. This is the justification for the two revert patches.
Simple enough to fix - return -2 if PARTED is not installed and then
check in the caller for -2 and change to ret = 0. Although I do
understand your point.
In addition the intergation of virStorageBackendBLKIDFindFS and
virStorageBackendBLKIDFindPart in virStorageBackendBLKIDFindEmpty is
weird.
There are two separate cases:
1) you need to look up the disklabel (partition table) type
2) you need to look up the filesystem type
I don't see any sane reason for combining the two.
It's a means to ensure you don't have something "unexpected" on the
target device you're about to build/use for your pool. Before any of
these patches, all FS would do is check for "known" FS types...
Furthermore it would *filter* on the current type, so it would
erroneously *pass* if the format type differed - thus allowing a build
of "new" format type on some existing type. Before any of these
patches, all DISK would do is use PARTED. Now it can use BLKID in order
to make sure something isn't on the target device. And then LOGICAL
did nothing - it just happily overwrote the first 512 bytes of the
target device and ran pvcreate on it without checking for *anything*.
John
8:33 a.m.
On Thu, Jan 12, 2017 at 03:24:07PM +0100, Peter Krempa wrote:
Few recent patches broke stuff. Revert/fix them.
Peter Krempa (3):
Revert "storage: For FS pool check for properly formatted target
volume"
Revert "storage: Validate the device formats at logical startup"
storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
src/storage/storage_backend_fs.c | 13 ++-----------
src/storage/storage_backend_logical.c | 15 +--------------
2 files changed, 3 insertions(+), 25 deletions(-)
ACK series
Jan
2900
days inactive
2901
days old
13 comments
4 participants
participants (4)
-
Jiri Denemark -
John Ferlan -
Ján Tomko -
Peter Krempa