Libguestfs - May 2018 - [PATCH v6 0/7] daemon: list_filesystems: filter out block devices which cannot hold filesystem

This patch series:
1. Addresses comments from v5 series review
2. Large commit is splitted to more granular commits for better code review.

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-ilesystems: Filter out MBR extended partitions.

Nikolay Ivanets (1):
  daemon: Reimplement 'part_get_mbr_part_type' API in OCaml.

 daemon/listfs.ml                        | 127 +++++++++++++-----------
 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, 166 insertions(+), 163 deletions(-)
 create mode 100755 tests/md/test-partitioned-md-devices.sh

-- 
2.17.0

From: Nikolay Ivanets <stenavin at 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

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

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index 56ebadeda..55ace8e9c 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,23 @@ let rec list_filesystems ()  
   List.flatten ret
 
+(* Look to see if device can directly contain filesystem (RHBZ#590167).
+ * Partitioned devices cannot contain fileystem, so we will exclude such
devices.
+ *)
+and is_not_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
+    let has_partition = List.exists (fun part -> String.is_prefix part
device) parts in
+    not has_partition
+  with Sys_error (_) -> 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
-- 
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 55ace8e9c..4014e3fe9 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 4014e3fe9..0d71a39db 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
 
@@ -87,6 +77,33 @@ and is_not_partitioned_device device      let has_partition =
List.exists (fun part -> String.is_prefix part device) parts in
     not has_partition
   with Sys_error (_) -> true
+  
+(* 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
@@ -143,20 +160,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 0d71a39db..fa7813378 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. *)
@@ -82,9 +82,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
@@ -100,7 +101,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 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index fa7813378..2ed7da6e3 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -83,7 +83,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
@@ -106,10 +106,17 @@ and is_partition_can_hold_filesystem partition        |
"E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
       (* Windows Snapshot Partition. *)
       | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> false
-    | _ -> true
+    | _ -> not (is_mbr_extended parttype device partnum)
   )
   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

On Tue, May 01, 2018 at 03:53:59AM +0300, Mykola Ivanets
wrote:
> Instead of using part_to_dev to find such devices we open the device's
> directory under /sys/block/<device> and look for entries starting
with
> <device>, eg. /sys/block/sda/sda1.
> +(* Look to see if device can directly contain filesystem (RHBZ#590167).
> + * Partitioned devices cannot contain fileystem, so we will exclude such
devices.
> + *)
Let's keep lines to < 80 characters.
> +and is_not_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
> +    let has_partition = List.exists (fun part -> String.is_prefix part
device) parts in
Also this line is >= 80 characters, but could easily be changed
so it's shorter.
> +    not has_partition
> +  with Sys_error (_) -> true
This is the real problem.  What Sys_error is expected here?  As far as
I can tell if anything throws Sys_error (or any other exception) here
then it's a real bug, so we shouldn't hide the error message.  If
you're not expecting a particular Sys_error, then 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-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Tue, May 01, 2018 at 03:54:04AM +0300, Mykola Ivanets
wrote:
> Extended MBR partitions cannot hold filesystems - filter them out.
> ---
>  daemon/listfs.ml | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/daemon/listfs.ml b/daemon/listfs.ml
> index fa7813378..2ed7da6e3 100644
> --- a/daemon/listfs.ml
> +++ b/daemon/listfs.ml
> @@ -83,7 +83,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
> @@ -106,10 +106,17 @@ and is_partition_can_hold_filesystem partition >  
| "E3C9E316-0B5C-4DB8-817D-F92DF00215AE"
>        (* Windows Snapshot Partition. *)
>        | "CADDEBF1-4400-4DE8-B103-12117DCF3CCF" -> false
> -    | _ -> true
> +    | _ -> not (is_mbr_extended parttype device partnum)
This is a bit confusing.  I think the equivalent can be
achieved by changing the final function to:

  if is_gpt_or_mbr then (
    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

The other patches (where I didn't have comments) look OK to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v