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

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". You can write this as: and is_ignored_gpt_type = function | pattern -> result | ... Why does the result depend on Optgroups.ldm_available ()? 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 [...] 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