From b93eb68ac3b1b8ec4c2d1e15c37e362d9bb6836c Mon Sep 17 00:00:00 2001 From: Simon Dassow Date: Sat, 19 Feb 2022 10:58:23 +0100 Subject: [PATCH 1/3] Small code path cleanup that came up in a code review with @eau-u4f --- atomic.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/atomic.go b/atomic.go index f7e2706..1988691 100644 --- a/atomic.go +++ b/atomic.go @@ -43,6 +43,11 @@ func WriteFile(filename string, r io.Reader) (err error) { if err := f.Sync(); err != nil { return fmt.Errorf("can't flush tempfile %q: %v", name, err) } + // get file info via file descriptor before closing it. + sourceInfo, err := f.Stat() + if err != nil { + return err + } if err := f.Close(); err != nil { return fmt.Errorf("can't close tempfile %q: %v", name, err) } @@ -50,20 +55,13 @@ func WriteFile(filename string, r io.Reader) (err error) { // get the file mode from the original file and use that for the replacement // file, too. destInfo, err := os.Stat(filename) - if os.IsNotExist(err) { - // no original file - } else if err != nil { + if err != nil && !os.IsNotExist(err) { return err - } else { - sourceInfo, err := os.Stat(name) - if err != nil { - return err - } - - if sourceInfo.Mode() != destInfo.Mode() { - if err := os.Chmod(name, destInfo.Mode()); err != nil { - return fmt.Errorf("can't set filemode on tempfile %q: %v", name, err) - } + } + // check for nil as one error type is allowed to happen. + if destInfo != nil && destInfo.Mode() != sourceInfo.Mode() { + if err := os.Chmod(name, destInfo.Mode()); err != nil { + return fmt.Errorf("can't set filemode on tempfile %q: %v", name, err) } } if err := ReplaceFile(name, filename); err != nil { From 01e75dd8a77164c9b7c6e0df02db26772443c5c4 Mon Sep 17 00:00:00 2001 From: Simon Dassow Date: Tue, 22 Feb 2022 07:54:45 +0100 Subject: [PATCH 2/3] Better approach to support file mode changes after cleanup As before this fixes natefinch/atomic#7 and it would allow to release a v2 where KeepFileMode is off by default. --- atomic.go | 68 ++++++++++++++++++++++++++++++++++++----- atomic_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 7 deletions(-) create mode 100644 atomic_test.go diff --git a/atomic.go b/atomic.go index 1988691..ebf2e6a 100644 --- a/atomic.go +++ b/atomic.go @@ -9,12 +9,52 @@ import ( "path/filepath" ) +type FileOptions struct { + defaultFileMode os.FileMode + fileMode os.FileMode + keepFileMode bool +} + +type Option func(*FileOptions) + +// FileMode can be given as an argument to `WriteFile()` to change the file +// mode to the desired value. +func FileMode(mode os.FileMode) Option { + return func(opts *FileOptions) { + opts.fileMode = mode + } +} + +// DefaultFileMode can be given as an argument to `WriteFile()` to change the +// file mode from the default value of ioutil.TempFile() (`0600`). +func DefaultFileMode(mode os.FileMode) Option { + return func(opts *FileOptions) { + opts.defaultFileMode = mode + } +} + +// KeepFileMode() can be given as an argument to `WriteFile()` to keep the file +// mode of an existing file instead of using the default value. +func KeepFileMode(keep bool) Option { + return func(opts *FileOptions) { + opts.keepFileMode = keep + } +} + // WriteFile atomically writes the contents of r to the specified filepath. If // an error occurs, the target file is guaranteed to be either fully written, or // not written at all. WriteFile overwrites any file that exists at the // location (but only if the write fully succeeds, otherwise the existing file // is unmodified). -func WriteFile(filename string, r io.Reader) (err error) { +func WriteFile(filename string, r io.Reader, opts ...Option) (err error) { + // original behaviour is to preserve the mode of an existing file. + fopts := &FileOptions{ + keepFileMode: true, + } + for _, opt := range opts { + opt(fopts) + } + // write to a temp file first, then we'll atomically replace the target file // with the temp file. dir, file := filepath.Split(filename) @@ -52,15 +92,29 @@ func WriteFile(filename string, r io.Reader) (err error) { return fmt.Errorf("can't close tempfile %q: %v", name, err) } + var fileMode os.FileMode + // change default file mode for when file does not exist yet. + if fopts.defaultFileMode != 0 { + fileMode = fopts.defaultFileMode + } // get the file mode from the original file and use that for the replacement // file, too. - destInfo, err := os.Stat(filename) - if err != nil && !os.IsNotExist(err) { - return err + if fopts.keepFileMode { + destInfo, err := os.Stat(filename) + if err != nil && !os.IsNotExist(err) { + return err + } + if destInfo != nil { + fileMode = destInfo.Mode() + } + } + // given file mode always takes precedence + if fopts.fileMode != 0 { + fileMode = fopts.fileMode } - // check for nil as one error type is allowed to happen. - if destInfo != nil && destInfo.Mode() != sourceInfo.Mode() { - if err := os.Chmod(name, destInfo.Mode()); err != nil { + // apply possible file mode change + if fileMode != 0 && fileMode != sourceInfo.Mode() { + if err := os.Chmod(name, fileMode); err != nil { return fmt.Errorf("can't set filemode on tempfile %q: %v", name, err) } } diff --git a/atomic_test.go b/atomic_test.go new file mode 100644 index 0000000..6771346 --- /dev/null +++ b/atomic_test.go @@ -0,0 +1,83 @@ +package atomic + +import ( + "bytes" + "os" + "testing" +) + +func TestWriteFile(t *testing.T) { + file := "foo.txt" + content := bytes.NewBufferString("foo") + defer func() { _ = os.Remove(file) }() + if err := WriteFile(file, content); err != nil { + t.Errorf("Failed to write file: %q: %v", file, err) + } + fi, err := os.Stat(file) + if err != nil { + t.Errorf("Failed to stat file: %q: %v", file, err) + } + if fi.Mode() != 0600 { + t.Errorf("File mode not correct") + } +} + +func TestWriteDefaultFileMode(t *testing.T) { + file := "bar.txt" + content := bytes.NewBufferString("bar") + defer func() { _ = os.Remove(file) }() + if err := WriteFile(file, content, DefaultFileMode(0644)); err != nil { + t.Errorf("Failed to write file: %q: %v", file, err) + } + fi, err := os.Stat(file) + if err != nil { + t.Errorf("Failed to stat file: %q: %v", file, err) + } + if fi.Mode() != 0644 { + t.Errorf("File mode not correct: %v", fi.Mode()) + } + // check if file mode is preserved + if err := os.Chmod(file, 0600); err != nil { + t.Errorf("Failed to change file mode: %q: %v", file, err) + } + if err := WriteFile(file, content, DefaultFileMode(0644)); err != nil { + t.Errorf("Failed to write file: %q: %v", file, err) + } + fi, err = os.Stat(file) + if err != nil { + t.Errorf("Failed to stat file: %q: %v", file, err) + } + if fi.Mode() != 0600 { + t.Errorf("File mode not correct: %v", fi.Mode()) + } +} + +func TestWriteFileMode(t *testing.T) { + file := "baz.txt" + content := bytes.NewBufferString("baz") + defer func() { _ = os.Remove(file) }() + if err := WriteFile(file, content, FileMode(0644)); err != nil { + t.Errorf("Failed to write file: %q: %v", file, err) + } + fi, err := os.Stat(file) + if err != nil { + t.Errorf("Failed to stat file: %q: %v", file, err) + } + if fi.Mode() != 0644 { + t.Errorf("File mode not correct: %v", fi.Mode()) + } + // ensure previous file mode is ingored + if err := os.Chmod(file, 0600); err != nil { + t.Errorf("Failed to change file mode: %q: %v", file, err) + } + if err := WriteFile(file, content, FileMode(0644)); err != nil { + t.Errorf("Failed to write file: %q: %v", file, err) + } + fi, err = os.Stat(file) + if err != nil { + t.Errorf("Failed to stat file: %q: %v", file, err) + } + if fi.Mode() != 0644 { + t.Errorf("File mode not correct: %v", fi.Mode()) + } +} From 6cb1782beba3cbc8dc6594b35a77be4e891e28f8 Mon Sep 17 00:00:00 2001 From: Simon Dassow Date: Wed, 23 Feb 2022 09:40:48 +0100 Subject: [PATCH 3/3] Improve documentation, and add go reference link --- README.md | 29 +++++++++++++++++++++++++---- atomic.go | 17 ++++++++++------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 37cd673..06e6ab1 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,8 @@ # atomic - import "github.com/natefinch/atomic" -atomic is a go package for atomic file writing + +Go package for atomic file writing + +[![Go Reference](https://pkg.go.dev/badge/github.com/natefinch/atomic.svg)](https://pkg.go.dev/github.com/natefinch/atomic) By default, writing to a file in go (and generally any language) can fail partway through... you then have a partially written file, which probably was @@ -25,11 +27,30 @@ change either file. ## func WriteFile ``` go -func WriteFile(filename string, r io.Reader) (err error) +func WriteFile(filename string, r io.Reader, opts ...Option) (err error) ``` WriteFile atomically writes the contents of r to the specified filepath. If an error occurs, the target file is guaranteed to be either fully written, or not written at all. WriteFile overwrites any file that exists at the location (but only if the write fully succeeds, otherwise the existing file -is unmodified). +is unmodified). Additional option arguments can be used to change the +default configuration for the target file. + + +## Example +``` go +import ( + "strings" + + "github.com/natefinch/atomic" +) + +func main() { + r := strings.NewReader("yes\n") + err := atomic.WriteFile("consistent.txt", r, atomic.FileMode(0440)) + if err != nil { + // handle error + } +} +``` diff --git a/atomic.go b/atomic.go index ebf2e6a..e1a0767 100644 --- a/atomic.go +++ b/atomic.go @@ -9,32 +9,34 @@ import ( "path/filepath" ) +// FileOptions define the behaviour of `FileWrite()`. type FileOptions struct { defaultFileMode os.FileMode fileMode os.FileMode keepFileMode bool } +// Option functions modify FileOptions. type Option func(*FileOptions) -// FileMode can be given as an argument to `WriteFile()` to change the file -// mode to the desired value. +// FileMode sets the file mode to the desired value and has precedence over all +// other options. func FileMode(mode os.FileMode) Option { return func(opts *FileOptions) { opts.fileMode = mode } } -// DefaultFileMode can be given as an argument to `WriteFile()` to change the -// file mode from the default value of ioutil.TempFile() (`0600`). +// DefaultFileMode sets the default file mode instead of using the +// `ioutil.TempFile()` default of `0600`. func DefaultFileMode(mode os.FileMode) Option { return func(opts *FileOptions) { opts.defaultFileMode = mode } } -// KeepFileMode() can be given as an argument to `WriteFile()` to keep the file -// mode of an existing file instead of using the default value. +// KeepFileMode preserves the file mode of an existing file instead of using the +// default value. func KeepFileMode(keep bool) Option { return func(opts *FileOptions) { opts.keepFileMode = keep @@ -45,7 +47,8 @@ func KeepFileMode(keep bool) Option { // an error occurs, the target file is guaranteed to be either fully written, or // not written at all. WriteFile overwrites any file that exists at the // location (but only if the write fully succeeds, otherwise the existing file -// is unmodified). +// is unmodified). Additional option arguments can be used to change the +// default configuration for the target file. func WriteFile(filename string, r io.Reader, opts ...Option) (err error) { // original behaviour is to preserve the mode of an existing file. fopts := &FileOptions{