gadget, osutil: use atomic file copy, adjust tests #9103
Conversation
The regular CopyFile() call can only overwrite the destination location in place. Add a new helper that wraps the destination path with AtomicFile, to obtain a 2 stage write & commit (or cancel) behavior. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Use the CopyFileAtomic helper for moving file data around. Update the tests to match slightly changed semantics. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
gadget/mountedfilesystem.go
Outdated
| // TODO try to preserve ownership and permission bits | ||
| if err := osutil.CopyFile(src, dst, copyFlags); err != nil { | ||
|
|
||
| // dst is a reflection of src, no additional flags are needed |
There was a problem hiding this comment.
Not sure I understand this comment - but maybe I just need more tea?
There was a problem hiding this comment.
Quite sure it needs a better comment, which I'll add
gadget/mountedfilesystem_test.go
Outdated
| @@ -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.
Thanks for this updated comment, but even though the file is 0000 rollback will restore it, yes? Maybe the comment could mention this too?
osutil/cp.go
Outdated
| // 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.
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.
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 cp under the hood. In that sense, the semantics are more like AtomicFile, so instead passing CopyFlags, it's taking AtomicWriteFlags.
There was a problem hiding this comment.
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.
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.
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.
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
this makes it a bit clearer that ATM some aspects of the behavior are closer to the AtomicWrite* family for consistency this involved also swapping argument order
The regular CopyFile() call can only overwrite the destination location in
place. Add a new helper that wraps the destination path with AtomicFile, to
obtain a 2 stage write & commit (or cancel) behavior.
Use the CopyFileAtomic helper in gadget for moving file data around.
Update the tests to match slightly changed semantics.