From 2b186fd1f6565e185a9c4a50023fd1fe28c82f0e Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 30 Mar 2017 16:47:05 +0200 Subject: [PATCH 1/2] Update tar test to avoid touching before hardlinking Touching the file before hardlinking was covering up a bug in how hard links are handled in tar. Without the touch the hardlink should still be included and both files should hardlink after the tar is applied. Signed-off-by: Derek McGowan --- archive/tar_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/archive/tar_test.go b/archive/tar_test.go index 144cd7f28..db216c0df 100644 --- a/archive/tar_test.go +++ b/archive/tar_test.go @@ -6,7 +6,6 @@ import ( "os" "os/exec" "testing" - "time" _ "crypto/sha256" @@ -67,8 +66,6 @@ func TestDiffApply(t *testing.T) { fstest.RemoveAll("/home"), fstest.CreateDir("/home/derek", 0700), fstest.CreateFile("/home/derek/.bashrc", []byte("#not going away\n"), 0640), - // "/etc/hosts" must be touched to be hardlinked in same layer - fstest.Chtime("/etc/hosts", time.Now()), fstest.Link("/etc/hosts", "/etc/hosts.allow"), ), } From 51b8e468e5467e5fdf046cdf5854e9c4765582c1 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Tue, 4 Apr 2017 13:32:56 -0700 Subject: [PATCH 2/2] Fix hardlinks with unmodified files Previously hardlinking to an unmodified file or linking to a file which was touched by not detected as modified caused a new file to be created on unpack. This new file and the original source file were not linked since no link record was created in the tar. This change addresses this by adding links for all hardlinks to a file when it is detected as changed. These links will be written after the source file is written and may occur out of order in regard to file name. Signed-off-by: Derek McGowan --- archive/tar.go | 62 ++++++++++++++++++++++++++++++------------ fs/copy.go | 2 +- fs/diff.go | 13 +++++++-- fs/diff_linux.go | 8 ++++++ fs/diff_windows.go | 6 ++++ fs/hardlink.go | 21 ++++++++++++-- fs/hardlink_unix.go | 22 ++------------- fs/hardlink_windows.go | 4 +-- 8 files changed, 94 insertions(+), 44 deletions(-) diff --git a/archive/tar.go b/archive/tar.go index 32511c7a2..b63bbe70f 100644 --- a/archive/tar.go +++ b/archive/tar.go @@ -260,18 +260,20 @@ func Apply(ctx context.Context, root string, r io.Reader) (int64, error) { } type changeWriter struct { - tw *tar.Writer - source string - whiteoutT time.Time - inodeCache map[uint64]string + tw *tar.Writer + source string + whiteoutT time.Time + inodeSrc map[uint64]string + inodeRefs map[uint64][]string } func newChangeWriter(w io.Writer, source string) *changeWriter { return &changeWriter{ - tw: tar.NewWriter(w), - source: source, - whiteoutT: time.Now(), - inodeCache: map[uint64]string{}, + tw: tar.NewWriter(w), + source: source, + whiteoutT: time.Now(), + inodeSrc: map[uint64]string{}, + inodeRefs: map[uint64][]string{}, } } @@ -334,15 +336,28 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e return errors.Wrap(err, "failed to set device headers") } - linkname, err := fs.GetLinkSource(name, f, cw.inodeCache) - if err != nil { - return errors.Wrap(err, "failed to get hardlink") - } - - if linkname != "" { - hdr.Typeflag = tar.TypeLink - hdr.Linkname = linkname - hdr.Size = 0 + // additionalLinks stores file names which must be linked to + // this file when this file is added + var additionalLinks []string + inode, isHardlink := fs.GetLinkInfo(f) + if isHardlink { + // If the inode has a source, always link to it + if source, ok := cw.inodeSrc[inode]; ok { + hdr.Typeflag = tar.TypeLink + hdr.Linkname = source + hdr.Size = 0 + } else { + if k == fs.ChangeKindUnmodified { + cw.inodeRefs[inode] = append(cw.inodeRefs[inode], name) + return nil + } + cw.inodeSrc[inode] = name + additionalLinks = cw.inodeRefs[inode] + delete(cw.inodeRefs, inode) + } + } else if k == fs.ChangeKindUnmodified { + // Nothing to write to diff + return nil } if capability, err := getxattr(source, "security.capability"); err != nil { @@ -374,6 +389,19 @@ func (cw *changeWriter) HandleChange(k fs.ChangeKind, p string, f os.FileInfo, e return errors.New("short write copying file") } } + + if additionalLinks != nil { + source = hdr.Name + for _, extra := range additionalLinks { + hdr.Name = extra + hdr.Typeflag = tar.TypeLink + hdr.Linkname = source + hdr.Size = 0 + if err := cw.tw.WriteHeader(hdr); err != nil { + return errors.Wrap(err, "failed to write file header") + } + } + } } return nil } diff --git a/fs/copy.go b/fs/copy.go index b5eceaf13..0d11fa527 100644 --- a/fs/copy.go +++ b/fs/copy.go @@ -65,7 +65,7 @@ func copyDirectory(dst, src string, inodes map[uint64]string) error { } continue case (fi.Mode() & os.ModeType) == 0: - link, err := GetLinkSource(target, fi, inodes) + link, err := getLinkSource(target, fi, inodes) if err != nil { return errors.Wrap(err, "failed to get hardlink") } diff --git a/fs/diff.go b/fs/diff.go index 52e2445bc..5fed7cf2d 100644 --- a/fs/diff.go +++ b/fs/diff.go @@ -16,9 +16,13 @@ import ( type ChangeKind int const ( + // ChangeKindUnmodified represents an unmodified + // file + ChangeKindUnmodified = iota + // ChangeKindAdd represents an addition of // a file - ChangeKindAdd = iota + ChangeKindAdd // ChangeKindModify represents a change to // an existing file @@ -31,6 +35,8 @@ const ( func (k ChangeKind) String() string { switch k { + case ChangeKindUnmodified: + return "unmodified" case ChangeKindAdd: return "add" case ChangeKindModify: @@ -287,7 +293,10 @@ func doubleWalkDiff(ctx context.Context, changeFn ChangeFunc, a, b string) (err f1 = nil f2 = nil if same { - continue + if !isLinked(f) { + continue + } + k = ChangeKindUnmodified } } if err := changeFn(k, p, f, nil); err != nil { diff --git a/fs/diff_linux.go b/fs/diff_linux.go index b6bad8454..24c290a5f 100644 --- a/fs/diff_linux.go +++ b/fs/diff_linux.go @@ -90,3 +90,11 @@ func compareCapabilities(p1, p2 string) (bool, error) { } return bytes.Equal(c1, c2), nil } + +func isLinked(f os.FileInfo) bool { + s, ok := f.Sys().(*syscall.Stat_t) + if !ok { + return false + } + return !f.IsDir() && s.Nlink > 1 +} diff --git a/fs/diff_windows.go b/fs/diff_windows.go index 90fb30fb9..7bbd66284 100644 --- a/fs/diff_windows.go +++ b/fs/diff_windows.go @@ -1,5 +1,7 @@ package fs +import "os" + func detectDirDiff(upper, lower string) *diffDirOptions { return nil } @@ -13,3 +15,7 @@ func compareCapabilities(p1, p2 string) (bool, error) { // TODO: Use windows equivalent return true, nil } + +func isLinked(os.FileInfo) bool { + return false +} diff --git a/fs/hardlink.go b/fs/hardlink.go index a6c256bd6..4d5156f40 100644 --- a/fs/hardlink.go +++ b/fs/hardlink.go @@ -2,11 +2,26 @@ package fs import "os" -// GetLinkSource returns a path for the given name and +// GetLinkID returns an identifier representing the node a hardlink is pointing +// to. If the file is not hard linked then 0 will be returned. +func GetLinkInfo(fi os.FileInfo) (uint64, bool) { + return getLinkInfo(fi) +} + +// getLinkSource returns a path for the given name and // file info to its link source in the provided inode // map. If the given file name is not in the map and // has other links, it is added to the inode map // to be a source for other link locations. -func GetLinkSource(name string, fi os.FileInfo, inodes map[uint64]string) (string, error) { - return getHardLink(name, fi, inodes) +func getLinkSource(name string, fi os.FileInfo, inodes map[uint64]string) (string, error) { + inode, isHardlink := getLinkInfo(fi) + if !isHardlink { + return "", nil + } + + path, ok := inodes[inode] + if !ok { + inodes[inode] = name + } + return path, nil } diff --git a/fs/hardlink_unix.go b/fs/hardlink_unix.go index 76d1897dd..3b825c940 100644 --- a/fs/hardlink_unix.go +++ b/fs/hardlink_unix.go @@ -3,31 +3,15 @@ package fs import ( - "errors" "os" "syscall" ) -func getHardLink(name string, fi os.FileInfo, inodes map[uint64]string) (string, error) { - if fi.IsDir() { - return "", nil - } - +func getLinkInfo(fi os.FileInfo) (uint64, bool) { s, ok := fi.Sys().(*syscall.Stat_t) if !ok { - return "", errors.New("unsupported stat type") + return 0, false } - // If inode is not hardlinked, no reason to lookup or save inode - if s.Nlink == 1 { - return "", nil - } - - inode := uint64(s.Ino) - - path, ok := inodes[inode] - if !ok { - inodes[inode] = name - } - return path, nil + return uint64(s.Ino), !fi.IsDir() && s.Nlink > 1 } diff --git a/fs/hardlink_windows.go b/fs/hardlink_windows.go index 81e50eec0..ad8845a7f 100644 --- a/fs/hardlink_windows.go +++ b/fs/hardlink_windows.go @@ -2,6 +2,6 @@ package fs import "os" -func getHardLink(string, os.FileInfo, map[uint64]string) (string, error) { - return "", nil +func getLinkInfo(fi os.FileInfo) (uint64, bool) { + return 0, false }