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

[Pino Toscano]

On Tuesday, 3 January 2017 11:18:53 CET Cédric Bosdonnat wrote:
> Getting checksum involves the same code than verifying them. Create
> a get_checksum function and use it in verify_checksum.
> ---

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.

> diff --git a/mllib/checksums.ml b/mllib/checksums.ml
> index dfa8c3ae7..3efc764b9 100644
> --- a/mllib/checksums.ml
> +++ b/mllib/checksums.ml
> @@ -45,23 +45,21 @@ let of_string csum_type csum_value =
>    | "sha512" -> SHA512 csum_value
>    | _ -> invalid_arg csum_type
>  
> -let verify_checksum csum filename =
> -  let prog, csum_ref =
> -    match csum with
> -    | SHA1 c -> "sha1sum", c
> -    | SHA256 c -> "sha256sum", c
> -    | SHA512 c -> "sha512sum", c
> -  in
> -
> +let get_checksum csum filename =
> +  let prog = (string_of_csum_t csum) ^ "sum" in

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170104/9d8f5c2e/attachment.sig>