Libguestfs - Mar 2018 - [PATCH v4 0/3] libguestfs: guestfs_list_filesystems: skip block devices which cannot hold file system

Initial discussion is here:
https://www.redhat.com/archives/libguestfs/2018-January/msg00188.html.
v2 was posted here:
https://www.redhat.com/archives/libguestfs/2018-January/msg00246.html.

v3 comparing to v2 is just a rebase with slightly changed commits comments.

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

1. The function is applicable for MBR partitions only (as noted in documentation
and as function name suggests).
2. An attempt to call the function for non-MBR partition fails with
"part_get_mbr_part_type can only be used on MBR Partitions" error and
NULL is returned.
3. MBR partition table can hold up to 4 "primary" partitions.
4. Partition with number >= 5 is logical partition.
5. Extnded partition number is <= 4 and has MBR id 0x05 or 0x0f
(http://thestarman.pcministry.com/asm/mbr/PartTypes.htm;
https://en.wikipedia.org/wiki/Partition_type).
---
 daemon/parted.c           | 106 ----------------------------------------------
 daemon/parted.ml          |  14 ++++++
 daemon/parted.mli         |   2 +
 generator/actions_core.ml |   1 +
 4 files changed, 17 insertions(+), 106 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index e5435b5..d67c6c5 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 ce8da8a..75d9d37 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -125,6 +125,20 @@ 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 id within the first 1024 cylinders.
+   * 0x0f - extended partition id beyond the first 1024 cylinders.
+   *)
+  match parttype, partnum, mbr_id with
+  | "msdos", (1|2|3|4), (0x05|0x0f) -> "extended"
+  | "msdos", (1|2|3|4), _ -> "primary"
+  | "msdos", _, _ -> "logical"
+  | _, _, _ ->
+     failwithf "part_get_mbr_part_type can only be used on MBR
Partitions"
+
 let part_set_gpt_attributes device partnum attributes    if partnum <= 0
then failwith "partition number must be >= 1";
 
diff --git a/daemon/parted.mli b/daemon/parted.mli
index d547f2f..0f59d29 100644
--- a/daemon/parted.mli
+++ b/daemon/parted.mli
@@ -28,6 +28,8 @@ val part_list : string -> partition list
 
 val part_get_parttype : string -> string
 
+val part_get_mbr_part_type : string -> int -> string
+
 val part_get_gpt_type : string -> int -> string
 val part_get_gpt_guid : string -> int -> string
 val part_get_gpt_attributes : string -> int -> int64
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 544cb6e..307e414 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.9.5

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).
---
 daemon/devsparts.ml  |  13 +++++++
 daemon/devsparts.mli |   1 +
 daemon/listfs.ml     | 105 +++++++++++++++++++++++++++------------------------
 3 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/daemon/devsparts.ml b/daemon/devsparts.ml
index 54108bb..42dc78d 100644
--- a/daemon/devsparts.ml
+++ b/daemon/devsparts.ml
@@ -71,6 +71,19 @@ 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/");
+  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
+
 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
diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 370ffb4..1349ae0 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -25,39 +25,18 @@ let rec list_filesystems ()    let has_ldm = 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,61 @@ 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
 
