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

[Peter Krempa]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170112/5eddf467/attachment-0001.sig>