[Libguestfs] [PATCH v2 1/7] mllib: factorize code to add Checksum.get_checksum function

[Richard W.M. Jones]

On Tue, Feb 07, 2017 at 04:14:16PM +0100, Cédric Bosdonnat wrote:
> Getting checksum involves the same code than verifying them. Create
> a get_checksum function and use it in verify_checksum.
> ---
>  mllib/checksums.ml  | 25 ++++++++++++++++---------
>  mllib/checksums.mli |  9 +++++++++
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/mllib/checksums.ml b/mllib/checksums.ml
> index 1009e131c..bee829085 100644
> --- a/mllib/checksums.ml
> +++ b/mllib/checksums.ml
> @@ -45,14 +45,13 @@ let of_string csum_type csum_value =
>    | "sha512" -> SHA512 csum_value
>    | _ -> invalid_arg csum_type
>  
> -let verify_checksum csum ?tar filename =
> -  let prog, csum_ref =
> +let do_compute_checksum csum ?tar filename =
> +  let prog =
>      match csum with
> -    | SHA1 c -> "sha1sum", c
> -    | SHA256 c -> "sha256sum", c
> -    | SHA512 c -> "sha512sum", c
> +    | SHA1 _ -> "sha1sum"
> +    | SHA256 _ -> "sha256sum"
> +    | SHA512 _ -> "sha512sum"
>    in
> -
>    let cmd =
>      match tar with
>      | None ->
> @@ -66,9 +65,17 @@ let verify_checksum csum ?tar filename =
>    | [] ->
>      error (f_"%s did not return any output") prog
>    | line :: _ ->
> -    let csum_actual = fst (String.split " " line) in
> -    if csum_ref <> csum_actual then
> -      raise (Mismatched_checksum (csum, csum_actual))
> +    fst (String.split " " line)
> +
> +let compute_checksum csum_type ?tar filename =
> +  do_compute_checksum (of_string csum_type "") ?tar filename

Why is there a separate do_compute_checksum function here?  It seems
like it is only called from here, not from anywhere else.

> +let verify_checksum csum ?tar filename =
> +  let csum_ref = string_of_csum csum in
> +  let csum_type = string_of_csum_t csum in
> +  let csum_actual = compute_checksum csum_type ?tar filename in
> +  if csum_ref <> csum_actual then
> +    raise (Mismatched_checksum (csum, csum_actual))

This is technically correct because all the checksum types happen to
be different lengths, but really wrong.  You should be comparing the
structured csum_t values, not just the strings.

Unfortunately the original verify_checksum is also broken in the same
way so this isn't a bug you've added, but it's a bug that needs to be
fixed.

I think the real problem is that compute_checksum should return a
structured csum_t, not a string, and then everything else follows
elegantly from that.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org