+(* 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 (Devsparts.is_partitioned_device device)
+
+(* 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 (MSR)
+ * and extended MBR partitions because they don't contain filesystem.
+ *)
+and check_partition partition +  try
+    (* Windows Logical Disk Manager metadata partition. *)
+    let gpt_ldm_metadata = "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3" in
+    (* Windows Logical Disk Manager data partition. *)
+    let gpt_ldm_data = "AF9B60A0-1431-4F62-BC68-3311714A69AD" in
+    (* Microsoft Reserved Partition. *)
+    let gpt_msr = "E3C9E316-0B5C-4DB8-817D-F92DF00215AE" in
+
+    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
+
+    (* MBR partition id will be converted into corresponding GPT type. *)
+    let gpt_type = Parted.part_get_gpt_type device partnum in
+
+    let is_ldm = gpt_type = gpt_ldm_metadata || gpt_type = gpt_ldm_data in
+    let is_ldm = Ldm.available () && (is_gpt || is_mbr) &&
is_ldm in
+    let is_msr = is_gpt && gpt_type = gpt_msr in
+
+    let mbr_type = Parted.part_get_mbr_part_type device partnum in
+    let is_extended = mbr_type = "extended" in
+
+    not (is_ldm || is_msr || is_extended)
+  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 +164,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.9.5

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 42260af..2e48756 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 0000000..36b6d8a
--- /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.9.5

On Sun, Jan 28, 2018 at 11:54:19PM +0200, Mykola Ivanets
wrote:
> Instead of parsing 'parted' output OCaml implementation relies on
the following facts:
> 
> 1. The function is applicable for MBR partitions only (as noted in
documentation and as function name suggests).
This might be how it's documented, but the implementation has a nod
towards gpt:
> -  for (i = 0, row = start;  row < end; ++i, ++row) {
> -    if (STREQ (parttype, "gpt")) {
> -      memcpy (temp_type, "primary", strlen
("primary"));
whereas the replacement deliberately breaks this case:
> +     failwithf "part_get_mbr_part_type can only be used on MBR
Partitions"
This change is too fundamental to go in just before the stable release
(1.38).  I'm going to release 1.38 real soon and then we can look at
this again.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Rebased.

The only thing left unclear is: "Would you like to document behavior for
non-MBR partitions
more clearly or keep previous implementation despite MBR partition type
doesn't make sense
for non-MBR partitions at all?"

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

1. The function is applicable for MBR partitions only (as noted in documentation
and as function name suggests).
2. An attempt to call the function for non-MBR partition fails with
"part_get_mbr_part_type can only be used on MBR Partitions" error and
NULL is returned.
3. MBR partition table can hold up to 4 "primary" partitions.
4. Partition with number >= 5 is logical partition.
5. Extnded partition number is <= 4 and has MBR id 0x05 or 0x0f
(http://thestarman.pcministry.com/asm/mbr/PartTypes.htm;
https://en.wikipedia.org/wiki/Partition_type).
---
 daemon/parted.c           | 106 ----------------------------------------------
 daemon/parted.ml          |  14 ++++++
 daemon/parted.mli         |   2 +
 generator/actions_core.ml |   1 +
 4 files changed, 17 insertions(+), 106 deletions(-)

diff --git a/daemon/parted.c b/daemon/parted.c
index e5435b5..d67c6c5 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 ce8da8a..75d9d37 100644
--- a/daemon/parted.ml
+++ b/daemon/parted.ml
@@ -125,6 +125,20 @@ 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 id within the first 1024 cylinders.
+   * 0x0f - extended partition id beyond the first 1024 cylinders.
+   *)
+  match parttype, partnum, mbr_id with
+  | "msdos", (1|2|3|4), (0x05|0x0f) -> "extended"
+  | "msdos", (1|2|3|4), _ -> "primary"
+  | "msdos", _, _ -> "logical"
+  | _, _, _ ->
+     failwithf "part_get_mbr_part_type can only be used on MBR
Partitions"
+
 let part_set_gpt_attributes device partnum attributes    if partnum <= 0
then failwith "partition number must be >= 1";
 
diff --git a/daemon/parted.mli b/daemon/parted.mli
index d547f2f..0f59d29 100644
--- a/daemon/parted.mli
+++ b/daemon/parted.mli
@@ -28,6 +28,8 @@ val part_list : string -> partition list
 
 val part_get_parttype : string -> string
 
+val part_get_mbr_part_type : string -> int -> string
+
 val part_get_gpt_type : string -> int -> string
 val part_get_gpt_guid : string -> int -> string
 val part_get_gpt_attributes : string -> int -> int64
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 544cb6e..307e414 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.9.5

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).
---
 daemon/devsparts.ml  |  13 +++++++
 daemon/devsparts.mli |   1 +
 daemon/listfs.ml     | 105 +++++++++++++++++++++++++++------------------------
 3 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/daemon/devsparts.ml b/daemon/devsparts.ml
index 54108bb..42dc78d 100644
--- a/daemon/devsparts.ml
+++ b/daemon/devsparts.ml
@@ -71,6 +71,19 @@ 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/");
+  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
+
 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
diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 370ffb4..1349ae0 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -25,39 +25,18 @@ let rec list_filesystems ()    let has_ldm = 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,61 @@ 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
 
+(* 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 (Devsparts.is_partitioned_device device)
+
+(* 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 (MSR)
+ * and extended MBR partitions because they don't contain filesystem.
+ *)
+and check_partition partition +  try
+    (* Windows Logical Disk Manager metadata partition. *)
+    let gpt_ldm_metadata = "5808C8AA-7E8F-42E0-85D2-E1E90434CFB3" in
+    (* Windows Logical Disk Manager data partition. *)
+    let gpt_ldm_data = "AF9B60A0-1431-4F62-BC68-3311714A69AD" in
+    (* Microsoft Reserved Partition. *)
+    let gpt_msr = "E3C9E316-0B5C-4DB8-817D-F92DF00215AE" in
+
+    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
+
+    (* MBR partition id will be converted into corresponding GPT type. *)
+    let gpt_type = Parted.part_get_gpt_type device partnum in
+
+    let is_ldm = gpt_type = gpt_ldm_metadata || gpt_type = gpt_ldm_data in
+    let is_ldm = Ldm.available () && (is_gpt || is_mbr) &&
is_ldm in
+    let is_msr = is_gpt && gpt_type = gpt_msr in
+
+    let mbr_type = Parted.part_get_mbr_part_type device partnum in
+    let is_extended = mbr_type = "extended" in
+
+    not (is_ldm || is_msr || is_extended)
+  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 +164,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.9.5

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 42260af..2e48756 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 0000000..36b6d8a
--- /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.9.5

On Mon, Mar 12, 2018 at 09:01:57AM +0200, Mykola Ivanets
wrote:
> Rebased.
> 
> The only thing left unclear is: "Would you like to document behavior
for non-MBR partitions
> more clearly or keep previous implementation despite MBR partition type
doesn't make sense
> for non-MBR partitions at all?"
https://gb.redhat.com/archives/libguestfs/2018-February/msg00027.html

I don't agree that the existing implementation doesn't make sense for
GPT.  It returns "primary", but the reimplementation returns an error.
That's going to break callers who weren't careful about only using
this API on MBR, and I think that is unnecessary breakage.  GPT
partitions are all "primary".

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org