From 31047c8113563880290a21d4dad8bc375d934310 Mon Sep 17 00:00:00 2001
From: Aaron Lehmann <aaron.lehmann@docker.com>
Date: Mon, 14 Dec 2015 14:30:51 -0800
Subject: [PATCH] Simplify digest.FromBytes calling convention

The current implementation of digest.FromBytes returns an error. This
error can never be non-nil, but its presence in the function signature
means each call site needs error handling code for an error that is
always nil.

I verified that none of the hash.Hash implementations in the standard
library can return an error on Write. Nor can any of the hash.Hash
implementations vendored in distribution.

This commit changes digest.FromBytes not to return an error. If Write
returns an error, it will panic, but as discussed above, this should
never happen.

This commit also avoids using a bytes.Reader to feed data into the hash
function in FromBytes. This makes the hypothetical case that would panic
a bit more explicit, and should also be more performant.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
---
 digest/digest.go                          | 16 +++++++++++++---
 digest/verifiers_test.go                  |  5 +----
 notifications/bridge.go                   |  5 +----
 notifications/bridge_test.go              |  5 +----
 notifications/listener_test.go            |  6 +-----
 registry/client/repository_test.go        | 16 +++-------------
 registry/handlers/api_test.go             |  6 ++----
 registry/handlers/images.go               |  8 +-------
 registry/proxy/proxyblobstore_test.go     | 10 ++--------
 registry/proxy/proxymanifeststore.go      |  7 +------
 registry/proxy/proxymanifeststore_test.go |  2 +-
 registry/storage/blob_test.go             |  5 +----
 registry/storage/blobstore.go             |  7 +------
 registry/storage/linkedblobstore.go       |  5 +----
 registry/storage/manifeststore_test.go    |  6 +-----
 15 files changed, 31 insertions(+), 78 deletions(-)

