Libguestfs - Apr 2018 - Re: [PATCH v5 2/3] daemon: list-filesystems: Don't list partitions which cannot hold file system.

This patch series:
1. Addresses comments from last review:
     part_get_mbr_part_type doesn't break original implementation in C.
2. Rebased on top of master and little bit refactored for readability.

Mykola Ivanets (1):
  tests: md: Test guestfish list-filesystems command skips partitioned
    md devices.

Nikolay Ivanets (2):
  daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.
  daemon: list-filesystems: Don't list partitions which cannot hold file
    system.

 daemon/listfs.ml                        | 130 +++++++++++++++---------
 daemon/parted.c                         | 106 -------------------
 daemon/parted.ml                        |  13 +++
 generator/actions_core.ml               |   1 +
 tests/md/Makefile.am                    |   3 +-
 tests/md/test-partitioned-md-devices.sh |  79 ++++++++++++++
 6 files changed, 176 insertions(+), 156 deletions(-)
 create mode 100755 tests/md/test-partitioned-md-devices.sh

-- 
2.17.0

From: Nikolay Ivanets <stenavin@gmail.com>

Instead of parsing 'parted' output OCaml implementation relies on the
following facts:

1. MBR partition table can hold up to 4 "primary" partitions.
2. Partition with number greater then 4 is "logical" partition.
3. Partition with number less then or equal to 4 with MBR ID 0x05 or 0x0f is
"extended" partition
(http://thestarman.pcministry.com/asm/mbr/PartTypes.htm;
https://en.wikipedia.org/wiki/Partition_type) or "primary" otherwise.
---
 daemon/parted.c           | 106 --------------------------------------
 daemon/parted.ml          |  13 +++++
 generator/actions_core.ml |   1 +
 3 files changed, 14 insertions(+), 106 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index e5435b5ea..d67c6c507 100644
--- a/daemon/parted.c
+++ b/daemon/parted.c
@@ -602,112 +602,6 @@ do_part_get_name (const char *device, int partnum)
   }
 }
 
