Repository: prometheus/procfs
Pull request #242 (2019-11-25 22:50:31)

procfs: tidy up NetUnix APIs and internals
Signed-off-by: Matt Layher <mdlayher@gmail.com>

- remove unnecessary exported APIs; `FS.NetUnix` is sufficient and anything else is added API surface for little gain
- remove unecessary constants which are only used once (twice for error messages)
- remove duplication in tests and switch to go-cmp
- more idiomatic error messages


Patch:
diff --git a/net_unix.go b/net_unix.go
index 93bd58f..c55b4b1 100644
--- a/net_unix.go
+++ b/net_unix.go
@@ -15,7 +15,6 @@ package procfs
 
 import (
 	"bufio"
-	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -27,25 +26,15 @@ import (
 // see https://elixir.bootlin.com/linux/v4.17/source/net/unix/af_unix.c#L2815
 // and https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/net.h#L48.
 
-const (
-	netUnixKernelPtrIdx = iota
-	netUnixRefCountIdx
-	_
-	netUnixFlagsIdx
-	netUnixTypeIdx
-	netUnixStateIdx
-	netUnixInodeIdx
-
-	// Inode and Path are optional.
-	netUnixStaticFieldsCnt = 6
-)
-
+// Constants for the various /proc/net/unix enumerations.
+// TODO: match against x/sys/unix or similar?
 const (
 	netUnixTypeStream    = 1
 	netUnixTypeDgram     = 2
 	netUnixTypeSeqpacket = 5
 
-	netUnixFlagListen = 1 << 16
+	netUnixFlagDefault = 0
+	netUnixFlagListen  = 1 << 16
 
 	netUnixStateUnconnected  = 1
 	netUnixStateConnecting   = 2
@@ -53,129 +42,127 @@ const (
 	netUnixStateDisconnected = 4
 )
 
-var errInvalidKernelPtrFmt = errors.New("Invalid Num(the kernel table slot number) format")
+// NetUNIXType is the type of the type field.
+type NetUNIXType uint64
 
-// NetUnixType is the type of the type field.
-type NetUnixType uint64
+// NetUNIXFlags is the type of the flags field.
+type NetUNIXFlags uint64
 
-// NetUnixFlags is the type of the flags field.
-type NetUnixFlags uint64
+// NetUNIXState is the type of the state field.
+type NetUNIXState uint64
 
-// NetUnixState is the type of the state field.
-type NetUnixState uint64
-
-// NetUnixLine represents a line of /proc/net/unix.
-type NetUnixLine struct {
+// NetUNIXLine represents a line of /proc/net/unix.
+type NetUNIXLine struct {
 	KernelPtr string
 	RefCount  uint64
 	Protocol  uint64
-	Flags     NetUnixFlags
-	Type      NetUnixType
-	State     NetUnixState
+	Flags     NetUNIXFlags
+	Type      NetUNIXType
+	State     NetUNIXState
 	Inode     uint64
 	Path      string
 }
 
-// NetUnix holds the data read from /proc/net/unix.
-type NetUnix struct {
-	Rows []*NetUnixLine
+// NetUNIX holds the data read from /proc/net/unix.
+type NetUNIX struct {
+	Rows []*NetUNIXLine
 }
 
-// NewNetUnix returns data read from /proc/net/unix.
-func NewNetUnix() (*NetUnix, error) {
-	fs, err := NewFS(DefaultMountPoint)
-	if err != nil {
-		return nil, err
-	}
-
-	return fs.NewNetUnix()
+// NetUNIX returns data read from /proc/net/unix.
+func (fs FS) NetUNIX() (*NetUNIX, error) {
+	return readNetUNIX(fs.proc.Path("net/unix"))
 }
 
-// NewNetUnix returns data read from /proc/net/unix.
-func (fs FS) NewNetUnix() (*NetUnix, error) {
-	return NewNetUnixByPath(fs.proc.Path("net/unix"))
-}
-
-// NewNetUnixByPath returns data read from /proc/net/unix by file path.
-// It might returns an error with partial parsed data, if an error occur after some data parsed.
-func NewNetUnixByPath(path string) (*NetUnix, error) {
-	f, err := os.Open(path)
+// readNetUNIX reads data in /proc/net/unix format from the specified file.
+func readNetUNIX(file string) (*NetUNIX, error) {
+	// This file could be quite large and a streaming read is desirable versus
+	// reading the entire contents at once.
+	f, err := os.Open(file)
 	if err != nil {
 		return nil, err
 	}
 	defer f.Close()
-	return NewNetUnixByReader(f)
+
+	return parseNetUNIX(f)
 }
 
-// NewNetUnixByReader returns data read from /proc/net/unix by a reader.
-// It might returns an error with partial parsed data, if an error occur after some data parsed.
-func NewNetUnixByReader(reader io.Reader) (*NetUnix, error) {
-	nu := &NetUnix{
-		Rows: make([]*NetUnixLine, 0, 32),
-	}
-	scanner := bufio.NewScanner(reader)
-	// Omit the header line.
-	scanner.Scan()
-	header := scanner.Text()
+// parseNetUNIX creates a NetUnix structure from the incoming stream.
+func parseNetUNIX(r io.Reader) (*NetUNIX, error) {
+	// Begin scanning by checking for the existence of Inode.
+	s := bufio.NewScanner(r)
+	s.Scan()
+
 	// From the man page of proc(5), it does not contain an Inode field,
-	// but in actually it exists.
-	// This code works for both cases.
-	hasInode := strings.Contains(header, "Inode")
+	// but in actually it exists. This code works for both cases.
+	hasInode := strings.Contains(s.Text(), "Inode")
 
-	minFieldsCnt := netUnixStaticFieldsCnt
+	// Expect a minimum number of fields, but Inode and Path are optional:
+	// Num       RefCount Protocol Flags    Type St Inode Path
+	minFields := 6
 	if hasInode {
-		minFieldsCnt++
+		minFields++
 	}
-	for scanner.Scan() {
-		line := scanner.Text()
-		item, err := nu.parseLine(line, hasInode, minFieldsCnt)
+
+	var nu NetUNIX
+	for s.Scan() {
+		line := s.Text()
+		item, err := nu.parseLine(line, hasInode, minFields)
 		if err != nil {
-			return nu, err
+			return nil, fmt.Errorf("failed to parse /proc/net/unix data %q: %v", line, err)
 		}
+
 		nu.Rows = append(nu.Rows, item)
 	}
 
-	return nu, scanner.Err()
+	if err := s.Err(); err != nil {
+		return nil, fmt.Errorf("failed to scan /proc/net/unix data: %v", err)
+	}
+
+	return &nu, nil
 }
 
-func (u *NetUnix) parseLine(line string, hasInode bool, minFieldsCnt int) (*NetUnixLine, error) {
+func (u *NetUNIX) parseLine(line string, hasInode bool, min int) (*NetUNIXLine, error) {
 	fields := strings.Fields(line)
-	fieldsLen := len(fields)
-	if fieldsLen < minFieldsCnt {
-		return nil, fmt.Errorf(
-			"Parse Unix domain failed: expect at least %d fields but got %d",
-			minFieldsCnt, fieldsLen)
-	}
-	kernelPtr, err := u.parseKernelPtr(fields[netUnixKernelPtrIdx])
-	if err != nil {
-		return nil, fmt.Errorf("Parse Unix domain num(%s) failed: %s", fields[netUnixKernelPtrIdx], err)
+
+	l := len(fields)
+	if l < min {
+		return nil, fmt.Errorf("expected at least %d fields but got %d", min, l)
 	}
-	users, err := u.parseUsers(fields[netUnixRefCountIdx])
+
+	// Field offsets are as follows:
+	// Num       RefCount Protocol Flags    Type St Inode Path
+
+	kernelPtr := strings.TrimSuffix(fields[0], ":")
+
+	users, err := u.parseUsers(fields[1])
 	if err != nil {
-		return nil, fmt.Errorf("Parse Unix domain ref count(%s) failed: %s", fields[netUnixRefCountIdx], err)
+		return nil, fmt.Errorf("failed to parse ref count(%s): %v", fields[1], err)
 	}
-	flags, err := u.parseFlags(fields[netUnixFlagsIdx])
+
+	flags, err := u.parseFlags(fields[3])
 	if err != nil {
-		return nil, fmt.Errorf("Parse Unix domain flags(%s) failed: %s", fields[netUnixFlagsIdx], err)
+		return nil, fmt.Errorf("failed to parse flags(%s): %v", fields[3], err)
 	}
-	typ, err := u.parseType(fields[netUnixTypeIdx])
+
+	typ, err := u.parseType(fields[4])
 	if err != nil {
-		return nil, fmt.Errorf("Parse Unix domain type(%s) failed: %s", fields[netUnixTypeIdx], err)
+		return nil, fmt.Errorf("failed to parse type(%s): %v", fields[4], err)
 	}
-	state, err := u.parseState(fields[netUnixStateIdx])
+
+	state, err := u.parseState(fields[5])
 	if err != nil {
-		return nil, fmt.Errorf("Parse Unix domain state(%s) failed: %s", fields[netUnixStateIdx], err)
+		return nil, fmt.Errorf("failed to parse state(%s): %v", fields[5], err)
 	}
+
 	var inode uint64
 	if hasInode {
-		inodeStr := fields[netUnixInodeIdx]
-		inode, err = u.parseInode(inodeStr)
+		inode, err = u.parseInode(fields[6])
 		if err != nil {
-			return nil, fmt.Errorf("Parse Unix domain inode(%s) failed: %s", inodeStr, err)
+			return nil, fmt.Errorf("failed to parse inode(%s): %v", fields[6], err)
 		}
 	}
 
-	nuLine := &NetUnixLine{
+	n := &NetUNIXLine{
 		KernelPtr: kernelPtr,
 		RefCount:  users,
 		Type:      typ,
@@ -185,57 +172,56 @@ func (u *NetUnix) parseLine(line string, hasInode bool, minFieldsCnt int) (*NetU
 	}
 
 	// Path field is optional.
-	if fieldsLen > minFieldsCnt {
-		pathIdx := netUnixInodeIdx + 1
+	if l > min {
+		// Path occurs at either index 6 or 7 depending on whether inode is
+		// already present.
+		pathIdx := 7
 		if !hasInode {
 			pathIdx--
 		}
-		nuLine.Path = fields[pathIdx]
-	}
-
-	return nuLine, nil
-}
 
-func (u NetUnix) parseKernelPtr(str string) (string, error) {
-	if !strings.HasSuffix(str, ":") {
-		return "", errInvalidKernelPtrFmt
+		n.Path = fields[pathIdx]
 	}
-	return str[:len(str)-1], nil
+
+	return n, nil
 }
 
-func (u NetUnix) parseUsers(hexStr string) (uint64, error) {
-	return strconv.ParseUint(hexStr, 16, 32)
+func (u NetUNIX) parseUsers(s string) (uint64, error) {
+	return strconv.ParseUint(s, 16, 32)
 }
 
-func (u NetUnix) parseType(hexStr string) (NetUnixType, error) {
-	typ, err := strconv.ParseUint(hexStr, 16, 16)
+func (u NetUNIX) parseType(s string) (NetUNIXType, error) {
+	typ, err := strconv.ParseUint(s, 16, 16)
 	if err != nil {
 		return 0, err
 	}
-	return NetUnixType(typ), nil
+
+	return NetUNIXType(typ), nil
 }
 
-func (u NetUnix) parseFlags(hexStr string) (NetUnixFlags, error) {
-	flags, err := strconv.ParseUint(hexStr, 16, 32)
+func (u NetUNIX) parseFlags(s string) (NetUNIXFlags, error) {
+	flags, err := strconv.ParseUint(s, 16, 32)
 	if err != nil {
 		return 0, err
 	}
-	return NetUnixFlags(flags), nil
+
+	return NetUNIXFlags(flags), nil
 }
 
-func (u NetUnix) parseState(hexStr string) (NetUnixState, error) {
-	st, err := strconv.ParseInt(hexStr, 16, 8)
+func (u NetUNIX) parseState(s string) (NetUNIXState, error) {
+	st, err := strconv.ParseInt(s, 16, 8)
 	if err != nil {
 		return 0, err
 	}
-	return NetUnixState(st), nil
+
+	return NetUNIXState(st), nil
 }
 
-func (u NetUnix) parseInode(inodeStr string) (uint64, error) {
-	return strconv.ParseUint(inodeStr, 10, 64)
+func (u NetUNIX) parseInode(s string) (uint64, error) {
+	return strconv.ParseUint(s, 10, 64)
 }
 
-func (t NetUnixType) String() string {
+func (t NetUNIXType) String() string {
 	switch t {
 	case netUnixTypeStream:
 		return "stream"
@@ -247,7 +233,7 @@ func (t NetUnixType) String() string {
 	return "unknown"
 }
 
-func (f NetUnixFlags) String() string {
+func (f NetUNIXFlags) String() string {
 	switch f {
 	case netUnixFlagListen:
 		return "listen"
@@ -256,7 +242,7 @@ func (f NetUnixFlags) String() string {
 	}
 }
 
-func (s NetUnixState) String() string {
+func (s NetUNIXState) String() string {
 	switch s {
 	case netUnixStateUnconnected:
 		return "unconnected"


Test patch:
diff --git a/net_unix_test.go b/net_unix_test.go
index 4643ced..2de59bb 100644
--- a/net_unix_test.go
+++ b/net_unix_test.go
@@ -15,21 +15,51 @@ package procfs
 
 import (
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+// Whether or not to run tests with inode fixtures.
+const (
+	checkInode   = true
+	noCheckInode = false
 )
 
-func TestNewNetUnix(t *testing.T) {
+func TestNetUnix(t *testing.T) {
+	fs, err := NewFS(procTestFixtures)
+	if err != nil {
+		t.Fatalf("failed to open procfs: %v", err)
+	}
+
+	got, err := fs.NetUNIX()
+	if err != nil {
+		t.Fatalf("failed to get UNIX socket data: %v", err)
+	}
+
+	testNetUNIX(t, checkInode, got)
+}
+
+func TestNetUnixNoInode(t *testing.T) {
 	fs, err := NewFS(procTestFixtures)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("failed to open procfs: %v", err)
 	}
 
-	nu, err := fs.NewNetUnix()
+	got, err := readNetUNIX(fs.proc.Path("net/unix_without_inode"))
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("failed to read UNIX socket data: %v", err)
 	}
 
-	lines := []*NetUnixLine{
-		&NetUnixLine{
+	testNetUNIX(t, noCheckInode, got)
+}
+
+func testNetUNIX(t *testing.T, testInode bool, got *NetUNIX) {
+	t.Helper()
+
+	// First verify that the input data matches a prepopulated structure.
+
+	want := []*NetUNIXLine{
+		{
 			KernelPtr: "0000000000000000",
 			RefCount:  2,
 			Flags:     1 << 16,
@@ -38,7 +68,7 @@ func TestNewNetUnix(t *testing.T) {
 			Inode:     3442596,
 			Path:      "/var/run/postgresql/.s.PGSQL.5432",
 		},
-		&NetUnixLine{
+		{
 			KernelPtr: "0000000000000000",
 			RefCount:  10,
 			Flags:     1 << 16,
@@ -47,7 +77,7 @@ func TestNewNetUnix(t *testing.T) {
 			Inode:     10061,
 			Path:      "/run/udev/control",
 		},
-		&NetUnixLine{
+		{
 			KernelPtr: "0000000000000000",
 			RefCount:  7,
 			Flags:     0,
@@ -56,7 +86,7 @@ func TestNewNetUnix(t *testing.T) {
 			Inode:     12392,
 			Path:      "/dev/log",
 		},
-		&NetUnixLine{
+		{
 			KernelPtr: "0000000000000000",
 			RefCount:  3,
 			Flags:     0,
@@ -65,7 +95,7 @@ func TestNewNetUnix(t *testing.T) {
 			Inode:     4787297,
 			Path:      "/var/run/postgresql/.s.PGSQL.5432",
 		},
-		&NetUnixLine{
+		{
 			KernelPtr: "0000000000000000",
 			RefCount:  3,
 			Flags:     0,
@@ -75,105 +105,66 @@ func TestNewNetUnix(t *testing.T) {
 		},
 	}
 
-	if want, have := len(lines), len(nu.Rows); want != have {
-		t.Errorf("want %d parsed net/unix lines, have %d", want, have)
-	}
-	for i, gotLine := range nu.Rows {
-		if i >= len(lines) {
-			continue
-		}
-		line := lines[i]
-		if *line != *gotLine {
-			t.Errorf("%d item: got %v, want %v", i, *gotLine, *line)
+	// Enable the fixtures to be used for multiple tests by clearing the inode
+	// field when appropriate.
+	if !testInode {
+		for i := 0; i < len(want); i++ {
+			want[i].Inode = 0
 		}
 	}
 
-	wantedFlags := "listen"
-	flags := lines[0].Flags.String()
-	if wantedFlags != flags {
-		t.Errorf("unexpected flag str: want %s, got %s", wantedFlags, flags)
-	}
-	wantedFlags = "default"
-	flags = lines[3].Flags.String()
-	if wantedFlags != flags {
-		t.Errorf("unexpected flag str: wanted %s, got %s", wantedFlags, flags)
+	if diff := cmp.Diff(want, got.Rows); diff != "" {
+		t.Fatalf("unexpected /proc/net/unix data (-want +got):\n%s", diff)
 	}
 
-	wantedType := "stream"
-	typ := lines[0].Type.String()
-	if wantedType != typ {
-		t.Errorf("unexpected type str: wanted %s, got %s", wantedType, typ)
+	// Now test the field enumerations and ensure they match up correctly
+	// with the constants used to generate readable strings.
+
+	wantFlags := []NetUNIXFlags{
+		netUnixFlagListen,
+		netUnixFlagListen,
+		netUnixFlagDefault,
+		netUnixFlagDefault,
+		netUnixFlagDefault,
 	}
 
-	wantedType = "seqpacket"
-	typ = lines[1].Type.String()
-	if wantedType != typ {
-		t.Errorf("unexpected type str: want %s, got %s", wantedType, typ)
+	wantType := []NetUNIXType{
+		netUnixTypeStream,
+		netUnixTypeSeqpacket,
+		netUnixTypeDgram,
+		netUnixTypeStream,
+		netUnixTypeStream,
 	}
-}
 
-func TestNewNetUnixWithoutInode(t *testing.T) {
-	fs, err := NewFS(procTestFixtures)
-	if err != nil {
-		t.Fatal(err)
+	wantState := []NetUNIXState{
+		netUnixStateUnconnected,
+		netUnixStateUnconnected,
+		netUnixStateUnconnected,
+		netUnixStateConnected,
+		netUnixStateConnected,
 	}
-	nu, err := NewNetUnixByPath(fs.proc.Path("net/unix_without_inode"))
-	if err != nil {
-		t.Fatal(err)
+
+	var (
+		gotFlags []NetUNIXFlags
+		gotType  []NetUNIXType
+		gotState []NetUNIXState
+	)
+
+	for _, r := range got.Rows {
+		gotFlags = append(gotFlags, r.Flags)
+		gotType = append(gotType, r.Type)
+		gotState = append(gotState, r.State)
 	}
 
-	lines := []*NetUnixLine{
-		&NetUnixLine{
-			KernelPtr: "0000000000000000",
-			RefCount:  2,
-			Flags:     1 << 16,
-			Type:      1,
-			State:     1,
-			Path:      "/var/run/postgresql/.s.PGSQL.5432",
-		},
-		&NetUnixLine{
-			KernelPtr: "0000000000000000",
-			RefCount:  10,
-			Flags:     1 << 16,
-			Type:      5,
-			State:     1,
-			Path:      "/run/udev/control",
-		},
-		&NetUnixLine{
-			KernelPtr: "0000000000000000",
-			RefCount:  7,
-			Flags:     0,
-			Type:      2,
-			State:     1,
-			Path:      "/dev/log",
-		},
-		&NetUnixLine{
-			KernelPtr: "0000000000000000",
-			RefCount:  3,
-			Flags:     0,
-			Type:      1,
-			State:     3,
-			Path:      "/var/run/postgresql/.s.PGSQL.5432",
-		},
-		&NetUnixLine{
-			KernelPtr: "0000000000000000",
-			RefCount:  3,
-			Flags:     0,
-			Type:      1,
-			State:     3,
-		},
+	if diff := cmp.Diff(wantFlags, gotFlags); diff != "" {
+		t.Fatalf("unexpected /proc/net/unix flags (-want +got):\n%s", diff)
 	}
 
-	if want, have := len(lines), len(nu.Rows); want != have {
-		t.Errorf("want %d parsed net/unix lines, have %d", want, have)
+	if diff := cmp.Diff(wantType, gotType); diff != "" {
+		t.Fatalf("unexpected /proc/net/unix types (-want +got):\n%s", diff)
 	}
-	for i, gotLine := range nu.Rows {
-		if i >= len(lines) {
-			continue
-		}
-		line := lines[i]
-		if *line != *gotLine {
-			t.Errorf("%d item: got %v, want %v", i, *gotLine, *line)
-		}
+
+	if diff := cmp.Diff(wantState, gotState); diff != "" {
+		t.Fatalf("unexpected /proc/net/unix states (-want +got):\n%s", diff)
 	}
 }