Libguestfs - Jun 2018 - [PATCH v8 0/6] daemon: list_filesystems: filter out block devices which cannot hold filesystem.

v8: - Rebased on top of master.
v7: - Addresses comments after v6 series review.
v6: - Addresses comments after v5 series review.
    - Large commit is splitted to more granular commits for better code review.
v5: - Addresses comments after v4 series review (part_get_mbr_part_type
doesn't break original implementation in C).
    - Rebased on top of master and little bit refactored for readability.
v4: - Rebased on top of master.
v3: - Rebased on top of master.
v2: - First implementation.
v1: - Initial discussion.

This patch series filters out block devices which cannot hold filesystems:
- partitioned MD devices;
- LDM partitions (only LDM volume can hold filesystem);
- Windows Logical Disk Manager data partition;
- Microsoft Reserved Partition;
- Windows Snapshot Partition;
- MBR extended partition.

Mykola Ivanets (6):
  daemon: Changing the way that we detect if a device contains
    partitions.
  daemon: list-filesystems: Ignore partitioned MD devices.
  tests: list-filesystems command ignores partitioned MD devices.
  daemon: list-filesystems: Change the way we filter out LDM partitions.
  daemon: list-filesystems: Filter out Microsoft Reserved and Windows
    Snapshot partitions.
  daemon: list-filesystems: Filter out MBR extended partitions.

 daemon/listfs.ml                        | 132 ++++++++++++++----------
 tests/md/Makefile.am                    |   3 +-
 tests/md/test-partitioned-md-devices.sh |  79 ++++++++++++++
 3 files changed, 157 insertions(+), 57 deletions(-)
 create mode 100755 tests/md/test-partitioned-md-devices.sh

-- 
2.17.0

Instead of using part_to_dev to find such devices we open the device's
directory under /sys/block/<dev_name> and look for entries starting with
<dev_name>, eg. /sys/block/sda/sda1.
---
 daemon/listfs.ml | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 56ebadeda..eced55bce 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -24,31 +24,15 @@ let rec list_filesystems ()    let has_lvm2 =
Optgroups.lvm2_available () in
   let has_ldm = Optgroups.ldm_available () in
 
+  (* Devices. *)
   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
-   * 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.
-   *)
-  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
-
-  (* Use vfs-type to check for filesystems on devices. *)
+  let devices = List.filter is_not_partitioned_device devices in
   let ret = List.filter_map check_with_vfs_type devices in
 
   (* Use vfs-type to check for filesystems on partitions, but
    * ignore MBR partition type 42 used by LDM.
    *)
+  let partitions = Devsparts.list_partitions () in
   let ret      ret @
       List.filter_map (
@@ -60,6 +44,7 @@ let rec list_filesystems ()        ) partitions in
 
   (* Use vfs-type to check for filesystems on md devices. *)
+  let mds = Md.list_md_devices () in
   let ret = ret @ List.filter_map check_with_vfs_type mds in
 
   (* LVM. *)
@@ -85,6 +70,24 @@ let rec list_filesystems ()  
   List.flatten ret
 
