[libvirt] [PATCH 3/3] storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.

[John Ferlan]

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