Libguestfs - Jan 2018 - [RFC] Inconsistent output of guestfs_list_filesystems

Before I rush to change something I request your comments on the subject.
Let me know what do you think and if it does make sense.

The issue: guesfs_list_filesystems is inconsistent in its output.
For, example, it filters out partitioned physical devices but doesn't do the
same for MD devices.

More over, according to its name and API documentation guestfs_list_filesystem
should return something
which potentially can be mounted (SWAP is an exclusion and it is stated in API
doc).
It is the reason partitioned physical devices are filtered out:
they are not mountable even in theory (they don't contain filesystem if they
contain partition(s)).

Another example of non-mountable block device is extended MSDOS partition.

Also nothing stops you from creating partition table and partitions on logical
volume
(yes, it is probably uncommon but nevertheless). In this case such partitions
are ignored completely.

Yet another example is Windows dynamic disks aka LDM: guestfs_list_filesystems
returns both
ldm_vol_ and ldm_part_ but strongly speaking only volumes should be considered
as mountables.

And last example is again with Windows dynamic disks and their physical
counterparts:
guestfs_list_filesystems filters out disks with MBR type byte 0x42 but what
about GPT?

Patch #1 will add one more test to demonstrate one of the issues described above
(partitioned MD device).
Patch #2 will define function which tells if device is partitioned (yes, it
doesn't work with dm-devices).
Patch #3 fixes the issue with partitioned MD devices.

At the moment don't pay too much attention at the code itself - it is just a
quick way to demonstrate the issue.

Test guestfish list-filesystems command finds file system on partitioned md
device and does't take into account md device itself (similar to as physical
devices are skipped if they are partitioned)
---
 tests/md/Makefile.am               |  1 +
 tests/md/test-list-filesystems2.sh | 79 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100755 tests/md/test-list-filesystems2.sh

diff --git a/tests/md/Makefile.am b/tests/md/Makefile.am
index 42260af..9be9ca4 100644
--- a/tests/md/Makefile.am
+++ b/tests/md/Makefile.am
@@ -21,6 +21,7 @@ TESTS = \
 	test-inspect-fstab.sh \
 	test-inspect-fstab-md.sh \
 	test-list-filesystems.sh \
+	test-list-filesystems2.sh \
 	test-list-md-devices.sh \
 	test-lvm-on-md-device.sh \
 	test-md-and-lvm-devices.sh \
diff --git a/tests/md/test-list-filesystems2.sh
b/tests/md/test-list-filesystems2.sh
new file mode 100755
index 0000000..36b6d8a
--- /dev/null
+++ b/tests/md/test-list-filesystems2.sh
@@ -0,0 +1,79 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2018 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test guestfish list-filesystems command finds file system on partitioned
+# md device and does't take into account md device itself (similar to as
+# physical devices are skipped if they are partitioned)
+
+set -e
+
+$TEST_FUNCTIONS
+skip_if_skipped
+
+disk1=list-filesystems2-1.img
+disk2=list-filesystems2-2.img
+
+rm -f $disk1 $disk2
+
+# Clean up if the script is killed or exits early
+cleanup ()
+{
+    status=$?
+    set +e
+
+    # Don't delete the output files if non-zero exit
+    if [ "$status" -eq 0 ]; then rm -f $disk1 $disk2; fi
+
+    exit $status
+}
+trap cleanup INT QUIT TERM EXIT
+
+output=$(
+guestfish <<EOF
+# Add 2 empty disks
+sparse $disk1 50M
+sparse $disk2 50M
+run
+
+# Create a raid0 based on the 2 disks
+md-create test "/dev/sda /dev/sdb" level:raid0
+
+part-init /dev/md127 mbr
+part-add /dev/md127 p 64 41023
+part-add /dev/md127 p 41024 81983
+
+# Create filesystems
+mkfs ext3 /dev/md127p1
+mkfs ext4 /dev/md127p2
+
+list-filesystems
+EOF
+)
+
+expected="/dev/md127p1: ext3
+/dev/md127p2: ext4"
+
+if [ "$output" != "$expected" ]; then
+    echo "$0: error: actual output did not match expected output"
+    echo -e "actual:\n$output"
+    echo -e "expected:\n$expected"
+    exit 1
+fi
+
+# cleanup() is called implicitly which cleans up everything
+exit 0
-- 
2.9.5

