Merge pull request caddyserver#1775 from tw4452852/roller_parse

log,error: fix roller parser issue
overcome · Jul 20, 2017 · 40b52fb · 40b52fb
2 parents f1dd9f2 + 91150bb
commit 40b52fb
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 19 deletions.
diff --git a/caddyhttp/errors/setup.go b/caddyhttp/errors/setup.go
@@ -44,18 +44,20 @@ func errorsParse(c *caddy.Controller) (*ErrorHandler, error) {
 		for c.NextBlock() {
 
 			what := c.Val()
-			if !c.NextArg() {
-				return c.ArgErr()
-			}
-			where := c.Val()
+			where := c.RemainingArgs()
 
 			if httpserver.IsLogRollerSubdirective(what) {
 				var err error
-				err = httpserver.ParseRoller(handler.Log.Roller, what, where)
+				err = httpserver.ParseRoller(handler.Log.Roller, what, where...)
 				if err != nil {
 					return err
 				}
 			} else {
+				if len(where) != 1 {
+					return c.ArgErr()
+				}
+				where := where[0]
+
 				// Error page; ensure it exists
 				if !filepath.IsAbs(where) {
 					where = filepath.Join(cfg.Root, where)

diff --git a/caddyhttp/errors/setup_test.go b/caddyhttp/errors/setup_test.go
@@ -85,7 +85,12 @@ func TestErrorsParse(t *testing.T) {
 				Roller: httpserver.DefaultLogRoller(),
 			},
 		}},
-		{`errors errors.txt { rotate_size 2 rotate_age 10 rotate_keep 3 rotate_compress }`, false, ErrorHandler{
+		{`errors errors.txt {
+			rotate_size 2
+			rotate_age 10
+			rotate_keep 3
+			rotate_compress
+		}`, false, ErrorHandler{
 			ErrorPages: map[int]string{},
 			Log: &httpserver.Logger{
 				Output: "errors.txt", Roller: &httpserver.LogRoller{
@@ -144,6 +149,12 @@ func TestErrorsParse(t *testing.T) {
 				},
 				Log: &httpserver.Logger{},
 			}},
+		{`errors errors.txt { rotate_size 2 rotate_age 10 rotate_keep 3 rotate_compress }`,
+			true, ErrorHandler{ErrorPages: map[int]string{}, Log: &httpserver.Logger{}}},
+		{`errors errors.txt {
+			rotate_compress invalid
+		}`,
+			true, ErrorHandler{ErrorPages: map[int]string{}, Log: &httpserver.Logger{}}},
 		// Next two test cases is the detection of duplicate status codes
 		{`errors {
 			503 503.html

diff --git a/caddyhttp/httpserver/roller.go b/caddyhttp/httpserver/roller.go
@@ -1,6 +1,7 @@
 package httpserver
 
 import (
+	"errors"
 	"io"
 	"path/filepath"
 	"strconv"
@@ -54,17 +55,32 @@ func IsLogRollerSubdirective(subdir string) bool {
 		subdir == directiveRotateCompress
 }
 
+var invalidRollerParameterErr = errors.New("invalid roller parameter")
+
 // ParseRoller parses roller contents out of c.
-func ParseRoller(l *LogRoller, what string, where string) error {
+func ParseRoller(l *LogRoller, what string, where ...string) error {
 	if l == nil {
 		l = DefaultLogRoller()
 	}
-	var value int
-	var err error
-	value, err = strconv.Atoi(where)
-	if what != directiveRotateCompress && err != nil {
-		return err
+
+	// rotate_compress doesn't accept any parameters.
+	// others only accept one parameter
+	if (what == directiveRotateCompress && len(where) != 0) ||
+		(what != directiveRotateCompress && len(where) != 1) {
+		return invalidRollerParameterErr
+	}
+
+	var (
+		value int
+		err   error
+	)
+	if what != directiveRotateCompress {
+		value, err = strconv.Atoi(where[0])
+		if err != nil {
+			return err
+		}
 	}
+
 	switch what {
 	case directiveRotateSize:
 		l.MaxSize = value

diff --git a/caddyhttp/log/setup.go b/caddyhttp/log/setup.go
@@ -38,17 +38,14 @@ func logParse(c *caddy.Controller) ([]*Rule, error) {
 
 		for c.NextBlock() {
 			what := c.Val()
-			if !c.NextArg() {
-				return nil, c.ArgErr()
-			}
-			where := c.Val()
+			where := c.RemainingArgs()
 
 			// only support roller related options inside a block
 			if !httpserver.IsLogRollerSubdirective(what) {
 				return nil, c.ArgErr()
 			}
 
-			if err := httpserver.ParseRoller(logRoller, what, where); err != nil {
+			if err := httpserver.ParseRoller(logRoller, what, where...); err != nil {
 				return nil, err
 			}
 		}

diff --git a/caddyhttp/log/setup_test.go b/caddyhttp/log/setup_test.go
@@ -194,7 +194,12 @@ func TestLogParse(t *testing.T) {
 				Format: "{when}",
 			}},
 		}}},
-		{`log access.log { rotate_size 2 rotate_age 10 rotate_keep 3 }`, false, []Rule{{
+		{`log access.log {
+			rotate_size 2
+			rotate_age 10
+			rotate_keep 3
+			rotate_compress
+		}`, false, []Rule{{
 			PathScope: "/",
 			Entries: []*Entry{{
 				Log: &httpserver.Logger{
@@ -203,7 +208,7 @@ func TestLogParse(t *testing.T) {
 						MaxSize:    2,
 						MaxAge:     10,
 						MaxBackups: 3,
-						Compress:   false,
+						Compress:   true,
 						LocalTime:  true,
 					}},
 				Format: DefaultLogFormat,
@@ -226,6 +231,8 @@ func TestLogParse(t *testing.T) {
 				Format: "{when}",
 			}},
 		}}},
+		{`log access.log { rotate_size 2 rotate_age 10 rotate_keep 3 }`, true, nil},
+		{`log access.log { rotate_compress invalid }`, true, nil},
 		{`log access.log { rotate_size }`, true, nil},
 		{`log access.log { invalid_option 1 }`, true, nil},
 		{`log / acccess.log "{remote} - [{when}] "{method} {port}" {scheme} {mitm} "`, true, nil},