New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gadget, osutil: use atomic file copy, adjust tests #9103
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -473,7 +473,7 @@ func (s *mountedfilesystemTestSuite) TestMountedWriterConflictingDestinationDire | |
|
||
// can't overwrite a directory with a file | ||
err = rw.Write(outDir, nil) | ||
c.Assert(err, ErrorMatches, fmt.Sprintf("cannot write filesystem content of source:foo-dir: cannot copy .*: unable to create %s/foo-dir: .* is a directory", outDir)) | ||
c.Assert(err, ErrorMatches, fmt.Sprintf(`cannot write filesystem content of source:foo-dir: cannot copy .*: cannot commit atomic file copy: rename %[1]s/foo-dir\.[a-zA-Z0-9]+~ %[1]s/foo-dir: file exists`, outDir)) | ||
|
||
} | ||
|
||
|
@@ -2021,7 +2021,8 @@ func (s *mountedfilesystemTestSuite) TestMountedUpdaterRollbackRestoreFails(c *C | |
{target: "foo", content: "written"}, | ||
{target: "some-dir/foo", content: "written"}, | ||
}) | ||
// make rollback fail when restoring | ||
// the file exists, rollback will try to restore it, but make the file | ||
// non-modifiable directly | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this updated comment, but even though the file is 0000 rollback will restore it, yes? Maybe the comment could mention this too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will add. |
||
err := os.Chmod(filepath.Join(outDir, "foo"), 0000) | ||
c.Assert(err, IsNil) | ||
|
||
|
@@ -2055,12 +2056,19 @@ func (s *mountedfilesystemTestSuite) TestMountedUpdaterRollbackRestoreFails(c *C | |
makeSizedFile(c, filepath.Join(s.backup, "struct-0/foo.backup"), 0, []byte("backup")) | ||
|
||
err = rw.Rollback() | ||
c.Assert(err, ErrorMatches, "cannot rollback content: cannot copy .*: unable to create .*/out-dir/foo: permission denied") | ||
c.Assert(err, IsNil) | ||
// the file was restored | ||
c.Check(filepath.Join(outDir, "foo"), testutil.FileEquals, "backup") | ||
// directory was removed | ||
c.Check(osutil.IsDirectory(filepath.Join(outDir, "some-dir")), Equals, false) | ||
|
||
// remove offending file | ||
c.Assert(os.Remove(filepath.Join(outDir, "foo")), IsNil) | ||
// mock the data again | ||
makeExistingData(c, outDir, []gadgetData{ | ||
{target: "foo", content: "written"}, | ||
{target: "some-dir/foo", content: "written"}, | ||
}) | ||
|
||
// make destination dir non-writable | ||
// make the directory non-writable | ||
err = os.Chmod(filepath.Join(outDir, "some-dir"), 0555) | ||
c.Assert(err, IsNil) | ||
// restore permissions later, otherwise test suite cleanup complains | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,46 @@ func CopyFile(src, dst string, flags CopyFlag) (err error) { | |
return nil | ||
} | ||
|
||
// CopyFileAtomic copies src to dst using AtomicFile internally to create the | ||
// destination. The destination path is always overwritten. The destination and | ||
// the owning directory are synced after copy completes. Pass additional flags | ||
// for AtomicFile wrapping the destination. | ||
func CopyFileAtomic(src, dst string, flags AtomicWriteFlags) (err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It slightly bothers me slightly that "CopyFileAtomic" takes different flags than "CopyFile()". OTOH maybe the perfect should not be the enemy of the good and we can always tweak/fix further as needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that we cannot really make any use of the flags. CopyFlagSync is meaningless because we always sync, CopyFlagOverwrite is racy unless we try to open the dst file with O_CREAT|O_EXCL next to AtomicFile, lastly CopyFlagPreserveAll cannot be implemented right now without calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should be called AtomicCopyFile. Regarding needeing cp, we could use it if needed and cp to a random file name and then AtomicRename that to the final name, it's not our current use case I think though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other possible naming would be AtomicWriteFileCopy, in that case though src and dst would have to be swapped I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like AtomicWriteFileCopy() and we can always consolidate into CopyFile() with the Atomic flag later, maybe a XXX: should we merge into CopyFIle with new AtomicCopyFile or similar. |
||
fin, err := openfile(src, os.O_RDONLY, 0) | ||
if err != nil { | ||
return fmt.Errorf("unable to open source file %s: %v", src, err) | ||
} | ||
defer func() { | ||
if cerr := fin.Close(); cerr != nil && err == nil { | ||
err = fmt.Errorf("when closing %s: %v", src, cerr) | ||
} | ||
}() | ||
|
||
fi, err := fin.Stat() | ||
if err != nil { | ||
return fmt.Errorf("unable to stat %s: %v", src, err) | ||
} | ||
|
||
fout, err := NewAtomicFile(dst, fi.Mode(), flags, NoChown, NoChown) | ||
if err != nil { | ||
return fmt.Errorf("cannot create atomic file: %v", err) | ||
} | ||
defer func() { | ||
if cerr := fout.Cancel(); cerr != ErrCannotCancel && err == nil { | ||
err = fmt.Errorf("cannot cancel temporary file copy %s: %v", fout.Name(), cerr) | ||
} | ||
}() | ||
|
||
if err := copyfile(fin, fout, fi); err != nil { | ||
return fmt.Errorf("unable to copy %s to %s: %v", src, fout.Name(), err) | ||
} | ||
|
||
if err := fout.Commit(); err != nil { | ||
return fmt.Errorf("cannot commit atomic file copy: %v", err) | ||
} | ||
return nil | ||
} | ||
|
||
func runCmd(cmd *exec.Cmd, errdesc string) error { | ||
if output, err := cmd.CombinedOutput(); err != nil { | ||
output = bytes.TrimSpace(output) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this comment - but maybe I just need more tea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite sure it needs a better comment, which I'll add