diff --git a/digest/digest.go b/digest/digest.go
index ae581f15..5961db2c 100644
--- a/digest/digest.go
+++ b/digest/digest.go
@@ -1,7 +1,6 @@
 package digest
 
 import (
-	"bytes"
 	"fmt"
 	"hash"
 	"io"
@@ -99,8 +98,19 @@ func FromTarArchive(rd io.Reader) (Digest, error) {
 }
 
 // FromBytes digests the input and returns a Digest.
-func FromBytes(p []byte) (Digest, error) {
-	return FromReader(bytes.NewReader(p))
+func FromBytes(p []byte) Digest {
+	digester := Canonical.New()
+
+	if _, err := digester.Hash().Write(p); err != nil {
+		// Writes to a Hash should never fail. None of the existing
+		// hash implementations in the stdlib or hashes vendored
+		// here can return errors from Write. Having a panic in this
+		// condition instead of having FromBytes return an error value
+		// avoids unnecessary error handling paths in all callers.
+		panic("write to hash function returned error: " + err.Error())
+	}
+
+	return digester.Digest()
 }
 
 // Validate checks that the contents of d is a valid digest, returning an
diff --git a/digest/verifiers_test.go b/digest/verifiers_test.go
index 5ee79f34..6e672df1 100644
--- a/digest/verifiers_test.go
+++ b/digest/verifiers_test.go
@@ -15,10 +15,7 @@ import (
 func TestDigestVerifier(t *testing.T) {
 	p := make([]byte, 1<<20)
 	rand.Read(p)
-	digest, err := FromBytes(p)
-	if err != nil {
-		t.Fatalf("unexpected error digesting bytes: %#v", err)
-	}
+	digest := FromBytes(p)
 
 	verifier, err := NewDigestVerifier(digest)
 	if err != nil {
diff --git a/notifications/bridge.go b/notifications/bridge.go
index d4a3e1f6..c33a9ff1 100644
--- a/notifications/bridge.go
+++ b/notifications/bridge.go
@@ -98,10 +98,7 @@ func (b *bridge) createManifestEvent(action string, repo string, sm *schema1.Sig
 
 	event.Target.Length = int64(len(p))
 	event.Target.Size = int64(len(p))
-	event.Target.Digest, err = digest.FromBytes(p)
-	if err != nil {
-		return nil, err
-	}
+	event.Target.Digest = digest.FromBytes(p)
 
 	event.Target.URL, err = b.ub.BuildManifestURL(sm.Name, event.Target.Digest.String())
 	if err != nil {
diff --git a/notifications/bridge_test.go b/notifications/bridge_test.go
index a291acb7..1c82d317 100644
--- a/notifications/bridge_test.go
+++ b/notifications/bridge_test.go
@@ -90,10 +90,7 @@ func createTestEnv(t *testing.T, fn testSinkFn) Listener {
 		t.Fatalf("error getting manifest payload: %v", err)
 	}
 
-	dgst, err = digest.FromBytes(payload)
-	if err != nil {
-		t.Fatalf("error digesting manifest payload: %v", err)
-	}
+	dgst = digest.FromBytes(payload)
 
 	return NewBridge(ub, source, actor, request, fn)
 }
diff --git a/notifications/listener_test.go b/notifications/listener_test.go
index 04dae118..d8804fcb 100644
--- a/notifications/listener_test.go
+++ b/notifications/listener_test.go
@@ -167,11 +167,7 @@ func checkExerciseRepository(t *testing.T, repository distribution.Repository) {
 		t.Fatalf("unexpected error getting manifest payload: %v", err)
 	}
 
-	dgst, err := digest.FromBytes(p)
-	if err != nil {
-		t.Fatalf("unexpected error digesting manifest payload: %v", err)
-	}
-
+	dgst := digest.FromBytes(p)
 	fetchedByManifest, err := manifests.Get(dgst)
 	if err != nil {
 		t.Fatalf("unexpected error fetching manifest: %v", err)
diff --git a/registry/client/repository_test.go b/registry/client/repository_test.go
index 058947de..a001b62f 100644
--- a/registry/client/repository_test.go
+++ b/registry/client/repository_test.go
@@ -38,12 +38,7 @@ func newRandomBlob(size int) (digest.Digest, []byte) {
 		panic("unable to read enough bytes")
 	}
 
-	dgst, err := digest.FromBytes(b)
-	if err != nil {
-		panic(err)
-	}
-
-	return dgst, b
+	return digest.FromBytes(b), b
 }
 
 func addTestFetch(repo string, dgst digest.Digest, content []byte, m *testutil.RequestResponseMap) {
@@ -509,16 +504,11 @@ func newRandomSchemaV1Manifest(name, tag string, blobCount int) (*schema1.Signed
 		panic(err)
 	}
 
-	dgst, err := digest.FromBytes(p)
-	if err != nil {
-		panic(err)
-	}
-
-	return sm, dgst, p
+	return sm, digest.FromBytes(p), p
 }
 
 func addTestManifestWithEtag(repo, reference string, content []byte, m *testutil.RequestResponseMap, dgst string) {
-	actualDigest, _ := digest.FromBytes(content)
+	actualDigest := digest.FromBytes(content)
 	getReqWithEtag := testutil.Request{
 		Method: "GET",
 		Route:  "/v2/" + repo + "/manifests/" + reference,
diff --git a/registry/handlers/api_test.go b/registry/handlers/api_test.go
index 7f52d13d..8dbec0fe 100644
--- a/registry/handlers/api_test.go
+++ b/registry/handlers/api_test.go
@@ -880,8 +880,7 @@ func testManifestAPI(t *testing.T, env *testEnv, args manifestArgs) (*testEnv, m
 	payload, err := signedManifest.Payload()
 	checkErr(t, err, "getting manifest payload")
 
-	dgst, err := digest.FromBytes(payload)
-	checkErr(t, err, "digesting manifest")
+	dgst := digest.FromBytes(payload)
 
 	args.signedManifest = signedManifest
 	args.dgst = dgst
@@ -1487,8 +1486,7 @@ func createRepository(env *testEnv, t *testing.T, imageName string, tag string)
 	payload, err := signedManifest.Payload()
 	checkErr(t, err, "getting manifest payload")
 
-	dgst, err := digest.FromBytes(payload)
-	checkErr(t, err, "digesting manifest")
+	dgst := digest.FromBytes(payload)
 
 	manifestDigestURL, err := env.builder.BuildManifestURL(imageName, dgst.String())
 	checkErr(t, err, "building manifest url")
diff --git a/registry/handlers/images.go b/registry/handlers/images.go
index d30fce26..2ec51b99 100644
--- a/registry/handlers/images.go
+++ b/registry/handlers/images.go
@@ -250,11 +250,5 @@ func digestManifest(ctx context.Context, sm *schema1.SignedManifest) (digest.Dig
 		p = sm.Raw
 	}
 
-	dgst, err := digest.FromBytes(p)
-	if err != nil {
-		ctxu.GetLogger(ctx).Errorf("error digesting manifest: %v", err)
-		return "", err
-	}
-
-	return dgst, err
+	return digest.FromBytes(p), nil
 }
diff --git a/registry/proxy/proxyblobstore_test.go b/registry/proxy/proxyblobstore_test.go
index a88fd8b3..eb623197 100644
--- a/registry/proxy/proxyblobstore_test.go
+++ b/registry/proxy/proxyblobstore_test.go
@@ -298,10 +298,7 @@ func testProxyStoreServe(t *testing.T, te *testEnv, numClients int) {
 				}
 
 				bodyBytes := w.Body.Bytes()
-				localDigest, err := digest.FromBytes(bodyBytes)
-				if err != nil {
-					t.Fatalf("Error making digest from blob")
-				}
+				localDigest := digest.FromBytes(bodyBytes)
 				if localDigest != remoteBlob.Digest {
 					t.Fatalf("Mismatching blob fetch from proxy")
 				}
@@ -335,10 +332,7 @@ func testProxyStoreServe(t *testing.T, te *testEnv, numClients int) {
 			t.Fatalf(err.Error())
 		}
 
-		dl, err := digest.FromBytes(w.Body.Bytes())
-		if err != nil {
-			t.Fatalf("Error making digest from blob")
-		}
+		dl := digest.FromBytes(w.Body.Bytes())
 		if dl != dr.Digest {
 			t.Errorf("Mismatching blob fetch from proxy")
 		}
diff --git a/registry/proxy/proxymanifeststore.go b/registry/proxy/proxymanifeststore.go
index 610d695e..1e9e24de 100644
--- a/registry/proxy/proxymanifeststore.go
+++ b/registry/proxy/proxymanifeststore.go
@@ -137,12 +137,7 @@ func manifestDigest(sm *schema1.SignedManifest) (digest.Digest, error) {
 
 	}
 
-	dgst, err := digest.FromBytes(payload)
-	if err != nil {
-		return "", err
-	}
-
-	return dgst, nil
+	return digest.FromBytes(payload), nil
 }
 
 func (pms proxyManifestStore) Put(manifest *schema1.SignedManifest) error {
diff --git a/registry/proxy/proxymanifeststore_test.go b/registry/proxy/proxymanifeststore_test.go
index 6e0fc51e..a5a0a21b 100644
--- a/registry/proxy/proxymanifeststore_test.go
+++ b/registry/proxy/proxymanifeststore_test.go
@@ -177,7 +177,7 @@ func populateRepo(t *testing.T, ctx context.Context, repository distribution.Rep
 	if err != nil {
 		t.Fatal(err)
 	}
-	return digest.FromBytes(pl)
+	return digest.FromBytes(pl), nil
 }
 
 // TestProxyManifests contains basic acceptance tests
diff --git a/registry/storage/blob_test.go b/registry/storage/blob_test.go
index c84c7432..ab533bd6 100644
--- a/registry/storage/blob_test.go
+++ b/registry/storage/blob_test.go
@@ -176,10 +176,7 @@ func TestSimpleBlobUpload(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error reading all of blob %s", err.Error())
 	}
-	expectedDigest, err := digest.FromBytes(randomBlob)
-	if err != nil {
-		t.Fatalf("Error getting digest from bytes: %s", err)
-	}
+	expectedDigest := digest.FromBytes(randomBlob)
 	simpleUpload(t, bs, randomBlob, expectedDigest)
 
 	d, err = bs.Stat(ctx, expectedDigest)
diff --git a/registry/storage/blobstore.go b/registry/storage/blobstore.go
index f6a8ac43..f8fe23fe 100644
--- a/registry/storage/blobstore.go
+++ b/registry/storage/blobstore.go
@@ -56,12 +56,7 @@ func (bs *blobStore) Open(ctx context.Context, dgst digest.Digest) (distribution
 // content is already present, only the digest will be returned. This should
 // only be used for small objects, such as manifests. This implemented as a convenience for other Put implementations
 func (bs *blobStore) Put(ctx context.Context, mediaType string, p []byte) (distribution.Descriptor, error) {
-	dgst, err := digest.FromBytes(p)
-	if err != nil {
-		context.GetLogger(ctx).Errorf("blobStore: error digesting content: %v, %s", err, string(p))
-		return distribution.Descriptor{}, err
-	}
-
+	dgst := digest.FromBytes(p)
 	desc, err := bs.statter.Stat(ctx, dgst)
 	if err == nil {
 		// content already present
diff --git a/registry/storage/linkedblobstore.go b/registry/storage/linkedblobstore.go
index f01088ba..25640367 100644
--- a/registry/storage/linkedblobstore.go
+++ b/registry/storage/linkedblobstore.go
@@ -75,10 +75,7 @@ func (lbs *linkedBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter
 }
 
 func (lbs *linkedBlobStore) Put(ctx context.Context, mediaType string, p []byte) (distribution.Descriptor, error) {
-	dgst, err := digest.FromBytes(p)
-	if err != nil {
-		return distribution.Descriptor{}, err
-	}
+	dgst := digest.FromBytes(p)
 	// Place the data in the blob store first.
 	desc, err := lbs.blobStore.Put(ctx, mediaType, p)
 	if err != nil {
diff --git a/registry/storage/manifeststore_test.go b/registry/storage/manifeststore_test.go
index 928ce219..de31b364 100644
--- a/registry/storage/manifeststore_test.go
+++ b/registry/storage/manifeststore_test.go
@@ -185,11 +185,7 @@ func TestManifestStorage(t *testing.T) {
 
 	// Now that we have a payload, take a moment to check that the manifest is
 	// return by the payload digest.
-	dgst, err := digest.FromBytes(payload)
-	if err != nil {
-		t.Fatalf("error getting manifest digest: %v", err)
-	}
-
+	dgst := digest.FromBytes(payload)
 	exists, err = ms.Exists(dgst)
 	if err != nil {
 		t.Fatalf("error checking manifest existence by digest: %v", err)