From fb9132ae90184b518146f0c2574860d4d49df3ef Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Sat, 30 Apr 2016 19:42:19 -0700 Subject: [PATCH] Inconsistent --tmpfs behavior This fix tries to address the issue raised in #22420. When `--tmpfs` is specified with `/tmp`, the default value is `rw,nosuid,nodev,noexec,relatime,size=65536k`. When `--tmpfs` is specified with `/tmp:rw`, then the value changed to `rw,nosuid,nodev,noexec,relatime`. The reason for such an inconsistency is because docker tries to add `size=65536k` option only when user provides no option. This fix tries to address this issue by always pre-progating `size=65536k` along with `rw,nosuid,nodev,noexec,relatime`. If user provides a different value (e.g., `size=8192k`), it will override the `size=65536k` anyway since the combined options will be parsed and merged to remove any duplicates. Additional test cases have been added to cover the changes in this fix. This fix fixes #22420. Signed-off-by: Yong Tang --- mount/flags.go | 155 ++++++++++++++++++++++++++------------- mount/mount_unix_test.go | 23 ++++++ 2 files changed, 129 insertions(+), 49 deletions(-) diff --git a/mount/flags.go b/mount/flags.go index d2fb1fb..607dbed 100644 --- a/mount/flags.go +++ b/mount/flags.go @@ -5,6 +5,112 @@ import ( "strings" ) +var flags = map[string]struct { + clear bool + flag int +}{ + "defaults": {false, 0}, + "ro": {false, RDONLY}, + "rw": {true, RDONLY}, + "suid": {true, NOSUID}, + "nosuid": {false, NOSUID}, + "dev": {true, NODEV}, + "nodev": {false, NODEV}, + "exec": {true, NOEXEC}, + "noexec": {false, NOEXEC}, + "sync": {false, SYNCHRONOUS}, + "async": {true, SYNCHRONOUS}, + "dirsync": {false, DIRSYNC}, + "remount": {false, REMOUNT}, + "mand": {false, MANDLOCK}, + "nomand": {true, MANDLOCK}, + "atime": {true, NOATIME}, + "noatime": {false, NOATIME}, + "diratime": {true, NODIRATIME}, + "nodiratime": {false, NODIRATIME}, + "bind": {false, BIND}, + "rbind": {false, RBIND}, + "unbindable": {false, UNBINDABLE}, + "runbindable": {false, RUNBINDABLE}, + "private": {false, PRIVATE}, + "rprivate": {false, RPRIVATE}, + "shared": {false, SHARED}, + "rshared": {false, RSHARED}, + "slave": {false, SLAVE}, + "rslave": {false, RSLAVE}, + "relatime": {false, RELATIME}, + "norelatime": {true, RELATIME}, + "strictatime": {false, STRICTATIME}, + "nostrictatime": {true, STRICTATIME}, +} + +var validFlags = map[string]bool{ + "": true, + "size": true, + "mode": true, + "uid": true, + "gid": true, + "nr_inodes": true, + "nr_blocks": true, + "mpol": true, +} + +var propagationFlags = map[string]bool{ + "bind": true, + "rbind": true, + "unbindable": true, + "runbindable": true, + "private": true, + "rprivate": true, + "shared": true, + "rshared": true, + "slave": true, + "rslave": true, +} + +// MergeTmpfsOptions merge mount options to make sure there is no duplicate. +func MergeTmpfsOptions(options []string) ([]string, error) { + // We use collisions maps to remove duplicates. + // For flag, the key is the flag value (the key for propagation flag is -1) + // For data=value, the key is the data + flagCollisions := map[int]bool{} + dataCollisions := map[string]bool{} + + var newOptions []string + // We process in reverse order + for i := len(options) - 1; i >= 0; i-- { + option := options[i] + if option == "defaults" { + continue + } + if f, ok := flags[option]; ok && f.flag != 0 { + // There is only one propagation mode + key := f.flag + if propagationFlags[option] { + key = -1 + } + // Check to see if there is collision for flag + if !flagCollisions[key] { + // We prepend the option and add to collision map + newOptions = append([]string{option}, newOptions...) + flagCollisions[key] = true + } + continue + } + opt := strings.SplitN(option, "=", 2) + if len(opt) != 2 || !validFlags[opt[0]] { + return nil, fmt.Errorf("Invalid tmpfs option %q", opt) + } + if !dataCollisions[opt[0]] { + // We prepend the option and add to collision map + newOptions = append([]string{option}, newOptions...) + dataCollisions[opt[0]] = true + } + } + + return newOptions, nil +} + // Parse fstab type mount options into mount() flags // and device specific data func parseOptions(options string) (int, string) { @@ -13,45 +119,6 @@ func parseOptions(options string) (int, string) { data []string ) - flags := map[string]struct { - clear bool - flag int - }{ - "defaults": {false, 0}, - "ro": {false, RDONLY}, - "rw": {true, RDONLY}, - "suid": {true, NOSUID}, - "nosuid": {false, NOSUID}, - "dev": {true, NODEV}, - "nodev": {false, NODEV}, - "exec": {true, NOEXEC}, - "noexec": {false, NOEXEC}, - "sync": {false, SYNCHRONOUS}, - "async": {true, SYNCHRONOUS}, - "dirsync": {false, DIRSYNC}, - "remount": {false, REMOUNT}, - "mand": {false, MANDLOCK}, - "nomand": {true, MANDLOCK}, - "atime": {true, NOATIME}, - "noatime": {false, NOATIME}, - "diratime": {true, NODIRATIME}, - "nodiratime": {false, NODIRATIME}, - "bind": {false, BIND}, - "rbind": {false, RBIND}, - "unbindable": {false, UNBINDABLE}, - "runbindable": {false, RUNBINDABLE}, - "private": {false, PRIVATE}, - "rprivate": {false, RPRIVATE}, - "shared": {false, SHARED}, - "rshared": {false, RSHARED}, - "slave": {false, SLAVE}, - "rslave": {false, RSLAVE}, - "relatime": {false, RELATIME}, - "norelatime": {true, RELATIME}, - "strictatime": {false, STRICTATIME}, - "nostrictatime": {true, STRICTATIME}, - } - for _, o := range strings.Split(options, ",") { // If the option does not exist in the flags table or the flag // is not supported on the platform, @@ -72,16 +139,6 @@ func parseOptions(options string) (int, string) { // ParseTmpfsOptions parse fstab type mount options into flags and data func ParseTmpfsOptions(options string) (int, string, error) { flags, data := parseOptions(options) - validFlags := map[string]bool{ - "": true, - "size": true, - "mode": true, - "uid": true, - "gid": true, - "nr_inodes": true, - "nr_blocks": true, - "mpol": true, - } for _, o := range strings.Split(data, ",") { opt := strings.SplitN(o, "=", 2) if !validFlags[opt[0]] { diff --git a/mount/mount_unix_test.go b/mount/mount_unix_test.go index d45fbc1..90fa348 100644 --- a/mount/mount_unix_test.go +++ b/mount/mount_unix_test.go @@ -137,3 +137,26 @@ func TestGetMounts(t *testing.T) { t.Fatal("/ should be mounted at least") } } + +func TestMergeTmpfsOptions(t *testing.T) { + options := []string{"noatime", "ro", "size=10k", "defaults", "atime", "defaults", "rw", "rprivate", "size=1024k", "slave"} + expected := []string{"atime", "rw", "size=1024k", "slave"} + merged, err := MergeTmpfsOptions(options) + if err != nil { + t.Fatal(err) + } + if len(expected) != len(merged) { + t.Fatalf("Expected %s got %s", expected, merged) + } + for index := range merged { + if merged[index] != expected[index] { + t.Fatalf("Expected %s for the %dth option, got %s", expected, index, merged) + } + } + + options = []string{"noatime", "ro", "size=10k", "atime", "rw", "rprivate", "size=1024k", "slave", "size"} + _, err = MergeTmpfsOptions(options) + if err == nil { + t.Fatal("Expected error got nil") + } +}