diff --git a/pkg/blobserver/localdisk/localdisk_test.go b/pkg/blobserver/localdisk/localdisk_test.go index 736f4b216..9816d53b1 100644 --- a/pkg/blobserver/localdisk/localdisk_test.go +++ b/pkg/blobserver/localdisk/localdisk_test.go @@ -18,7 +18,10 @@ package localdisk import ( "fmt" + "io/ioutil" "os" + "path/filepath" + "runtime" "strconv" "sync" "testing" @@ -144,3 +147,44 @@ func TestMissingGetReturnsNoEnt(t *testing.T) { t.Errorf("expected nil blob; got a value") } } + +func rename(old, new string) error { + if err := os.Rename(old, new); err != nil { + if renameErr := mapRenameError(err, old, new); renameErr != nil { + return err + } + } + return nil +} + +type file struct { + name string + contents string +} + +func TestRename(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Skipping test if not on windows") + } + files := []file{ + file{name: filepath.Join(os.TempDir(), "foo"), contents: "foo"}, + file{name: filepath.Join(os.TempDir(), "bar"), contents: "barr"}, + file{name: filepath.Join(os.TempDir(), "baz"), contents: "foo"}, + } + for _, v := range files { + if err := ioutil.WriteFile(v.name, []byte(v.contents), 0755); err != nil { + t.Fatal(err) + } + } + + // overwriting "bar" with "foo" should not be allowed + if err := rename(files[0].name, files[1].name); err == nil { + t.Fatalf("Renaming %v into %v should not succeed", files[0].name, files[1].name) + } + + // but overwriting "baz" with "foo" is ok because they have the same + // contents + if err := rename(files[0].name, files[2].name); err != nil { + t.Fatal(err) + } +} diff --git a/pkg/blobserver/localdisk/receive.go b/pkg/blobserver/localdisk/receive.go index 429f65f4f..f29a9805a 100644 --- a/pkg/blobserver/localdisk/receive.go +++ b/pkg/blobserver/localdisk/receive.go @@ -75,7 +75,9 @@ func (ds *DiskStorage) ReceiveBlob(blobRef blob.Ref, source io.Reader) (ref blob fileName := ds.blobPath("", blobRef) if err = os.Rename(tempFile.Name(), fileName); err != nil { - return + if err = mapRenameError(err, tempFile.Name(), fileName); err != nil { + return + } } stat, err = os.Lstat(fileName) diff --git a/pkg/blobserver/localdisk/receive_posix.go b/pkg/blobserver/localdisk/receive_posix.go index 709ff97db..064db17cc 100644 --- a/pkg/blobserver/localdisk/receive_posix.go +++ b/pkg/blobserver/localdisk/receive_posix.go @@ -33,3 +33,7 @@ func linkOrCopy(src, dst string) error { } return err } + +func handleRenameError(err error, oldfile, newfile string) error { + return err +} diff --git a/pkg/blobserver/localdisk/receive_windows.go b/pkg/blobserver/localdisk/receive_windows.go index 5f189f7dc..15fe39b37 100644 --- a/pkg/blobserver/localdisk/receive_windows.go +++ b/pkg/blobserver/localdisk/receive_windows.go @@ -16,6 +16,43 @@ limitations under the License. package localdisk +import ( + "fmt" + "os" + "syscall" +) + func linkOrCopy(src, dst string) error { return copyFile(src, dst) } + +// mapRenameError returns nil if and only if +// 1) the input err is the error returned on windows when trying to rename +// a file over one that already exists +// 2) oldfile and newfile are the same files (i.e have the same size) +func mapRenameError(err error, oldfile, newfile string) error { + linkErr, ok := err.(*os.LinkError) + if !ok { + return err + } + if linkErr.Err != error(syscall.ERROR_ALREADY_EXISTS) { + return err + } + // TODO(mpl): actually on linux at least, os.Rename apparently + // erases the destination with no error even if it is different + // from the source. + // So why don't we allow the same for windows? and if needed, + // do the check size before renaming? + statNew, err := os.Stat(newfile) + if err != nil { + return err + } + statOld, err := os.Stat(oldfile) + if err != nil { + return err + } + if statNew.Size() != statOld.Size() { + return fmt.Errorf("Will not overwrite destination file %v with source file %v, as they are different.", newfile, oldfile) + } + return nil +}