> 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
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
