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

[Cedric Bosdonnat]

On Tue, 2017-02-07 at 16:13 +0000, Richard W.M. Jones wrote:
> 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.

I only followed Pino's recommendations here:
https://www.redhat.com/archives/libguestfs/2017-January/msg00015.html

> > +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.

I'll check what I can do with it then.

--
Cedric