From 037f0a4c91aacd9d2d7f8191d6dc532034f8441a Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Nov 2020 21:45:54 +0100 Subject: [PATCH 1/4] Refactor device ID checking --- cmd/restic/exclude.go | 81 ++++++++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/cmd/restic/exclude.go b/cmd/restic/exclude.go index 540a3d2ff..3c6000877 100644 --- a/cmd/restic/exclude.go +++ b/cmd/restic/exclude.go @@ -199,12 +199,17 @@ func isDirExcludedByFile(dir, tagFilename, header string) bool { return true } -// gatherDevices returns the set of unique device ids of the files and/or -// directory paths listed in "items". -func gatherDevices(items []string) (deviceMap map[string]uint64, err error) { - deviceMap = make(map[string]uint64) - for _, item := range items { - item, err = filepath.Abs(filepath.Clean(item)) +// DeviceMap is used to track allowed source devices for backup. This is used to +// check for crossing mount points during backup (for --one-file-system). It +// maps the name of a source path to its device ID. +type DeviceMap map[string]uint64 + +// NewDeviceMap creates a new device map from the list of source paths. +func NewDeviceMap(allowedSourcePaths []string) (DeviceMap, error) { + deviceMap := make(map[string]uint64) + + for _, item := range allowedSourcePaths { + item, err := filepath.Abs(filepath.Clean(item)) if err != nil { return nil, err } @@ -213,30 +218,63 @@ func gatherDevices(items []string) (deviceMap map[string]uint64, err error) { if err != nil { return nil, err } + id, err := fs.DeviceID(fi) if err != nil { return nil, err } + deviceMap[item] = id } + if len(deviceMap) == 0 { return nil, errors.New("zero allowed devices") } + return deviceMap, nil } +// IsAllowed returns true if the path is located on an allowed device. +func (m DeviceMap) IsAllowed(item string, deviceID uint64) (bool, error) { + for dir := item; ; dir = filepath.Dir(dir) { + debug.Log("item %v, test dir %v", item, dir) + + // find a parent directory that is on an allowed device (otherwise + // we would not traverse the directory at all) + allowedID, ok := m[dir] + if !ok { + if dir == filepath.Dir(dir) { + // arrived at root, no allowed device found. this should not happen. + break + } + continue + } + + // if the item has a different device ID than the parent directory, + // we crossed a file system boundary + if allowedID != deviceID { + debug.Log("item %v (dir %v) on disallowed device %d", item, dir, deviceID) + return false, nil + } + + // item is on allowed device, accept it + debug.Log("item %v allowed", item) + return true, nil + } + + return false, fmt.Errorf("item %v (device ID %v) not found, deviceMap: %v", item, deviceID, m) +} + // rejectByDevice returns a RejectFunc that rejects files which are on a // different file systems than the files/dirs in samples. func rejectByDevice(samples []string) (RejectFunc, error) { - allowed, err := gatherDevices(samples) + deviceMap, err := NewDeviceMap(samples) if err != nil { return nil, err } - debug.Log("allowed devices: %v\n", allowed) + debug.Log("allowed devices: %v\n", deviceMap) return func(item string, fi os.FileInfo) bool { - item = filepath.Clean(item) - id, err := fs.DeviceID(fi) if err != nil { // This should never happen because gatherDevices() would have @@ -244,26 +282,13 @@ func rejectByDevice(samples []string) (RejectFunc, error) { panic(err) } - for dir := item; ; dir = filepath.Dir(dir) { - debug.Log("item %v, test dir %v", item, dir) - - allowedID, ok := allowed[dir] - if !ok { - if dir == filepath.Dir(dir) { - break - } - continue - } - - if allowedID != id { - debug.Log("path %q on disallowed device %d", item, id) - return true - } - - return false + allowed, err := deviceMap.IsAllowed(filepath.Clean(item), id) + if err != nil { + // this should not happen + panic(fmt.Sprintf("error checking device ID of %v: %v", item, err)) } - panic(fmt.Sprintf("item %v, device id %v not found, allowedDevs: %v", item, id, allowed)) + return !allowed }, nil } From f576d3d82614fd4f359d6abddc76ea037449bb56 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Nov 2020 21:52:21 +0100 Subject: [PATCH 2/4] Add tests --- cmd/restic/exclude_test.go | 44 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/cmd/restic/exclude_test.go b/cmd/restic/exclude_test.go index bc8c29866..c7bec4352 100644 --- a/cmd/restic/exclude_test.go +++ b/cmd/restic/exclude_test.go @@ -318,3 +318,47 @@ func TestIsExcludedByFileSize(t *testing.T) { } } } + +func TestDeviceMap(t *testing.T) { + deviceMap := DeviceMap{ + filepath.FromSlash("/"): 1, + filepath.FromSlash("/usr/local"): 5, + } + + var tests = []struct { + item string + deviceID uint64 + allowed bool + }{ + {"/root", 1, true}, + {"/usr", 1, true}, + + {"/proc", 2, false}, + {"/proc/1234", 2, false}, + + {"/usr", 3, false}, + {"/usr/share", 3, false}, + + {"/usr/local", 5, true}, + {"/usr/local/foobar", 5, true}, + + {"/usr/local/foobar/submount", 23, false}, + {"/usr/local/foobar/submount/file", 23, false}, + + {"/usr/local/foobar/outhersubmount", 1, false}, + {"/usr/local/foobar/outhersubmount/otherfile", 1, false}, + } + + for _, test := range tests { + t.Run("", func(t *testing.T) { + res, err := deviceMap.IsAllowed(filepath.FromSlash(test.item), test.deviceID) + if err != nil { + t.Fatal(err) + } + + if res != test.allowed { + t.Fatalf("wrong result returned by IsAllowed(%v): want %v, got %v", test.item, test.allowed, res) + } + }) + } +} From 82ae942965ed2b30efa0ca7b296f0a8d4a36f594 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Nov 2020 22:08:37 +0100 Subject: [PATCH 3/4] backup: Keep mountpoints for --one-file-system When a file system is mounted at a directory, lstat() returns attributes of the root node of the mounted file system, including the device ID of the other file system. The previous code used when --one-file-system is specified excluded the directory itself because of that. This commit changes the code so that mountpoints are kept as empty directories, its attributes set to the root note of the mounted file system. The behavior mimics `tar`, which does the same. --- cmd/restic/exclude.go | 44 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/cmd/restic/exclude.go b/cmd/restic/exclude.go index 3c6000877..e63c689ec 100644 --- a/cmd/restic/exclude.go +++ b/cmd/restic/exclude.go @@ -288,7 +288,49 @@ func rejectByDevice(samples []string) (RejectFunc, error) { panic(fmt.Sprintf("error checking device ID of %v: %v", item, err)) } - return !allowed + if allowed { + // accept item + return false + } + + // reject everything except directories + if !fi.IsDir() { + return true + } + + // special case: make sure we keep mountpoints (directories which + // contain a mounted file system). Test this by checking if the parent + // directory would be included. + parentDir := filepath.Dir(filepath.Clean(item)) + + parentFI, err := fs.Lstat(parentDir) + if err != nil { + debug.Log("item %v: error running lstat() on parent directory: %v", item, err) + // if in doubt, reject + return true + } + + parentDeviceID, err := fs.DeviceID(parentFI) + if err != nil { + debug.Log("item %v: getting device ID of parent directory: %v", item, err) + // if in doubt, reject + return true + } + + parentAllowed, err := deviceMap.IsAllowed(parentDir, parentDeviceID) + if err != nil { + debug.Log("item %v: error checking parent directory: %v", item, err) + // if in doubt, reject + return true + } + + if parentAllowed { + // we found a mount point, so accept the directory + return false + } + + // reject everything else + return true }, nil } From 162117c42c342a3b9e9f86afa8520fa8b16111ed Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 21 Nov 2020 22:18:40 +0100 Subject: [PATCH 4/4] Add changelog --- changelog/unreleased/issue-909 | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 changelog/unreleased/issue-909 diff --git a/changelog/unreleased/issue-909 b/changelog/unreleased/issue-909 new file mode 100644 index 000000000..1480ed675 --- /dev/null +++ b/changelog/unreleased/issue-909 @@ -0,0 +1,12 @@ +Enhancement: Keep mountpoints as empty directories + +When the `--one-file-system` option is specified to `restic backup`, it +ignores all file systems mounted below one of the target directories. This +means that when a snapshot is restored, users needed to manually recreate the +mountpoint directories. + +Restic now keeps mountpoints as empty directories and therefore implements +the same approach as `tar`. + +https://github.com/restic/restic/issues/909 +https://github.com/restic/restic/pull/3119