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