-char *
-do_part_get_mbr_part_type (const char *device, int partnum)
-{
-  CLEANUP_FREE char *parttype;
-  char *part_type;
-
-  parttype = do_part_get_parttype (device);
-  if (parttype == NULL)
-    return NULL;
-
-  /* machine parseable output by 'parted -m' did not provide
-   * partition type info.
-   * Use traditional style.
-   */
-  CLEANUP_FREE char *out = print_partition_table (device, false);
-  if (!out)
-    return NULL;
-
-  CLEANUP_FREE_STRING_LIST char **lines = split_lines (out);
-
-  if (!lines)
-    return NULL;
-
-  size_t start = 0, end = 0, row;
-
-  for (row = 0; lines[row] != NULL; ++row)
-    if (STRPREFIX (lines[row], "Number")) {
-      start = row + 1;
-      break;
-    }
-
-  if (start == 0) {
-    reply_with_error ("parted output has no \"Number\"
line");
-    return NULL;
-  }
-
-  for (row = start; lines[row] != NULL; ++row)
-    if (STREQ (lines[row], "")) {
-      end = row;
-      break;
-    }
-
-  if (end == 0) {
-    reply_with_error ("parted output has no blank after end of
table");
-    return NULL;
-  }
-
-  /* Now parse the lines. */
-  size_t i;
-  int64_t temp_int64;
-  int part_num;
-  char temp_type[16] = {'\0'};
-  for (i = 0, row = start;  row < end; ++i, ++row) {
-    if (STREQ (parttype, "gpt")) {
-      memcpy (temp_type, "primary", strlen ("primary"));
-      if (sscanf (lines[row], "%d%" SCNi64 "B%" SCNi64
"B%" SCNi64 "B",
-		  &part_num,
-		  &temp_int64,
-		  &temp_int64,
-		  &temp_int64) != 4) {
-        reply_with_error ("could not parse row from output of parted print
command: %s", lines[row]);
-        return NULL;
-      }
-    } else {
-      if (sscanf (lines[row], "%d%" SCNi64 "B%" SCNi64
"B%" SCNi64 "B" "%15s",
-		  &part_num,
-		  &temp_int64,
-		  &temp_int64,
-		  &temp_int64,
-		  temp_type) != 5) {
-        reply_with_error ("could not parse row from output of parted print
command: %s", lines[row]);
-        return NULL;
-      }
-    }
-
-    if (part_num != partnum)
-      continue;
-
-    if (STRPREFIX (temp_type, "primary")) {
-      part_type = strdup ("primary");
-      if (part_type == NULL)
-	goto error;
-    } else if (STRPREFIX (temp_type, "logical")) {
-      part_type = strdup ("logical");
-      if (part_type == NULL)
-	goto error;
-    } else if (STRPREFIX (temp_type, "extended")) {
-      part_type = strdup ("extended");
-      if (part_type == NULL)
-	goto error;
-    } else
-      goto error;
-
-    return part_type;
-  }
-
-  if (row == end) {
-    reply_with_error ("could not find partnum: %d", partnum);
-    return NULL;
-  }
-
- error:
-  reply_with_error ("strdup failed");
-  return NULL;
-}
-
 static char *
 extract_uuid (const char *value)
 {
diff --git a/daemon/parted.ml b/daemon/parted.ml
index ce8da8a60..42ccb84c5 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -125,6 +125,19 @@ let part_get_parttype device    | _ ->
      failwithf "%s: cannot parse the output of parted" device
 
+let part_get_mbr_part_type device partnum +  let parttype = part_get_parttype
device in
+  let mbr_id = part_get_mbr_id device partnum in
+
+  (* 0x05 - extended partition.
+   * 0x0f - extended partition using BIOS INT 13h extensions.
+   *)
+  match parttype, partnum, mbr_id with
+  | "msdos", (1|2|3|4), (0x05|0x0f) -> "extended"
+  | "msdos", (1|2|3|4), _ -> "primary"
+  | "msdos", _, _ -> "logical"
+  | _, _, _ -> "primary"
+
 let part_set_gpt_attributes device partnum attributes    if partnum <= 0
then failwith "partition number must be >= 1";
 
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 394576398..a049159ec 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -9213,6 +9213,7 @@ All data will be zeroed, but metadata and the like is
preserved." };
   { defaults with
     name = "part_get_mbr_part_type"; added = (1, 29, 32);
     style = RString (RPlainString, "partitiontype"), [String (Device,
"device"); Int "partnum"], [];
+    impl = OCaml "Parted.part_get_mbr_part_type";
     tests = [
       InitEmpty, Always, TestResultString (
         [["part_init"; "/dev/sda"; "mbr"];
-- 
2.17.0

From: Nikolay Ivanets <stenavin@gmail.com>

The following partitions are filtered out:
1. Partitioned md devices (just as partitioned physical devices are filtered
out).
2. Extended MBR partitions.
3. LDM Partitions (MBR and GPT partitions used by Windows Logical Disk Manager).
4. Microsoft Reserved Partitions (GUID E3C9E316-0B5C-4DB8-817D-F92DF00215AE).
5. Windows Snapshot Partitions (GUID CADDEBF1-4400-4DE8-B103-12117DCF3CCF).
---
 daemon/listfs.ml | 130 +++++++++++++++++++++++++++++------------------
 1 file changed, 81 insertions(+), 49 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 56ebadeda..5499bacb2 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -25,39 +25,18 @@ let rec list_filesystems ()    let has_ldm =
Optgroups.ldm_available () in
 
   let devices = Devsparts.list_devices () in
-  let partitions = Devsparts.list_partitions () in
+  let devices = List.filter check_device devices 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
+  let mds = List.filter check_device mds in
 
   (* Use vfs-type to check for filesystems on devices. *)
   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 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 = Devsparts.list_partitions () in
+  let partitions = List.filter check_partition partitions in
+
+  (* Use vfs-type to check for filesystems on partitions. *)
+  let ret = ret @ List.filter_map check_with_vfs_type partitions in
 
   (* Use vfs-type to check for filesystems on md devices. *)
   let ret = ret @ List.filter_map check_with_vfs_type mds in
@@ -75,16 +54,86 @@ 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
 
   List.flatten ret
 
+and is_partitioned_device device = 
+  assert (String.is_prefix device "/dev/"); 
+  let device = String.sub device 5 (String.length device - 5) in 
+  let dir = "/sys/block/" ^ device in 
+ 
+  try 
+    (* Open the device's directory under /sys/block/<device> and 
+     * look for entries starting with <device>, eg. /sys/block/sda/sda1 
+     *) 
+    let parts = Array.to_list (Sys.readdir dir) in 
+    List.exists (fun part -> String.is_prefix part device) parts 
+  with Sys_error (_) -> false
+
+(* 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
+ * exclude such devices.
+ *)
+and check_device device +  not (is_partitioned_device device)
+
+and is_ignored_gpt_type gpt_type +  match gpt_type with
+  (* Windows Logical Disk Manager metadata partition. *)
+  | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3" ->
Optgroups.ldm_available ()
+  (* Windows Logical Disk Manager data partition. *)
+  | "AF9B60A0-1431-4F62-BC68-3311714A69AD" ->
Optgroups.ldm_available ()
+  (* Microsoft Reserved Partition. *)
+  | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE" -> true
+  (* Windows Snapshot Partition. *)
+  | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> true
+  | _ -> false
+  
+(* 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.  We also ignores Microsoft Reserved Partitions,
+ * Windows Snapshot Partition and extended MBR partitions because they
+ * cannot hold filesystem.
+ *)
+and check_partition partition +  try
+    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
+
+    let is_mbr_extended +      if is_mbr then (
+        let mbr_type = Parted.part_get_mbr_part_type device partnum in
+        mbr_type = "extended"
+      )
+      else false in
+
+    let is_gpt_ignored +      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
+        is_ignored_gpt_type gpt_type
+      )
+      else false in
+    
+    not (is_mbr_extended || is_gpt_ignored)
+  with exn ->
+        if verbose () then
+          eprintf "check_partition: %s: %s\n"
+            partition (Printexc.to_string exn);
+        true
+
 (* 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
@@ -140,20 +189,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

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

This commit is really hard to follow.  I wonder if it can be broken up
to make it easier to review.  There seem to be at least two parts to
the patch: (1) Changing the way that we detect if a device contains
partitions.  (2) Changing the way that we filter out LDM partitions.

Some other comments:

"check_device", "check_partition" are really generic names. 
Name the
functions according to what they do, eg.  "is_not_partitioned_device".
> +and is_ignored_gpt_type gpt_type > +  match gpt_type with
You can write this as:

  and is_ignored_gpt_type = function
  | pattern -> result
  | ...
> +  (* Windows Logical Disk Manager metadata partition. *)
> +  | "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3" ->
Optgroups.ldm_available ()
> +  (* Windows Logical Disk Manager data partition. *)
> +  | "AF9B60A0-1431-4F62-BC68-3311714A69AD" ->
Optgroups.ldm_available ()
Why does the result depend on Optgroups.ldm_available ()?
> +  (* Microsoft Reserved Partition. *)
> +  | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE" -> true
> +  (* Windows Snapshot Partition. *)
> +  | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> true
You can combine the right hand sides of multiple matches, which helps
the pattern match optimizer in the compiler, eg:

  (* Microsoft Reserved Partition. *)
  | "E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
  (* Windows Snapshot Partition. *)
  | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> true
  | _ -> false
> +and check_partition partition > +  try
[...]
> +  with exn ->
Catching the generic exception is usually a big red flag.  What
exceptions are you expecting here?  If you're not expecting an
exception, don't try to catch anything.

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