From c4fbf2c779143b7dc73016966128eeadd56af770 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 6 Jan 2019 14:31:11 +0100 Subject: [PATCH 1/2] Return error when reading zero byte from stdin This commit changes the internal file system implementation for reading data from stdin, it now returns an error when no bytes could be read. I think it's worth failing in this case, the user instructed restic to read some data from stdin, and no data was read at all. Maybe it was in a pipe and some earlier stage failed. See #2135 for a short discussion. --- internal/archiver/archiver_test.go | 2 - internal/fs/fs_reader.go | 45 ++++++++++++++++----- internal/fs/fs_reader_test.go | 64 ++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 11 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index e7cda551a..8749dcc81 100644 --- a/internal/archiver/archiver_test.go +++ b/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))}, } diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 385c8f92b..27fc46eb6 100644 --- a/internal/fs/fs_reader.go +++ b/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 { - return readerFile{ - ReadCloser: rd, +func newReaderFile(rd io.ReadCloser, fi os.FileInfo, allowEmptyFile bool) *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) { - return r.ReadCloser.Read(p) +// ErrFileEmpty is returned inside a *os.PathError by Read() for the file +// 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 + } + + // 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 { +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(). diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index f4cb2bb34..93e8b35ef 100644 --- a/internal/fs/fs_reader_test.go +++ b/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) + } + }) + } +} From 6f69ae1b8deb3aa3bb171c8542b657810338c0d8 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 6 Jan 2019 14:47:58 +0100 Subject: [PATCH 2/2] Add docs, changelog file --- changelog/unreleased/issue-2135 | 10 ++++++++++ doc/040_backup.rst | 8 ++++++++ 2 files changed, 18 insertions(+) create mode 100644 changelog/unreleased/issue-2135 diff --git a/changelog/unreleased/issue-2135 b/changelog/unreleased/issue-2135 new file mode 100644 index 000000000..b689b54f7 --- /dev/null +++ b/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 diff --git a/doc/040_backup.rst b/doc/040_backup.rst index 951f65a4c..14715de9a 100644 --- a/doc/040_backup.rst +++ b/doc/040_backup.rst @@ -286,6 +286,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 @@ -299,6 +300,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 +`__ for more +details on this. + + Tags for backup ***************