The function takes one argument - device path (e.g. /dev/sda) and returns true
if device contains partition(s) or false otherwise. The function look in
/sys/block/<device>/ for entries starting with <device>, eg.
/sys/block/sda/sda1.
---
 daemon/devsparts.ml  | 14 ++++++++++++++
 daemon/devsparts.mli |  1 +
 2 files changed, 15 insertions(+)

diff --git a/daemon/devsparts.ml b/daemon/devsparts.ml
index 54108bb..60ac11a 100644
--- a/daemon/devsparts.ml
+++ b/daemon/devsparts.ml
@@ -71,6 +71,20 @@ let list_devices ()      map_block_devices ~return_md:false
((^) "/dev/") in
   sort_device_names devices
 
+let is_partitioned_device device +  assert (String.is_prefix device
"/dev/");
+  assert (not (String.is_prefix device "/dev/mapper/"));
+  let device = String.sub device 5 (String.length device - 5) in
+
+  (* Open the device's directory under /sys/block *)
+  let parts = Sys.readdir ("/sys/block/" ^ device) in
+  let parts = Array.to_list parts in
+
+  (* Look in /sys/block/<device>/ for entries starting with
+   * <device>, eg. /sys/block/sda/sda1.
+   *)
+  List.exists (fun part -> String.is_prefix part device) parts
+
 let rec list_partitions ()    let partitions = map_block_devices
~return_md:true add_partitions in
   let partitions = List.flatten partitions in
diff --git a/daemon/devsparts.mli b/daemon/devsparts.mli
index 7b669c2..d3224a1 100644
--- a/daemon/devsparts.mli
+++ b/daemon/devsparts.mli
@@ -21,5 +21,6 @@ val list_partitions : unit -> string list
 val part_to_dev : string -> string
 val part_to_partnum : string -> int
 val is_whole_device : string -> bool
+val is_partitioned_device : string -> bool
 val nr_devices : unit -> int
 val device_index : string -> int
-- 
2.9.5

Filter partitioned md devices out the same way as partitioned physical devices
are filtered out
---
 daemon/listfs.ml | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 370ffb4..dc424f5 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -24,24 +24,20 @@ let rec list_filesystems ()    let has_lvm2 = Lvm.available
() in
   let has_ldm = Ldm.available () in
 
-  let devices = Devsparts.list_devices () in
   let partitions = Devsparts.list_partitions () in
-  let mds = Md.list_md_devices () in
 
   (* Look to see if any devices directly contain filesystems
-   * (RHBZ#590167).  However vfs-type will fail to tell us anything
+   * (RHBZ#590167). However vfs-type will fail to tell us anything
    * useful about devices which just contain partitions, so we also
-   * get the list of partitions and exclude the corresponding devices
-   * by using part-to-dev.
+   * get the list of partitions and exclude the corresponding devices.
    *)
-  let devices_containing_partitions = List.fold_left (
-    fun set part ->
-      StringSet.add (Devsparts.part_to_dev part) set
-  ) StringSet.empty partitions in
-  let devices = List.filter (
-    fun dev ->
-      not (StringSet.mem dev devices_containing_partitions)
-  ) devices in
+  let device_without_partitions device +    not
(Devsparts.is_partitioned_device device) in
+
+  let devices = Devsparts.list_devices () in
+  let devices = List.filter device_without_partitions devices in
+  let mds = Md.list_md_devices () in
+  let mds = List.filter device_without_partitions mds in
 
   (* Use vfs-type to check for filesystems on devices. *)
   let ret = List.filter_map check_with_vfs_type devices in
-- 
2.9.5

On Tue, Jan 23, 2018 at 12:44:10AM +0200, Mykola Ivanets
wrote:
> Before I rush to change something I request your comments on the subject.
> Let me know what do you think and if it does make sense.
> 
> The issue: guesfs_list_filesystems is inconsistent in its output.
The concept of guestfs_list_filesystem is that it returns a list
something like the list that running ‘df’ inside a traditional Unix
would return, ie. a list of mounted filesystems backed by real
devices, plus swap.

That's not an exact definition.  A better way to understand it might
be to look at how it is is used (ie. ‘git grep list[-_]filesystems’).
> For, example, it filters out partitioned physical devices but
> doesn't do the same for MD devices.
Yes, this is a bug.  I agree that the approach in your patches
(ie. filtering out explicitly partitioned devices) is better than the
current ad-hoc approach of listing whole devices, then partitioned
devices, then trying to remove whole devices uses a complicated
StringSet operation.
> More over, according to its name and API documentation
> guestfs_list_filesystem should return something which potentially
> can be mounted (SWAP is an exclusion and it is stated in API doc).
> It is the reason partitioned physical devices are filtered out: they
> are not mountable even in theory (they don't contain filesystem if
> they contain partition(s)).
Yes.
> Another example of non-mountable block device is extended MSDOS partition.
Do we return extended partitions?  I thought not ..
> Also nothing stops you from creating partition table and partitions
> on logical volume (yes, it is probably uncommon but
> nevertheless). In this case such partitions are ignored completely.
This is a bit of a corner case that we probably shouldn't spend too
much time on.  Most ordinary Linux guests would also ignore these
(they probably indicate nested VMs).
> Yet another example is Windows dynamic disks aka LDM:
> guestfs_list_filesystems returns both ldm_vol_ and ldm_part_ but
> strongly speaking only volumes should be considered as mountables.
I think this is a bug too.
> And last example is again with Windows dynamic disks and their
> physical counterparts: guestfs_list_filesystems filters out disks
> with MBR type byte 0x42 but what about GPT?
Again, likely to be a bug.

More comments in the patches ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

The test is fine, but it should come after the fix in the list of
commits, otherwise you break git bisection.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Tue, Jan 23, 2018 at 12:44:12AM +0200, Mykola Ivanets
wrote:
> The function takes one argument - device path (e.g. /dev/sda) and returns
true if device contains partition(s) or false otherwise. The function look in
/sys/block/<device>/ for entries starting with <device>, eg.
/sys/block/sda/sda1.
This is OK, but patches 2 & 3 should be combined.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Tue, Jan 23, 2018 at 12:44:13AM +0200, Mykola Ivanets
wrote:
> Filter partitioned md devices out the same way as partitioned physical
devices are filtered out
> ---
>  daemon/listfs.ml | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/daemon/listfs.ml b/daemon/listfs.ml
> index 370ffb4..dc424f5 100644
> --- a/daemon/listfs.ml
> +++ b/daemon/listfs.ml
> @@ -24,24 +24,20 @@ let rec list_filesystems () >    let has_lvm2 =
Lvm.available () in
>    let has_ldm = Ldm.available () in
>  
> -  let devices = Devsparts.list_devices () in
>    let partitions = Devsparts.list_partitions () in
> -  let mds = Md.list_md_devices () in
>  
>    (* Look to see if any devices directly contain filesystems
> -   * (RHBZ#590167).  However vfs-type will fail to tell us anything
> +   * (RHBZ#590167). However vfs-type will fail to tell us anything
This whitespace change is wrong.
>     * useful about devices which just contain partitions, so we also
> -   * get the list of partitions and exclude the corresponding devices
> -   * by using part-to-dev.
> +   * get the list of partitions and exclude the corresponding devices.
>     *)
> -  let devices_containing_partitions = List.fold_left (
> -    fun set part ->
> -      StringSet.add (Devsparts.part_to_dev part) set
> -  ) StringSet.empty partitions in
> -  let devices = List.filter (
> -    fun dev ->
> -      not (StringSet.mem dev devices_containing_partitions)
> -  ) devices in
> +  let device_without_partitions device > +    not
(Devsparts.is_partitioned_device device) in
> +
> +  let devices = Devsparts.list_devices () in
> +  let devices = List.filter device_without_partitions devices in
> +  let mds = Md.list_md_devices () in
> +  let mds = List.filter device_without_partitions mds in
Yes, this is better than the previous approach with sets.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

2018-01-23 12:11 GMT+02:00 Richard W.M. Jones
<rjones@redhat.com>:
> On Tue, Jan 23, 2018 at 12:44:10AM +0200, Mykola Ivanets wrote:
>> Before I rush to change something I request your comments on the
subject.
>> Let me know what do you think and if it does make sense.
>>
>> The issue: guesfs_list_filesystems is inconsistent in its output.
>
> The concept of guestfs_list_filesystem is that it returns a list
> something like the list that running ‘df’ inside a traditional Unix
> would return, ie. a list of mounted filesystems backed by real
> devices, plus swap.
>
> That's not an exact definition.  A better way to understand it might
> be to look at how it is is used (ie. ‘git grep list[-_]filesystems’).
Isn't more like 'lsblk' rather than 'df'?  Because
'df' is useless
with unmounted
file systems where is 'lsblk' discovers all available block devices.

--
    Mykola Ivanets