Repository: prometheus/procfs
Pull request #228 (2019-10-25 15:42:50)

Update mountinfo parsing
* Use a regexp to parse the mountinfo lines.
* Use util.ReadFileNoStat() to instead of a buffered reader.
* Add a test for hypens in the mountpoint.

Fixes: https://github.com/prometheus/procfs/issues/225

Signed-off-by: Ben Kochie <superq@gmail.com>

discordianfish: Hrm not sure that using a regex is the best way. How do the usual linux tools parse this? I'd assume there is some regex-free way to do that. If not, this is fine.
SuperQ: The only code I've found so far was some random Perl code. It splits on spaces, which might be a reasonable way to go. But I thought the regexp would be a little better for validating the contents of the various sections.
discordianfish: I'd argue we don't want to validate this at all.

See: http://man7.org/linux/man-pages/man5/proc.5.html
I think the right way to parse this is:
- split by space
- store elements 1-6 individually
- start looping over element 7+ until you find a -
- store remaining elements individually
SuperQ: Ok, I'll re-work this.
SuperQ: @discordianfish Ok, now it's a bunch of much simpler offsets and loops. Thanks for the suggestion.
SuperQ: I cleaned up a couple more things, I feel like the optional field parsing is now easier to understand, and less buggy.
discordianfish: Beside that, LGTM
SuperQ: Ok, I returned the optional field check to ignore not fail.

Patch:
diff --git a/mountinfo.go b/mountinfo.go
index 61fa618..bb01bb5 100644
--- a/mountinfo.go
+++ b/mountinfo.go
@@ -15,19 +15,13 @@ package procfs
 
 import (
 	"bufio"
+	"bytes"
 	"fmt"
-	"io"
-	"os"
 	"strconv"
 	"strings"
-)
 
-var validOptionalFields = map[string]bool{
-	"shared":         true,
-	"master":         true,
-	"propagate_from": true,
-	"unbindable":     true,
-}
+	"github.com/prometheus/procfs/internal/util"
+)
 
 // A MountInfo is a type that describes the details, options
 // for each mount, parsed from /proc/self/mountinfo.
@@ -58,18 +52,10 @@ type MountInfo struct {
 	SuperOptions map[string]string
 }
 
-// Returns part of the mountinfo line, if it exists, else an empty string.
-func getStringSliceElement(parts []string, idx int, defaultValue string) string {
-	if idx >= len(parts) {
-		return defaultValue
-	}
-	return parts[idx]
-}
-
 // Reads each line of the mountinfo file, and returns a list of formatted MountInfo structs.
