11:47 p.m.
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
11:47 p.m.
New subject: [PATCH v8 1/6] daemon: Changing the way that we detect if a device contains partitions.
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
11:47 p.m.
New subject: [PATCH v8 2/6] daemon: list-filesystems: Ignore partitioned MD devices.
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
11:47 p.m.
New subject: [PATCH v8 3/6] tests: list-filesystems command ignores partitioned MD devices.
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
11:47 p.m.
New subject: [PATCH v8 4/6] daemon: list-filesystems: Change the way we filter out LDM partitions.
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
11:47 p.m.
New subject: [PATCH v8 5/6] daemon: list-filesystems: Filter out Microsoft Reserved and Windows Snapshot partitions.
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
11:47 p.m.
New subject: [PATCH v8 6/6] daemon: list-filesystems: Filter out MBR extended partitions.
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
7:47 a.m.
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
2335
days inactive
2335
days old
7 comments
2 participants
participants (2)
-
Mykola Ivanets -
Richard W.M. Jones