Re: [Libguestfs] [PATCH 2/5] mllib: factorize code to add Checksum.get_checksum function

On Tuesday, 3 January 2017 11:18:53 CET Cédric Bosdonnat wrote: The idea is good, although there are few issues: - you need to rebase the patch on master, since the code changed and this patch will not apply - I'd call the function as 'calculate_checksum', as fits more what it does - I don't like much the usage of csum_t as parameter, since it's a bit misleading to pass a checksum string which will be ignored; one idea to avoid duplicating code with no need to refactor much could be still use your approach internally of using a csum_t, constructing that from the string of a checksum type -- something like: let rec calculate_checksum csum_type filename = do_calculate_checksum (of_string csum_type "") and do_calculate_checksum csum filename = (* rest of your code *) calculate_checksum would be the public API (exported), while do_calculate_checksum the private implementation used by calculate_checksum and verify_checksum. Also, of_string already has the string -> csum_t mapping, including raising an error on unknown type. I'd leave the way prog is set as it is, as it is more explicit, and will be easier to adapt in the future when I get to make this code work on non-GNU platforms (e.g. FreeBSD). Thanks, -- Pino Toscano