dns/dnsmessage: change Builder to append and update documentation

The new appending behavior is required for an efficient DNS client.

(*Builder).Start was intended to append, but didn't. This was a mistake
(all tests and examples assumed that it did).

In addition, message compression when appending has been fixed.

Updates golang/go#16218

Change-Id: I3f42aa6e653e2990fa90368a2803e588ea15b85a
Reviewed-on: https://go-review.googlesource.com/97716Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

dns/dnsmessage/message_test.go

@@ -62,7 +62,7 @@ func TestQuestionPackUnpack(t *testing.T) {
 		Type:  TypeA,
 		Class: ClassINET,
 	}
-	buf, err := want.pack(make([]byte, 1, 50), map[string]int{})
+	buf, err := want.pack(make([]byte, 1, 50), map[string]int{}, 1)
 	if err != nil {
 		t.Fatal("Packing failed:", err)
 	}
@@ -129,7 +129,7 @@ func TestNamePackUnpack(t *testing.T) {
 	for _, test := range tests {
 		in := mustNewName(test.in)
 		want := mustNewName(test.want)
-		buf, err := in.pack(make([]byte, 0, 30), map[string]int{})
+		buf, err := in.pack(make([]byte, 0, 30), map[string]int{}, 0)
 		if err != test.err {
 			t.Errorf("Packing of %q: got err = %v, want err = %v", test.in, err, test.err)
 			continue
@@ -248,6 +248,40 @@ func TestDNSPackUnpack(t *testing.T) {
 	}
 }
 
+func TestDNSAppendPackUnpack(t *testing.T) {
+	wants := []Message{
+		{
+			Questions: []Question{
+				{
+					Name:  mustNewName("."),
+					Type:  TypeAAAA,
+					Class: ClassINET,
+				},
+			},
+			Answers:     []Resource{},
+			Authorities: []Resource{},
+			Additionals: []Resource{},
+		},
+		largeTestMsg(),
+	}
+	for i, want := range wants {
+		b := make([]byte, 2, 514)
+		b, err := want.AppendPack(b)
+		if err != nil {
+			t.Fatalf("%d: packing failed: %v", i, err)
+		}
+		b = b[2:]
+		var got Message
+		err = got.Unpack(b)
+		if err != nil {
+			t.Fatalf("%d: unpacking failed: %v", i, err)
+		}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("%d: got = %+v, want = %+v", i, &got, &want)
+		}
+	}
+}
+
 func TestSkipAll(t *testing.T) {
 	msg := largeTestMsg()
 	buf, err := msg.Pack()
@@ -412,7 +446,7 @@ func TestVeryLongTxt(t *testing.T) {
 		},
 		&TXTResource{loremIpsum},
 	}
-	buf, err := want.pack(make([]byte, 0, 8000), map[string]int{})
+	buf, err := want.pack(make([]byte, 0, 8000), map[string]int{}, 0)
 	if err != nil {
 		t.Fatal("Packing failed:", err)
 	}
@@ -434,6 +468,26 @@ func TestVeryLongTxt(t *testing.T) {
 	}
 }
 
