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

[Richard W.M. Jones]

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