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

[Nikolay Ivanets]

> 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.

Agree. Splitted this commit to a series of more granular commits.

> "check_device", "check_partition" are really generic names.  Name the
> functions according to what they do, eg.  "is_not_partitioned_device".

Agree.

> > +and is_ignored_gpt_type gpt_type =
> > +  match gpt_type with
>
> You can write this as:
>
>   and is_ignored_gpt_type = function
>   | pattern -> result
>   | ...

I've got rid of separate function at all.

> > +  (* 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 ()?

Yes, it shouldn't.

> > +  (* 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

Ah, thanks.

> 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.

Agree. There is no expected exceptions there.
I thought: If something went wrong - let's not filter out device or
partition.

v6 is coming.

--
  Mykola Ivanets
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20180501/1d89accb/attachment.htm>