From 20a2865e5342e3faa387138e6c52e2bd3f957525 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Thu, 23 Nov 2017 16:31:31 -0800 Subject: [PATCH] Fix setting mtimes on directories Previously, the code would set the mtime on the directories before creating files in the directory itself. This was problematic because it resulted in the mtimes on the directories being incorrectly set. This change makes it so that the mtime is set only _after_ all of the files have been created. Signed-off-by: Sargun Dhillon Upstream-commit: 77a2bc3e5bbc9be3fe166ed8321b7cd04e7bd097 Component: engine --- .../engine/daemon/graphdriver/copy/copy.go | 25 +++++- .../daemon/graphdriver/copy/copy_test.go | 85 ++++++++++++++++++- 2 files changed, 106 insertions(+), 4 deletions(-) diff --git a/components/engine/daemon/graphdriver/copy/copy.go b/components/engine/daemon/graphdriver/copy/copy.go index 3046089512..7a98bec8ba 100644 --- a/components/engine/daemon/graphdriver/copy/copy.go +++ b/components/engine/daemon/graphdriver/copy/copy.go @@ -11,6 +11,7 @@ package copy */ import "C" import ( + "container/list" "fmt" "io" "os" @@ -111,6 +112,11 @@ type fileID struct { ino uint64 } +type dirMtimeInfo struct { + dstPath *string + stat *syscall.Stat_t +} + // DirCopy copies or hardlinks the contents of one directory to another, // properly handling xattrs, and soft links // @@ -118,9 +124,11 @@ type fileID struct { func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { copyWithFileRange := true copyWithFileClone := true + // This is a map of source file inodes to dst file paths copiedFiles := make(map[fileID]string) + dirsToSetMtimes := list.New() err := filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error { if err != nil { return err @@ -226,7 +234,9 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { // system.Chtimes doesn't support a NOFOLLOW flag atm // nolint: unconvert - if !isSymlink { + if f.IsDir() { + dirsToSetMtimes.PushFront(&dirMtimeInfo{dstPath: &dstPath, stat: stat}) + } else if !isSymlink { aTime := time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec)) mTime := time.Unix(int64(stat.Mtim.Sec), int64(stat.Mtim.Nsec)) if err := system.Chtimes(dstPath, aTime, mTime); err != nil { @@ -240,7 +250,18 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error { } return nil }) - return err + if err != nil { + return err + } + for e := dirsToSetMtimes.Front(); e != nil; e = e.Next() { + mtimeInfo := e.Value.(*dirMtimeInfo) + ts := []syscall.Timespec{mtimeInfo.stat.Atim, mtimeInfo.stat.Mtim} + if err := system.LUtimesNano(*mtimeInfo.dstPath, ts); err != nil { + return err + } + } + + return nil } func doCopyXattrs(srcPath, dstPath string) error { diff --git a/components/engine/daemon/graphdriver/copy/copy_test.go b/components/engine/daemon/graphdriver/copy/copy_test.go index ce99536e01..d21699114d 100644 --- a/components/engine/daemon/graphdriver/copy/copy_test.go +++ b/components/engine/daemon/graphdriver/copy/copy_test.go @@ -3,17 +3,20 @@ package copy import ( + "fmt" "io/ioutil" "math/rand" "os" "path/filepath" + "syscall" "testing" - - "golang.org/x/sys/unix" + "time" "github.com/docker/docker/pkg/parsers/kernel" + "github.com/docker/docker/pkg/system" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" ) func TestIsCopyFileRangeSyscallAvailable(t *testing.T) { @@ -47,6 +50,84 @@ func TestCopyWithoutRange(t *testing.T) { doCopyTest(t, ©WithFileRange, ©WithFileClone) } +func TestCopyDir(t *testing.T) { + srcDir, err := ioutil.TempDir("", "srcDir") + require.NoError(t, err) + populateSrcDir(t, srcDir, 3) + + dstDir, err := ioutil.TempDir("", "testdst") + require.NoError(t, err) + defer os.RemoveAll(dstDir) + + assert.NoError(t, DirCopy(srcDir, dstDir, Content, false)) + require.NoError(t, filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error { + if err != nil { + return err + } + + // Rebase path + relPath, err := filepath.Rel(srcDir, srcPath) + require.NoError(t, err) + if relPath == "." { + return nil + } + + dstPath := filepath.Join(dstDir, relPath) + require.NoError(t, err) + + // If we add non-regular dirs and files to the test + // then we need to add more checks here. + dstFileInfo, err := os.Lstat(dstPath) + require.NoError(t, err) + + srcFileSys := f.Sys().(*syscall.Stat_t) + dstFileSys := dstFileInfo.Sys().(*syscall.Stat_t) + + t.Log(relPath) + if srcFileSys.Dev == dstFileSys.Dev { + assert.NotEqual(t, srcFileSys.Ino, dstFileSys.Ino) + } + // Todo: check size, and ctim is not equal + /// on filesystems that have granular ctimes + assert.Equal(t, srcFileSys.Mode, dstFileSys.Mode) + assert.Equal(t, srcFileSys.Uid, dstFileSys.Uid) + assert.Equal(t, srcFileSys.Gid, dstFileSys.Gid) + assert.Equal(t, srcFileSys.Mtim, dstFileSys.Mtim) + + return nil + })) +} + +func randomMode(baseMode int) os.FileMode { + for i := 0; i < 7; i++ { + baseMode = baseMode | (1&rand.Intn(2))<