From b331c4c45e4bc1cc445b6aa3e973eaedad84ebd1 Mon Sep 17 00:00:00 2001 From: Karim Kanso Date: Wed, 11 Mar 2020 09:17:55 +0000 Subject: [PATCH] Removed use of ioutil.ReadDir as it performs a stat syscall on each file which is not necerssary. --- internal/psscanner/proclist.go | 41 +++-- internal/psscanner/proclist_test.go | 218 +++++++++++++++++---------- internal/psscanner/psscanner_test.go | 2 +- 3 files changed, 157 insertions(+), 104 deletions(-) diff --git a/internal/psscanner/proclist.go b/internal/psscanner/proclist.go index 7211c69..ea6c316 100644 --- a/internal/psscanner/proclist.go +++ b/internal/psscanner/proclist.go @@ -1,17 +1,12 @@ package psscanner import ( - "errors" "fmt" - "io/ioutil" + "io" "os" "strconv" ) -var procDirReader = func() ([]os.FileInfo, error) { - return ioutil.ReadDir("/proc") -} - type procList map[int]struct{} type pidProcessor interface { @@ -37,15 +32,21 @@ func (pl procList) refresh(p pidProcessor) error { } func getPIDs() ([]int, error) { - proc, err := procDirReader() + f, err := dirOpen("/proc") if err != nil { return nil, fmt.Errorf("opening proc dir: %v", err) } + defer f.Close() + + names, err := f.Readdirnames(-1) + if err != nil { + return nil, fmt.Errorf("reading proc dir: %v", err) + } pids := make([]int, 0) - for _, f := range proc { - pid, err := file2Pid(f) - if err != nil { + for _, f := range names { + pid, err := strconv.Atoi(f) + if err != nil || pid <= 0 { continue } pids = append(pids, pid) @@ -53,17 +54,11 @@ func getPIDs() ([]int, error) { return pids, nil } -var errNotAPid = errors.New("not a pid") - -func file2Pid(f os.FileInfo) (int, error) { - if !f.IsDir() { - return -1, errNotAPid - } - - pid, err := strconv.Atoi(f.Name()) - if err != nil || pid <= 0 { - return -1, errNotAPid - } - - return pid, nil +type readDirNamesCloser interface { + Readdirnames(n int) (names []string, err error) + io.Closer +} + +var dirOpen func(string) (readDirNamesCloser, error) = func(s string) (readDirNamesCloser, error) { + return os.Open(s) } diff --git a/internal/psscanner/proclist_test.go b/internal/psscanner/proclist_test.go index 0a147d2..4ec0881 100644 --- a/internal/psscanner/proclist_test.go +++ b/internal/psscanner/proclist_test.go @@ -3,90 +3,129 @@ package psscanner import ( "errors" "fmt" - "os" "reflect" "strings" "testing" - "time" ) // GetPIDs func TestGetPIDs(t *testing.T) { tests := []struct { - proc []os.FileInfo - procErr error - pids []int - err string + name string + proc []string + procErrOpen error + procErrRead error + pids []int + err string }{ - {proc: []os.FileInfo{newMockDir("42"), newMockDir("somedir")}, procErr: nil, pids: []int{42}, err: ""}, // reads numbers and ignores everything else - {proc: []os.FileInfo{newMockDir("42"), newMockFile("13")}, procErr: nil, pids: []int{42}, err: ""}, // reads directories and ignores files - {proc: []os.FileInfo{newMockDir("0"), newMockDir("-1")}, procErr: nil, pids: []int{}, err: ""}, // ignores 0 and negative numbers - {proc: []os.FileInfo{}, procErr: nil, pids: []int{}, err: ""}, // can handle empty procfs - {proc: []os.FileInfo{}, procErr: errors.New("file-system-error"), pids: nil, err: "opening proc dir: file-system-error"}, // returns errors + { + name: "numbers-only", + proc: []string{"42", "somedir"}, + procErrOpen: nil, + procErrRead: nil, + pids: []int{42}, + err: "", + }, + { + name: "multiple-entries", + proc: []string{"42", "13"}, + procErrOpen: nil, + procErrRead: nil, + pids: []int{42, 13}, + err: "", + }, + { + name: "ignores-lte-0", + proc: []string{"0", "-1"}, + procErrOpen: nil, + procErrRead: nil, + pids: []int{}, + err: "", + }, + { + name: "empty-procfs", + proc: []string{}, + procErrOpen: nil, + procErrRead: nil, + pids: []int{}, + err: "", + }, + { + name: "handle-open-error", + proc: []string{}, + procErrOpen: errors.New("file-system-error"), + procErrRead: nil, + pids: nil, + err: "opening proc dir: file-system-error", + }, + { + name: "handle-read-error", + proc: []string{}, + procErrOpen: nil, + procErrRead: errors.New("file-system-error"), + pids: nil, + err: "reading proc dir: file-system-error", + }, } for _, tt := range tests { - restore := mockProcDirReader(tt.proc, tt.procErr) - pids, err := getPIDs() - if !reflect.DeepEqual(pids, tt.pids) { - t.Errorf("Wrong pids returned: got %v but want %v", pids, tt.pids) - } - if (err != nil || tt.err != "") && fmt.Sprintf("%v", err) != tt.err { - t.Errorf("Wrong error returned: got %v but want %s", err, tt.err) - } - restore() + t.Run(tt.name, func(t *testing.T) { + defer mockDir("/proc", tt.proc, tt.procErrRead, tt.procErrOpen, t)() + pids, err := getPIDs() + if !reflect.DeepEqual(pids, tt.pids) { + t.Errorf("Wrong pids returned: got %v but want %v", pids, tt.pids) + } + if (err != nil || tt.err != "") && fmt.Sprintf("%v", err) != tt.err { + t.Errorf("Wrong error returned: got %v but want %s", err, tt.err) + } + }) } } -func mockProcDirReader(proc []os.FileInfo, err error) (restore func()) { - oldFunc := procDirReader - procDirReader = func() ([]os.FileInfo, error) { - return proc, err +type MockDir struct { + names []string + err error +} + +func (f *MockDir) Close() error { + return nil +} + +func min(a, b int) int { + if a > b { + return b + } + return a +} + +func (f *MockDir) Readdirnames(n int) (names []string, err error) { + if n < 0 { + return f.names, f.err + } + return f.names[:min(n, len(f.names))], f.err +} + +// Hook/chain a mocked file into the "open" variable +func mockDir(name string, names []string, errRead error, errOpen error, t *testing.T) func() { + oldopen := dirOpen + dirOpen = func(n string) (readDirNamesCloser, error) { + if name == n { + if testing.Verbose() { + t.Logf("opening mocked dir: %s", n) + } + return &MockDir{ + names: names, + err: errRead, + }, errOpen + } + return oldopen(n) } return func() { - procDirReader = oldFunc + dirOpen = oldopen } } -func newMockDir(name string) *mockFileInfo { - return &mockFileInfo{ - name: name, - isDir: true, - } -} - -func newMockFile(name string) *mockFileInfo { - return &mockFileInfo{ - name: name, - isDir: false, - } -} - -type mockFileInfo struct { - name string - isDir bool -} - -func (f *mockFileInfo) Name() string { - return f.name -} -func (f *mockFileInfo) Size() int64 { - return 0 -} -func (f *mockFileInfo) Mode() os.FileMode { - return 0 -} -func (f *mockFileInfo) ModTime() time.Time { - return time.Now() -} -func (f *mockFileInfo) IsDir() bool { - return f.isDir -} -func (f *mockFileInfo) Sys() interface{} { - return nil -} - type mockPidProcessor struct { t *testing.T pids []int @@ -124,7 +163,7 @@ func TestRefresh(t *testing.T) { pidsProcessed: []int{3, 2}, }, { - name: "nothing new", + name: "nothing-new", pl: procList{1: unit, 2: unit, 3: unit}, newPids: []int{1, 2, 3}, plAfter: procList{1: unit, 2: unit, 3: unit}, @@ -134,7 +173,7 @@ func TestRefresh(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - defer mockPidList(tt.newPids)() + defer mockPidList(tt.newPids, t)() m := &mockPidProcessor{t, []int{}} tt.pl.refresh(m) @@ -152,24 +191,43 @@ func TestRefresh(t *testing.T) { // separate test for failing, only one case where getPids fails func TestRefreshFail(t *testing.T) { e := errors.New("file-system-error") - defer mockProcDirReader([]os.FileInfo{}, e)() - m := &mockPidProcessor{t, []int{}} - pl := procList{1: unit} - err := pl.refresh(m) - if err == nil { - t.Errorf("Expected an error") - } else { - if strings.Index(err.Error(), e.Error()) == -1 { - t.Errorf("Unexpected error: %v", err) - } + for _, tt := range []struct { + name string + errRead error + errOpen error + }{ + { + name: "open-dir-fail", + errRead: nil, + errOpen: e, + }, + { + name: "read-dir-fail", + errRead: e, + errOpen: nil, + }, + } { + t.Run(tt.name, func(t *testing.T) { + defer mockDir("/proc", []string{}, tt.errRead, tt.errOpen, t)() + m := &mockPidProcessor{t, []int{}} + pl := procList{1: unit} + err := pl.refresh(m) + if err == nil { + t.Errorf("Expected an error") + } else { + if strings.Index(err.Error(), e.Error()) == -1 { + t.Errorf("Unexpected error: %v", err) + } + } + + }) } } -func mockPidList(pids []int) func() { - dirs := make([]os.FileInfo, 0) +func mockPidList(pids []int, t *testing.T) func() { + dirs := make([]string, 0) for _, pid := range pids { - dirs = append(dirs, newMockDir(fmt.Sprintf("%d", pid))) + dirs = append(dirs, fmt.Sprintf("%d", pid)) } - restore := mockProcDirReader(dirs, nil) - return restore + return mockDir("/proc", dirs, nil, nil, t) } diff --git a/internal/psscanner/psscanner_test.go b/internal/psscanner/psscanner_test.go index 75e09ab..0c93e03 100644 --- a/internal/psscanner/psscanner_test.go +++ b/internal/psscanner/psscanner_test.go @@ -31,7 +31,7 @@ func TestRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - defer mockPidList(tt.pids)() + defer mockPidList(tt.pids, t)() for _, pid := range tt.pids { defer mockPidCmdLine(pid, []byte("the-command"), nil, nil, t)() defer mockPidStatus(pid, []byte{}, nil, nil, t)() // don't mock read value since it's not worth it