Browse code

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 <sargun@sargun.me>

Sargun Dhillon authored on 2017/11/24 09:31:31
Showing 2 changed files
... ...
@@ -11,6 +11,7 @@ package copy
11 11
 */
12 12
 import "C"
13 13
 import (
14
+	"container/list"
14 15
 	"fmt"
15 16
 	"io"
16 17
 	"os"
... ...
@@ -111,6 +112,11 @@ type fileID struct {
111 111
 	ino uint64
112 112
 }
113 113
 
114
+type dirMtimeInfo struct {
115
+	dstPath *string
116
+	stat    *syscall.Stat_t
117
+}
118
+
114 119
 // DirCopy copies or hardlinks the contents of one directory to another,
115 120
 // properly handling xattrs, and soft links
116 121
 //
... ...
@@ -118,9 +124,11 @@ type fileID struct {
118 118
 func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
119 119
 	copyWithFileRange := true
120 120
 	copyWithFileClone := true
121
+
121 122
 	// This is a map of source file inodes to dst file paths
122 123
 	copiedFiles := make(map[fileID]string)
123 124
 
125
+	dirsToSetMtimes := list.New()
124 126
 	err := filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error {
125 127
 		if err != nil {
126 128
 			return err
... ...
@@ -226,7 +234,9 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
226 226
 
227 227
 		// system.Chtimes doesn't support a NOFOLLOW flag atm
228 228
 		// nolint: unconvert
229
-		if !isSymlink {
229
+		if f.IsDir() {
230
+			dirsToSetMtimes.PushFront(&dirMtimeInfo{dstPath: &dstPath, stat: stat})
231
+		} else if !isSymlink {
230 232
 			aTime := time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec))
231 233
 			mTime := time.Unix(int64(stat.Mtim.Sec), int64(stat.Mtim.Nsec))
232 234
 			if err := system.Chtimes(dstPath, aTime, mTime); err != nil {
... ...
@@ -240,7 +250,18 @@ func DirCopy(srcDir, dstDir string, copyMode Mode, copyXattrs bool) error {
240 240
 		}
241 241
 		return nil
242 242
 	})
243
-	return err
243
+	if err != nil {
244
+		return err
245
+	}
246
+	for e := dirsToSetMtimes.Front(); e != nil; e = e.Next() {
247
+		mtimeInfo := e.Value.(*dirMtimeInfo)
248
+		ts := []syscall.Timespec{mtimeInfo.stat.Atim, mtimeInfo.stat.Mtim}
249
+		if err := system.LUtimesNano(*mtimeInfo.dstPath, ts); err != nil {
250
+			return err
251
+		}
252
+	}
253
+
254
+	return nil
244 255
 }
245 256
 
246 257
 func doCopyXattrs(srcPath, dstPath string) error {
... ...
@@ -3,17 +3,20 @@
3 3
 package copy
4 4
 
5 5
 import (
6
+	"fmt"
6 7
 	"io/ioutil"
7 8
 	"math/rand"
8 9
 	"os"
9 10
 	"path/filepath"
11
+	"syscall"
10 12
 	"testing"
11
-
12
-	"golang.org/x/sys/unix"
13
+	"time"
13 14
 
14 15
 	"github.com/docker/docker/pkg/parsers/kernel"
16
+	"github.com/docker/docker/pkg/system"
15 17
 	"github.com/stretchr/testify/assert"
16 18
 	"github.com/stretchr/testify/require"
19
+	"golang.org/x/sys/unix"
17 20
 )
18 21
 
19 22
 func TestIsCopyFileRangeSyscallAvailable(t *testing.T) {
... ...
@@ -47,6 +50,84 @@ func TestCopyWithoutRange(t *testing.T) {
47 47
 	doCopyTest(t, &copyWithFileRange, &copyWithFileClone)
48 48
 }
49 49
 
50
+func TestCopyDir(t *testing.T) {
51
+	srcDir, err := ioutil.TempDir("", "srcDir")
52
+	require.NoError(t, err)
53
+	populateSrcDir(t, srcDir, 3)
54
+
55
+	dstDir, err := ioutil.TempDir("", "testdst")
56
+	require.NoError(t, err)
57
+	defer os.RemoveAll(dstDir)
58
+
59
+	assert.NoError(t, DirCopy(srcDir, dstDir, Content, false))
60
+	require.NoError(t, filepath.Walk(srcDir, func(srcPath string, f os.FileInfo, err error) error {
61
+		if err != nil {
62
+			return err
63
+		}
64
+
65
+		// Rebase path
66
+		relPath, err := filepath.Rel(srcDir, srcPath)
67
+		require.NoError(t, err)
68
+		if relPath == "." {
69
+			return nil
70
+		}
71
+
72
+		dstPath := filepath.Join(dstDir, relPath)
73
+		require.NoError(t, err)
74
+
75
+		// If we add non-regular dirs and files to the test
76
+		// then we need to add more checks here.
77
+		dstFileInfo, err := os.Lstat(dstPath)
78
+		require.NoError(t, err)
79
+
80
+		srcFileSys := f.Sys().(*syscall.Stat_t)
81
+		dstFileSys := dstFileInfo.Sys().(*syscall.Stat_t)
82
+
83
+		t.Log(relPath)
84
+		if srcFileSys.Dev == dstFileSys.Dev {
85
+			assert.NotEqual(t, srcFileSys.Ino, dstFileSys.Ino)
86
+		}
87
+		// Todo: check size, and ctim is not equal
88
+		/// on filesystems that have granular ctimes
89
+		assert.Equal(t, srcFileSys.Mode, dstFileSys.Mode)
90
+		assert.Equal(t, srcFileSys.Uid, dstFileSys.Uid)
91
+		assert.Equal(t, srcFileSys.Gid, dstFileSys.Gid)
92
+		assert.Equal(t, srcFileSys.Mtim, dstFileSys.Mtim)
93
+
94
+		return nil
95
+	}))
96
+}
97
+
98
+func randomMode(baseMode int) os.FileMode {
99
+	for i := 0; i < 7; i++ {
100
+		baseMode = baseMode | (1&rand.Intn(2))<<uint(i)
101
+	}
102
+	return os.FileMode(baseMode)
103
+}
104
+
105
+func populateSrcDir(t *testing.T, srcDir string, remainingDepth int) {
106
+	if remainingDepth == 0 {
107
+		return
108
+	}
109
+	aTime := time.Unix(rand.Int63(), 0)
110
+	mTime := time.Unix(rand.Int63(), 0)
111
+
112
+	for i := 0; i < 10; i++ {
113
+		dirName := filepath.Join(srcDir, fmt.Sprintf("srcdir-%d", i))
114
+		// Owner all bits set
115
+		require.NoError(t, os.Mkdir(dirName, randomMode(0700)))
116
+		populateSrcDir(t, dirName, remainingDepth-1)
117
+		require.NoError(t, system.Chtimes(dirName, aTime, mTime))
118
+	}
119
+
120
+	for i := 0; i < 10; i++ {
121
+		fileName := filepath.Join(srcDir, fmt.Sprintf("srcfile-%d", i))
122
+		// Owner read bit set
123
+		require.NoError(t, ioutil.WriteFile(fileName, []byte{}, randomMode(0400)))
124
+		require.NoError(t, system.Chtimes(fileName, aTime, mTime))
125
+	}
126
+}
127
+
50 128
 func doCopyTest(t *testing.T, copyWithFileRange, copyWithFileClone *bool) {
51 129
 	dir, err := ioutil.TempDir("", "docker-copy-check")
52 130
 	require.NoError(t, err)