+func TestStartAppends(t *testing.T) {
+	buf := make([]byte, 2, 514)
+	wantBuf := []byte{4, 44}
+	copy(buf, wantBuf)
+
+	b := NewBuilder(buf, Header{})
+	b.EnableCompression()
+
+	buf, err := b.Finish()
+	if err != nil {
+		t.Fatal("Building failed:", err)
+	}
+	if got, want := len(buf), headerLen+2; got != want {
+		t.Errorf("Got len(buf} = %d, want = %d", got, want)
+	}
+	if string(buf[:2]) != string(wantBuf) {
+		t.Errorf("Original data not preserved, got = %v, want = %v", buf[:2], wantBuf)
+	}
+}
+
 func TestStartError(t *testing.T) {
 	tests := []struct {
 		name string
@@ -514,8 +568,8 @@ func TestBuilder(t *testing.T) {
 		t.Fatal("Packing without builder:", err)
 	}
 
-	var b Builder
-	b.Start(nil, msg.Header)
+	b := NewBuilder(nil, msg.Header)
+	b.EnableCompression()
 
 	if err := b.StartQuestions(); err != nil {
 		t.Fatal("b.StartQuestions():", err)
@@ -653,9 +707,7 @@ func TestResourcePack(t *testing.T) {
 	}
 }
 
-func BenchmarkParsing(b *testing.B) {
-	b.ReportAllocs()
-
+func benchmarkParsingSetup() ([]byte, error) {
 	name := mustNewName("foo.bar.example.com.")
 	msg := Message{
 		Header: Header{Response: true, Authoritative: true},
@@ -700,13 +752,15 @@ func BenchmarkParsing(b *testing.B) {
 
 	buf, err := msg.Pack()
 	if err != nil {
-		b.Fatal("msg.Pack():", err)
+		return nil, fmt.Errorf("msg.Pack(): %v", err)
 	}
+	return buf, nil
+}
 
-	for i := 0; i < b.N; i++ {
+func benchmarkParsing(tb testing.TB, buf []byte) {
 	var p Parser
 	if _, err := p.Start(buf); err != nil {
-			b.Fatal("p.Start(buf):", err)
+		tb.Fatal("p.Start(buf):", err)
 	}
 
 	for {
@@ -715,7 +769,7 @@ func BenchmarkParsing(b *testing.B) {
 			break
 		}
 		if err != nil {
-				b.Fatal("p.Question():", err)
+			tb.Fatal("p.Question():", err)
 		}
 	}
 
@@ -731,39 +785,60 @@ func BenchmarkParsing(b *testing.B) {
 		switch h.Type {
 		case TypeA:
 			if _, err := p.AResource(); err != nil {
-					b.Fatal("p.AResource():", err)
+				tb.Fatal("p.AResource():", err)
 			}
 		case TypeAAAA:
 			if _, err := p.AAAAResource(); err != nil {
-					b.Fatal("p.AAAAResource():", err)
+				tb.Fatal("p.AAAAResource():", err)
 			}
 		case TypeCNAME:
 			if _, err := p.CNAMEResource(); err != nil {
-					b.Fatal("p.CNAMEResource():", err)
+				tb.Fatal("p.CNAMEResource():", err)
 			}
 		case TypeNS:
 			if _, err := p.NSResource(); err != nil {
-					b.Fatal("p.NSResource():", err)
+				tb.Fatal("p.NSResource():", err)
 			}
 		default:
-				b.Fatalf("unknown type: %T", h)
-			}
+			tb.Fatalf("unknown type: %T", h)
 		}
 	}
 }
 
-func BenchmarkBuilding(b *testing.B) {
+func BenchmarkParsing(b *testing.B) {
+	buf, err := benchmarkParsingSetup()
+	if err != nil {
+		b.Fatal(err)
+	}
+
 	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		benchmarkParsing(b, buf)
+	}
+}
 
+func TestParsingAllocs(t *testing.T) {
+	buf, err := benchmarkParsingSetup()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if allocs := testing.AllocsPerRun(100, func() { benchmarkParsing(t, buf) }); allocs > 0.5 {
+		t.Errorf("Allocations during parsing: got = %f, want ~0", allocs)
+	}
+}
+
+func benchmarkBuildingSetup() (Name, []byte) {
 	name := mustNewName("foo.bar.example.com.")
 	buf := make([]byte, 0, packStartingCap)
+	return name, buf
+}
 
-	for i := 0; i < b.N; i++ {
-		var bld Builder
-		bld.StartWithoutCompression(buf, Header{Response: true, Authoritative: true})
+func benchmarkBuilding(tb testing.TB, name Name, buf []byte) {
+	bld := NewBuilder(buf, Header{Response: true, Authoritative: true})
 
 	if err := bld.StartQuestions(); err != nil {
-			b.Fatal("bld.StartQuestions():", err)
+		tb.Fatal("bld.StartQuestions():", err)
 	}
 	q := Question{
 		Name:  name,
@@ -771,7 +846,7 @@ func BenchmarkBuilding(b *testing.B) {
 		Class: ClassINET,
 	}
 	if err := bld.Question(q); err != nil {
-			b.Fatalf("bld.Question(%+v): %v", q, err)
+		tb.Fatalf("bld.Question(%+v): %v", q, err)
 	}
 
 	hdr := ResourceHeader{
@@ -779,32 +854,46 @@ func BenchmarkBuilding(b *testing.B) {
 		Class: ClassINET,
 	}
 	if err := bld.StartAnswers(); err != nil {
-			b.Fatal("bld.StartQuestions():", err)
+		tb.Fatal("bld.StartQuestions():", err)
 	}
 
 	ar := AResource{[4]byte{}}
 	if err := bld.AResource(hdr, ar); err != nil {
-			b.Fatalf("bld.AResource(%+v, %+v): %v", hdr, ar, err)
+		tb.Fatalf("bld.AResource(%+v, %+v): %v", hdr, ar, err)
 	}
 
 	aaar := AAAAResource{[16]byte{}}
 	if err := bld.AAAAResource(hdr, aaar); err != nil {
-			b.Fatalf("bld.AAAAResource(%+v, %+v): %v", hdr, aaar, err)
+		tb.Fatalf("bld.AAAAResource(%+v, %+v): %v", hdr, aaar, err)
 	}
 
 	cnr := CNAMEResource{name}
 	if err := bld.CNAMEResource(hdr, cnr); err != nil {
-			b.Fatalf("bld.CNAMEResource(%+v, %+v): %v", hdr, cnr, err)
+		tb.Fatalf("bld.CNAMEResource(%+v, %+v): %v", hdr, cnr, err)
 	}
 
 	nsr := NSResource{name}
 	if err := bld.NSResource(hdr, nsr); err != nil {
-			b.Fatalf("bld.NSResource(%+v, %+v): %v", hdr, nsr, err)
+		tb.Fatalf("bld.NSResource(%+v, %+v): %v", hdr, nsr, err)
 	}
 
 	if _, err := bld.Finish(); err != nil {
-			b.Fatal("bld.Finish():", err)
+		tb.Fatal("bld.Finish():", err)
 	}
+}
+
+func BenchmarkBuilding(b *testing.B) {
+	name, buf := benchmarkBuildingSetup()
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		benchmarkBuilding(b, name, buf)
+	}
+}
+
+func TestBuildingAllocs(t *testing.T) {
+	name, buf := benchmarkBuildingSetup()
+	if allocs := testing.AllocsPerRun(100, func() { benchmarkBuilding(t, name, buf) }); allocs > 0.5 {
+		t.Errorf("Allocations during building: got = %f, want ~0", allocs)
 	}
 }