plugin/errors: remove panic/recover (#1777)

Remove panic/recover and also use pkg/log to print the error. This
brings some consistency into the logging.

Fixes #1776

plugin/errors/errors.go

@@ -3,77 +3,27 @@ package errors
 
 import (
 	"context"
-	"fmt"
-	"log"
-	"runtime"
-	"strings"
-	"time"
 
 	"github.com/coredns/coredns/plugin"
+	clog "github.com/coredns/coredns/plugin/pkg/log"
 	"github.com/coredns/coredns/request"
 
 	"github.com/miekg/dns"
 )
 
 // errorHandler handles DNS errors (and errors from other plugin).
-type errorHandler struct {
-	Next    plugin.Handler
-	LogFile string
-	Log     *log.Logger
-}
+type errorHandler struct{ Next plugin.Handler }
 
 // ServeDNS implements the plugin.Handler interface.
 func (h errorHandler) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
-	defer h.recovery(ctx, w, r)
-
 	rcode, err := plugin.NextOrFailure(h.Name(), h.Next, ctx, w, r)
 
 	if err != nil {
 		state := request.Request{W: w, Req: r}
-		errMsg := fmt.Sprintf("%s [ERROR %d %s %s] %v", time.Now().Format(timeFormat), rcode, state.Name(), state.Type(), err)
-
-		h.Log.Println(errMsg)
+		clog.Errorf("%d %s %s: %v", rcode, state.Name(), state.Type(), err)
 	}
 
 	return rcode, err
 }
 
 func (h errorHandler) Name() string { return "errors" }
-
-func (h errorHandler) recovery(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) {
-	rec := recover()
-	if rec == nil {
-		return
-	}
-
-	// Obtain source of panic
-	// From: https://gist.github.com/swdunlop/9629168
-	var name, file string // function name, file name
-	var line int
-	var pc [16]uintptr
-	n := runtime.Callers(3, pc[:])
-	for _, pc := range pc[:n] {
-		fn := runtime.FuncForPC(pc)
-		if fn == nil {
-			continue
-		}
-		file, line = fn.FileLine(pc)
-		name = fn.Name()
-		if !strings.HasPrefix(name, "runtime.") {
-			break
-		}
-	}
-
-	// Trim file path
-	delim := "/coredns/"
-	pkgPathPos := strings.Index(file, delim)
-	if pkgPathPos > -1 && len(file) > pkgPathPos+len(delim) {
-		file = file[pkgPathPos+len(delim):]
-	}
-
-	panicMsg := fmt.Sprintf("%s [PANIC %s %s] %s:%d - %v", time.Now().Format(timeFormat), r.Question[0].Name, dns.Type(r.Question[0].Qtype), file, line, rec)
-	// Currently we don't use the function name, since file:line is more conventional
-	h.Log.Printf(panicMsg)
-}
-
-const timeFormat = "02/Jan/2006:15:04:05 -0700"

plugin/errors/errors_test.go

@@ -18,7 +18,8 @@ import (
 
 func TestErrors(t *testing.T) {
 	buf := bytes.Buffer{}
-	em := errorHandler{Log: log.New(&buf, "", 0)}
+	log.SetOutput(&buf)
+	em := errorHandler{}
 
 	testErr := errors.New("test error")
 	tests := []struct {
@@ -36,7 +37,7 @@ func TestErrors(t *testing.T) {
 		{
 			next:         genErrorHandler(dns.RcodeNotAuth, testErr),
 			expectedCode: dns.RcodeNotAuth,
-			expectedLog:  fmt.Sprintf("[ERROR %d %s] %v\n", dns.RcodeNotAuth, "example.org. A", testErr),
+			expectedLog:  fmt.Sprintf("[ERROR] %d %s: %v\n", dns.RcodeNotAuth, "example.org. A", testErr),
 			expectedErr:  testErr,
 		},
 	}

plugin/errors/setup.go

@@ -2,8 +2,6 @@ package errors
 
 import (
 	"fmt"
-	"log"
-	"os"
 
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
@@ -24,8 +22,6 @@ func setup(c *caddy.Controller) error {
 		return plugin.Error("errors", err)
 	}
 
-	handler.Log = log.New(os.Stdout, "", 0)
-
 	dnsserver.GetConfig(c).AddPlugin(func(next plugin.Handler) plugin.Handler {
 		handler.Next = next
 		return handler
@@ -47,12 +43,10 @@ func errorsParse(c *caddy.Controller) (errorHandler, error) {
 		args := c.RemainingArgs()
 		switch len(args) {
 		case 0:
-			handler.LogFile = "stdout"
 		case 1:
 			if args[0] != "stdout" {
 				return handler, fmt.Errorf("invalid log file: %s", args[0])
 			}
-			handler.LogFile = args[0]
 		default:
 			return handler, c.ArgErr()
 		}

plugin/errors/setup_test.go

@@ -8,31 +8,26 @@ import (
 
 func TestErrorsParse(t *testing.T) {
 	tests := []struct {
-		inputErrorsRules     string
-		shouldErr            bool
-		expectedErrorHandler errorHandler
+		inputErrorsRules string
+		shouldErr        bool
 	}{
-		{`errors`, false, errorHandler{LogFile: "stdout"}},
-		{`errors stdout`, false, errorHandler{LogFile: "stdout"}},
-		{`errors errors.txt`, true, errorHandler{LogFile: ""}},
-		{`errors visible`, true, errorHandler{LogFile: ""}},
-		{`errors { log visible }`, true, errorHandler{LogFile: "stdout"}},
+		{`errors`, false},
+		{`errors stdout`, false},
+		{`errors errors.txt`, true},
+		{`errors visible`, true},
+		{`errors { log visible }`, true},
 		{`errors
-		errors `, true, errorHandler{LogFile: "stdout"}},
-		{`errors a b`, true, errorHandler{LogFile: ""}},
+		errors `, true},
+		{`errors a b`, true},
 	}
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.inputErrorsRules)
-		actualErrorsRule, err := errorsParse(c)
+		_, err := errorsParse(c)
 
 		if err == nil && test.shouldErr {
 			t.Errorf("Test %d didn't error, but it should have", i)
 		} else if err != nil && !test.shouldErr {
 			t.Errorf("Test %d errored, but it shouldn't have; got '%v'", i, err)
 		}
-		if actualErrorsRule.LogFile != test.expectedErrorHandler.LogFile {
-			t.Errorf("Test %d expected LogFile to be %s, but got %s",
-				i, test.expectedErrorHandler.LogFile, actualErrorsRule.LogFile)
-		}
 	}
 }