-func parseMountInfo(r io.Reader) ([]*MountInfo, error) {
+func parseMountInfo(info []byte) ([]*MountInfo, error) {
 	mounts := []*MountInfo{}
-	scanner := bufio.NewScanner(r)
+	scanner := bufio.NewScanner(bytes.NewReader(info))
 	for scanner.Scan() {
 		mountString := scanner.Text()
 		parsedMounts, err := parseMountInfoString(mountString)
@@ -89,57 +75,75 @@ func parseMountInfo(r io.Reader) ([]*MountInfo, error) {
 func parseMountInfoString(mountString string) (*MountInfo, error) {
 	var err error
 
-	// OptionalFields can be zero, hence these checks to ensure we do not populate the wrong values in the wrong spots
-	separatorIndex := strings.Index(mountString, "-")
-	if separatorIndex == -1 {
-		return nil, fmt.Errorf("no separator found in mountinfo string: %s", mountString)
+	mountInfo := strings.Split(mountString, " ")
+	mountInfoLength := len(mountInfo)
+	if mountInfoLength < 11 {
+		return nil, fmt.Errorf("couldn't find enough fields in mount string: %s", mountString)
 	}
-	beforeFields := strings.Fields(mountString[:separatorIndex])
-	afterFields := strings.Fields(mountString[separatorIndex+1:])
-	if (len(beforeFields) + len(afterFields)) < 7 {
-		return nil, fmt.Errorf("too few fields")
+
+	if mountInfo[mountInfoLength-4] != "-" {
+		return nil, fmt.Errorf("couldn't find separator in expected field: %s", mountInfo[mountInfoLength-4])
 	}
 
 	mount := &MountInfo{
-		MajorMinorVer:  getStringSliceElement(beforeFields, 2, ""),
-		Root:           getStringSliceElement(beforeFields, 3, ""),
-		MountPoint:     getStringSliceElement(beforeFields, 4, ""),
-		Options:        mountOptionsParser(getStringSliceElement(beforeFields, 5, "")),
+		MajorMinorVer:  mountInfo[2],
+		Root:           mountInfo[3],
+		MountPoint:     mountInfo[4],
+		Options:        mountOptionsParser(mountInfo[5]),
 		OptionalFields: nil,
-		FSType:         getStringSliceElement(afterFields, 0, ""),
-		Source:         getStringSliceElement(afterFields, 1, ""),
-		SuperOptions:   mountOptionsParser(getStringSliceElement(afterFields, 2, "")),
+		FSType:         mountInfo[mountInfoLength-3],
+		Source:         mountInfo[mountInfoLength-2],
+		SuperOptions:   mountOptionsParser(mountInfo[mountInfoLength-1]),
 	}
 
-	mount.MountId, err = strconv.Atoi(getStringSliceElement(beforeFields, 0, ""))
+	mount.MountId, err = strconv.Atoi(mountInfo[0])
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse mount ID")
 	}
-	mount.ParentId, err = strconv.Atoi(getStringSliceElement(beforeFields, 1, ""))
+	mount.ParentId, err = strconv.Atoi(mountInfo[1])
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse parent ID")
 	}
 	// Has optional fields, which is a space separated list of values.
 	// Example: shared:2 master:7
-	if len(beforeFields) > 6 {
-		mount.OptionalFields = make(map[string]string)
-		optionalFields := beforeFields[6:]
-		for _, field := range optionalFields {
-			optionSplit := strings.Split(field, ":")
-			target, value := optionSplit[0], ""
-			if len(optionSplit) == 2 {
-				value = optionSplit[1]
-			}
-			// Checks if the 'keys' in the optional fields in the mountinfo line are acceptable.
-			// Allowed 'keys' are shared, master, propagate_from, unbindable.
-			if _, ok := validOptionalFields[target]; ok {
-				mount.OptionalFields[target] = value
-			}
+	if mountInfo[6] != "" {
+		mount.OptionalFields, err = mountOptionsParseOptionalFields(mountInfo[6 : mountInfoLength-4])
+		if err != nil {
+			return nil, err
 		}
 	}
 	return mount, nil
 }
 
+// mountOptionsIsValidField checks a string against a valid list of optional fields keys.
+func mountOptionsIsValidField(s string) bool {
+	switch s {
+	case
+		"shared",
+		"master",
+		"propagate_from",
+		"unbindable":
+		return true
+	}
+	return false
+}
+
+// mountOptionsParseOptionalFields parses a list of optional fields strings into a double map of strings.
+func mountOptionsParseOptionalFields(o []string) (map[string]string, error) {
+	optionalFields := make(map[string]string)
+	for _, field := range o {
+		optionSplit := strings.SplitN(field, ":", 2)
+		value := ""
+		if len(optionSplit) == 2 {
+			value = optionSplit[1]
+		}
+		if mountOptionsIsValidField(optionSplit[0]) {
+			optionalFields[optionSplit[0]] = value
+		}
+	}
+	return optionalFields, nil
+}
+
 // Parses the mount options, superblock options.
 func mountOptionsParser(mountOptions string) map[string]string {
 	opts := make(map[string]string)
@@ -159,20 +163,18 @@ func mountOptionsParser(mountOptions string) map[string]string {
 
 // Retrieves mountinfo information from `/proc/self/mountinfo`.
 func GetMounts() ([]*MountInfo, error) {
-	f, err := os.Open("/proc/self/mountinfo")
+	data, err := util.ReadFileNoStat("/proc/self/mountinfo")
 	if err != nil {
 		return nil, err
 	}
-	defer f.Close()
-	return parseMountInfo(f)
+	return parseMountInfo(data)
 }
 
 // Retrieves mountinfo information from a processes' `/proc/<pid>/mountinfo`.
 func GetProcMounts(pid int) ([]*MountInfo, error) {
-	f, err := os.Open(fmt.Sprintf("/proc/%d/mountinfo", pid))
+	data, err := util.ReadFileNoStat(fmt.Sprintf("/proc/%d/mountinfo", pid))
 	if err != nil {
 		return nil, err
 	}
-	defer f.Close()
-	return parseMountInfo(f)
+	return parseMountInfo(data)
 }
diff --git a/proc.go b/proc.go
index bde17b3..330e472 100644
--- a/proc.go
+++ b/proc.go
@@ -241,13 +241,11 @@ func (p Proc) MountStats() ([]*Mount, error) {
 // It supplies information missing in `/proc/self/mounts` and
 // fixes various other problems with that file too.
 func (p Proc) MountInfo() ([]*MountInfo, error) {
-	f, err := os.Open(p.path("mountinfo"))
+	data, err := util.ReadFileNoStat(p.path("mountinfo"))
 	if err != nil {
 		return nil, err
 	}
-	defer f.Close()
-
-	return parseMountInfo(f)
+	return parseMountInfo(data)
 }
 
 func (p Proc) fileDescriptors() ([]string, error) {


Test patch:
diff --git a/mountinfo_test.go b/mountinfo_test.go
index 3565979..86fe85e 100644
--- a/mountinfo_test.go
+++ b/mountinfo_test.go
@@ -114,6 +114,23 @@ func TestMountInfo(t *testing.T) {
 			},
 			invalid: false,
 		},
+		{
+			name: "CIFS mounted at /with/a-hyphen",
+			s:    "454 29 0:87 / /with/a-hyphen rw,relatime shared:255 - cifs //remote-storage/Path rw,vers=3.1.1,cache=strict,username=user,uid=1000,forceuid,gid=0,noforcegid,addr=127.0.0.1,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,echo_interval=60,actimeo=1",
+			mount: &MountInfo{
+				MountId:        454,
+				ParentId:       29,
+				MajorMinorVer:  "0:87",
+				Root:           "/",
+				MountPoint:     "/with/a-hyphen",
+				Options:        map[string]string{"rw": "", "relatime": ""},
+				OptionalFields: map[string]string{"shared": "255"},
+				FSType:         "cifs",
+				Source:         "//remote-storage/Path",
+				SuperOptions:   map[string]string{"rw": "", "vers": "3.1.1", "cache": "strict", "username": "user", "uid": "1000", "forceuid": "", "gid": "0", "noforcegid": "", "addr": "127.0.0.1", "file_mode": "0755", "dir_mode": "0755", "soft": "", "nounix": "", "serverino": "", "mapposix": "", "echo_interval": "60", "actimeo": "1"},
+			},
+			invalid: false,
+		},
 	}
 
 	for i, test := range tests {