Merge pull request 2139 from restic/fail-zero-bytes-stdin

Return error when reading zero byte from stdin

changelog/unreleased/issue-2135

@@ -0,0 +1,10 @@
+Bugfix: Return error when no bytes could be read from stdin
+
+We assume that users reading backup data from stdin want to know when no data
+could be read, so now restic returns an error when `backup --stdin` is called
+but no bytes could be read. Usually, this means that an earlier command in a
+pipe has failed. The documentation was amended and now recommends setting the
+`pipefail` option (`set -o pipefail`).
+
+https://github.com/restic/restic/pull/2135
+https://github.com/restic/restic/pull/2139

doc/040_backup.rst

@@ -289,6 +289,7 @@ this mode of operation, just supply the option ``--stdin`` to the
 
 .. code-block:: console
 
+    $ set -o pipefail
     $ mysqldump [...] | restic -r /srv/restic-repo backup --stdin
 
 This creates a new snapshot of the output of ``mysqldump``. You can then
@@ -302,6 +303,13 @@ specified with ``--stdin-filename``, e.g. like this:
 
     $ mysqldump [...] | restic -r /srv/restic-repo backup --stdin --stdin-filename production.sql
 
+The option ``pipefail`` is highly recommended so that a non-zero exit code from
+one of the programs in the pipe (e.g. ``mysqldump`` here) makes the whole chain
+return a non-zero exit code. Refer to the `Use the Unofficial Bash Strict Mode
+<http://redsymbol.net/articles/unofficial-bash-strict-mode/>`__ for more
+details on this.
+
+
 Tags for backup
 ***************
 

internal/archiver/archiver_test.go

@@ -160,7 +160,6 @@ func TestArchiverSaveFileReaderFS(t *testing.T) {
 	var tests = []struct {
 		Data string
 	}{
-		{Data: ""},
 		{Data: "foo"},
 		{Data: string(restictest.Random(23, 12*1024*1024+1287898))},
 	}
@@ -271,7 +270,6 @@ func TestArchiverSaveReaderFS(t *testing.T) {
 	var tests = []struct {
 		Data string
 	}{
-		{Data: ""},
 		{Data: "foo"},
 		{Data: string(restictest.Random(23, 12*1024*1024+1287898))},
 	}

internal/fs/fs_reader.go

@@ -1,6 +1,7 @@
 package fs
 
 import (
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -19,10 +20,13 @@ type Reader struct {
 	Name string
 	io.ReadCloser
 
+	// for FileInfo
 	Mode    os.FileMode
 	ModTime time.Time
 	Size    int64
 
+	AllowEmptyFile bool
+
 	open sync.Once
 }
 
@@ -40,7 +44,7 @@ func (fs *Reader) Open(name string) (f File, err error) {
 	switch name {
 	case fs.Name:
 		fs.open.Do(func() {
-			f = newReaderFile(fs.ReadCloser, fs.fi())
+			f = newReaderFile(fs.ReadCloser, fs.fi(), fs.AllowEmptyFile)
 		})
 
 		if f == nil {
@@ -78,7 +82,7 @@ func (fs *Reader) OpenFile(name string, flag int, perm os.FileMode) (f File, err
 	}
 
 	fs.open.Do(func() {
-		f = newReaderFile(fs.ReadCloser, fs.fi())
+		f = newReaderFile(fs.ReadCloser, fs.fi(), fs.AllowEmptyFile)
 	})
 
 	if f == nil {
@@ -158,9 +162,10 @@ func (fs *Reader) Dir(p string) string {
 	return path.Dir(p)
 }
 
-func newReaderFile(rd io.ReadCloser, fi os.FileInfo) readerFile {
+func newReaderFile(rd io.ReadCloser, fi os.FileInfo, allowEmptyFile bool) *readerFile {
-	return readerFile{
+	return &readerFile{
 		ReadCloser:     rd,
+		AllowEmptyFile: allowEmptyFile,
 		fakeFile: fakeFile{
 			FileInfo: fi,
 			name:     fi.Name(),
@@ -170,19 +175,41 @@ func newReaderFile(rd io.ReadCloser, fi os.FileInfo) readerFile {
 
 type readerFile struct {
 	io.ReadCloser
+	AllowEmptyFile, bytesRead bool
+
 	fakeFile
 }
 
-func (r readerFile) Read(p []byte) (int, error) {
+// ErrFileEmpty is returned inside a *os.PathError by Read() for the file
-	return r.ReadCloser.Read(p)
+// opened from the fs provided by Reader when no data could be read and
+// AllowEmptyFile is not set.
+var ErrFileEmpty = errors.New("no data read")
+
+func (r *readerFile) Read(p []byte) (int, error) {
+	n, err := r.ReadCloser.Read(p)
+	if n > 0 {
+		r.bytesRead = true
 	}
 
-func (r readerFile) Close() error {
+	// return an error if we did not read any data
+	if err == io.EOF && !r.AllowEmptyFile && !r.bytesRead {
+		fmt.Printf("reader: %d bytes read, err %v, bytesRead %v, allowEmpty %v\n", n, err, r.bytesRead, r.AllowEmptyFile)
+		return n, &os.PathError{
+			Path: r.fakeFile.name,
+			Op:   "read",
+			Err:  ErrFileEmpty,
+		}
+	}
+
+	return n, err
+}
+
+func (r *readerFile) Close() error {
 	return r.ReadCloser.Close()
 }
 
 // ensure that readerFile implements File
-var _ File = readerFile{}
+var _ File = &readerFile{}
 
 // fakeFile implements all File methods, but only returns errors for anything
 // except Stat() and Name().

internal/fs/fs_reader_test.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -317,3 +318,66 @@ func TestFSReader(t *testing.T) {
 		})
 	}
 }
+
+func TestFSReaderMinFileSize(t *testing.T) {
+	var tests = []struct {
+		name        string
+		data        string
+		allowEmpty  bool
+		readMustErr bool
+	}{
+		{
+			name: "regular",
+			data: "foobar",
+		},
+		{
+			name:        "empty",
+			data:        "",
+			allowEmpty:  false,
+			readMustErr: true,
+		},
+		{
+			name:        "empty2",
+			data:        "",
+			allowEmpty:  true,
+			readMustErr: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fs := &Reader{
+				Name:           "testfile",
+				ReadCloser:     ioutil.NopCloser(strings.NewReader(test.data)),
+				Mode:           0644,
+				ModTime:        time.Now(),
+				AllowEmptyFile: test.allowEmpty,
+			}
+
+			f, err := fs.Open("testfile")
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			buf, err := ioutil.ReadAll(f)
+			if test.readMustErr {
+				if err == nil {
+					t.Fatal("expected error not found, got nil")
+				}
+			} else {
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			if string(buf) != test.data {
+				t.Fatalf("wrong data returned, want %q, got %q", test.data, string(buf))
+			}
+
+			err = f.Close()
+			if err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}