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

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.