+(* Look to see if device can directly contain filesystem (RHBZ#590167).
+ * Partitioned devices cannot contain filesystem, so we will exclude
+ * such devices.
+ *)
+and is_not_partitioned_device device +  assert (String.is_prefix device
"/dev/");
+  let dev_name = String.sub device 5 (String.length device - 5) in
+  let dev_dir = "/sys/block/" ^ dev_name in
+
+  (* Open the device's directory under /sys/block/<dev_name> and
+   * look for entries starting with <dev_name>, eg. /sys/block/sda/sda1
+   *)
+  let is_device_partition file = String.is_prefix file dev_name in
+  let files = Array.to_list (Sys.readdir dev_dir) in
+  let has_partition = List.exists is_device_partition files in
+
+  not has_partition
+
 (* Use vfs-type to check for a filesystem of some sort of [device].
  * Returns [Some [device, vfs_type; ...]] if found (there may be
  * multiple devices found in the case of btrfs), else [None] if nothing
-- 
2.17.0

Ignore partitioned MD devices the same way we ignore regular partitioned
devices because they cannot contain filesystems as well.
---
 daemon/listfs.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index eced55bce..10b1a8594 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -43,8 +43,9 @@ let rec list_filesystems ()              None               
(* ignore type 42 *)
       ) partitions in
 
-  (* Use vfs-type to check for filesystems on md devices. *)
+  (* MD. *)
   let mds = Md.list_md_devices () in
+  let mds = List.filter is_not_partitioned_device mds in
   let ret = ret @ List.filter_map check_with_vfs_type mds in
 
   (* LVM. *)
-- 
2.17.0

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

diff --git a/tests/md/Makefile.am b/tests/md/Makefile.am
index 42260af58..2e487568a 100644
--- a/tests/md/Makefile.am
+++ b/tests/md/Makefile.am
@@ -24,7 +24,8 @@ TESTS = \
 	test-list-md-devices.sh \
 	test-lvm-on-md-device.sh \
 	test-md-and-lvm-devices.sh \
-	test-mdadm.sh
+	test-mdadm.sh \
+	test-partitioned-md-devices.sh
 
 TESTS_ENVIRONMENT = $(top_builddir)/run --test
 
diff --git a/tests/md/test-partitioned-md-devices.sh
b/tests/md/test-partitioned-md-devices.sh
new file mode 100755
index 000000000..36b6d8a25
--- /dev/null
+++ b/tests/md/test-partitioned-md-devices.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.17.0

1. Now we use GPT partition type to filter out LDM partitions.
2. We also check for filesystems on LDM volumes because LDM partitions
doesn't contain filesystems (list_ldm_partitions is not called anymore).
3. Obvious repetitive comments are moved to a function description.
---
 daemon/listfs.ml | 70 ++++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 10b1a8594..a1ee38de3 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -20,6 +20,10 @@ open Printf
 
 open Std_utils
 
+(* Enumerate block devices (including MD, LVM, LDM and partitions) and use
+ * vfs-type to check for filesystems on devices.  Some block devices cannot
+ * contain filesystems, so we filter them out.
+ *)
 let rec list_filesystems ()    let has_lvm2 = Optgroups.lvm2_available () in
   let has_ldm = Optgroups.ldm_available () in
@@ -29,19 +33,10 @@ let rec list_filesystems ()    let devices = List.filter
is_not_partitioned_device devices in
   let ret = List.filter_map check_with_vfs_type devices in
 
-  (* Use vfs-type to check for filesystems on partitions, but
-   * ignore MBR partition type 42 used by LDM.
-   *)
+  (* Partitions. *)
   let partitions = Devsparts.list_partitions () in
-  let ret -    ret @
-      List.filter_map (
-        fun part ->
-          if not has_ldm || not (is_mbr_partition_type_42 part) then
-            check_with_vfs_type part
-          else
-            None                (* ignore type 42 *)
-      ) partitions in
+  let partitions = List.filter is_not_ldm_partition partitions in
+  let ret = ret @ List.filter_map check_with_vfs_type partitions in
 
   (* MD. *)
   let mds = Md.list_md_devices () in
@@ -52,7 +47,6 @@ let rec list_filesystems ()    let ret      if has_lvm2 then (
       let lvs = Lvm.lvs () in
-      (* Use vfs-type to check for filesystems on LVs. *)
       ret @ List.filter_map check_with_vfs_type lvs
     )
     else ret in
@@ -61,11 +55,7 @@ let rec list_filesystems ()    let ret      if has_ldm then (
       let ldmvols = Ldm.list_ldm_volumes () in
-      let ldmparts = Ldm.list_ldm_partitions () in
-      (* Use vfs-type to check for filesystems on Windows dynamic disks. *)
-      ret @
-        List.filter_map check_with_vfs_type ldmvols @
-        List.filter_map check_with_vfs_type ldmparts
+      ret @ List.filter_map check_with_vfs_type ldmvols
     )
     else ret in
 
@@ -88,6 +78,33 @@ and is_not_partitioned_device device    let has_partition =
List.exists is_device_partition files in
 
   not has_partition
+  
+(* We should ignore Windows Logical Disk Manager (LDM) partitions,
+ * because these are members of a Windows dynamic disk group.  Trying
+ * to read them will cause errors (RHBZ#887520).  Assuming that
+ * libguestfs was compiled with ldm support, we'll get the filesystems
+ * on these later.
+ *)
+and is_not_ldm_partition partition +  let device = Devsparts.part_to_dev
partition in
+  let partnum = Devsparts.part_to_partnum partition in
+  let parttype = Parted.part_get_parttype device in
+
+  let is_gpt = parttype = "gpt" in
+  let is_mbr = parttype = "msdos" in
+  let is_gpt_or_mbr = is_gpt || is_mbr in
+
+  if is_gpt_or_mbr then (
+    (* MBR partition id will be converted into corresponding GPT type. *)
+    let gpt_type = Parted.part_get_gpt_type device partnum in
+    match gpt_type with
+    (* Windows Logical Disk Manager metadata partition. *)
+    | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3"
+      (* Windows Logical Disk Manager data partition. *)
+      | "AF9B60A0-1431-4F62-BC68-3311714A69AD" -> false
+    | _ -> true
+  )
+  else true
 
 (* Use vfs-type to check for a filesystem of some sort of [device].
  * Returns [Some [device, vfs_type; ...]] if found (there may be
@@ -144,20 +161,3 @@ and check_with_vfs_type device  
   else
     Some [mountable, vfs_type]
-
-(* We should ignore partitions that have MBR type byte 0x42, because
- * these are members of a Windows dynamic disk group.  Trying to read
- * them will cause errors (RHBZ#887520).  Assuming that libguestfs was
- * compiled with ldm support, we'll get the filesystems on these later.
- *)
-and is_mbr_partition_type_42 partition -  try
-    let partnum = Devsparts.part_to_partnum partition in
-    let device = Devsparts.part_to_dev partition in
-    let mbr_id = Parted.part_get_mbr_id device partnum in
-    mbr_id = 0x42
-  with exn ->
-     if verbose () then
-       eprintf "is_mbr_partition_type_42: %s: %s\n"
-               partition (Printexc.to_string exn);
-     false
-- 
2.17.0

Filter out Microsoft Reserved Partition and Windows Snapshot Partition.
---
 daemon/listfs.ml | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index a1ee38de3..57f2f622d 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -35,7 +35,7 @@ let rec list_filesystems ()  
   (* Partitions. *)
   let partitions = Devsparts.list_partitions () in
-  let partitions = List.filter is_not_ldm_partition partitions in
+  let partitions = List.filter is_partition_can_hold_filesystem partitions in
   let ret = ret @ List.filter_map check_with_vfs_type partitions in
 
   (* MD. *)
@@ -83,9 +83,10 @@ and is_not_partitioned_device device   * because these are
members of a Windows dynamic disk group.  Trying
  * to read them will cause errors (RHBZ#887520).  Assuming that
  * libguestfs was compiled with ldm support, we'll get the filesystems
- * on these later.
+ * on these later.  We also ignore Microsoft Reserved Partition and
+ * Windows Snapshot Partition.
  *)
-and is_not_ldm_partition partition +and is_partition_can_hold_filesystem
partition    let device = Devsparts.part_to_dev partition in
   let partnum = Devsparts.part_to_partnum partition in
   let parttype = Parted.part_get_parttype device in
@@ -101,7 +102,11 @@ and is_not_ldm_partition partition      (* Windows Logical
Disk Manager metadata partition. *)
     | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3"
       (* Windows Logical Disk Manager data partition. *)
-      | "AF9B60A0-1431-4F62-BC68-3311714A69AD" -> false
+      | "AF9B60A0-1431-4F62-BC68-3311714A69AD"
+      (* Microsoft Reserved Partition. *)
+      | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
+      (* Windows Snapshot Partition. *)
+      | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> false
     | _ -> true
   )
   else true
-- 
2.17.0

Extended MBR partitions cannot hold filesystems - filter them out.
---
 daemon/listfs.ml | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 57f2f622d..908e59cc4 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -84,7 +84,7 @@ and is_not_partitioned_device device   * to read them will
cause errors (RHBZ#887520).  Assuming that
  * libguestfs was compiled with ldm support, we'll get the filesystems
  * on these later.  We also ignore Microsoft Reserved Partition and
- * Windows Snapshot Partition.
+ * Windows Snapshot Partition as well as MBR extended partitions.
  *)
 and is_partition_can_hold_filesystem partition    let device =
Devsparts.part_to_dev partition in
@@ -96,21 +96,32 @@ and is_partition_can_hold_filesystem partition    let
is_gpt_or_mbr = is_gpt || is_mbr in
 
   if is_gpt_or_mbr then (
-    (* MBR partition id will be converted into corresponding GPT type. *)
-    let gpt_type = Parted.part_get_gpt_type device partnum in
-    match gpt_type with
-    (* Windows Logical Disk Manager metadata partition. *)
-    | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3"
-      (* Windows Logical Disk Manager data partition. *)
-      | "AF9B60A0-1431-4F62-BC68-3311714A69AD"
-      (* Microsoft Reserved Partition. *)
-      | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
-      (* Windows Snapshot Partition. *)
-      | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> false
-    | _ -> true
+    if is_mbr_extended parttype device partnum then
+      false
+    else (
+      (* MBR partition id will be converted into corresponding GPT type. *)
+      let gpt_type = Parted.part_get_gpt_type device partnum in
+      match gpt_type with
+      (* Windows Logical Disk Manager metadata partition. *)
+      | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3"
+        (* Windows Logical Disk Manager data partition. *)
+        | "AF9B60A0-1431-4F62-BC68-3311714A69AD"
+        (* Microsoft Reserved Partition. *)
+        | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
+        (* Windows Snapshot Partition. *)
+        | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> false
+      | _ -> true
+    )
   )
   else true
 
+and is_mbr_extended parttype device partnum +  if parttype = "msdos"
then (
+    let mbr_type = Parted.part_get_mbr_part_type device partnum in
+    mbr_type = "extended"
+  )
+  else false
+
 (* Use vfs-type to check for a filesystem of some sort of [device].
  * Returns [Some [device, vfs_type; ...]] if found (there may be
  * multiple devices found in the case of btrfs), else [None] if nothing
-- 
2.17.0

This all looks good now.  If it passes the tests then I'll push it.

Thanks, 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