refactor: factor common code to check whether file exists (#1045)

Part of ooni/probe#2401

cmd/ooniprobe/internal/autorun/autorun_darwin.go

@@ -11,7 +11,7 @@ import (
 	"text/template"
 
 	"github.com/apex/log"
-	"github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/utils"
+	"github.com/ooni/probe-cli/v3/internal/fsx"
 	"github.com/ooni/probe-cli/v3/internal/shellx"
 	"golang.org/x/sys/execabs"
 	"golang.org/x/sys/unix"
@@ -90,7 +90,7 @@ func (managerDarwin) LogStream() error {
 
 func (managerDarwin) mustNotHavePlist() error {
 	log.Infof("exec: test -f %s && already_registered()", plistPath)
-	if utils.FileExists(plistPath) {
+	if fsx.RegularFileExists(plistPath) {
 		// This is not atomic. Do we need atomicity here?
 		return errors.New("autorun: service already registered")
 	}

cmd/ooniprobe/internal/utils/paths.go

@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 
 	"github.com/ooni/probe-cli/v3/cmd/ooniprobe/internal/utils/homedir"
+	"github.com/ooni/probe-cli/v3/internal/fsx"
 )
 
 // RequiredDirs returns the required ooni home directories
@@ -44,13 +45,6 @@ func DBDir(home string, name string) string {
 	return filepath.Join(home, "db", fmt.Sprintf("%s.sqlite3", name))
 }
 
-// FileExists returns true if the specified path exists and is a
-// regular file.
-func FileExists(path string) bool {
-	stat, err := os.Stat(path)
-	return err == nil && stat.Mode().IsRegular()
-}
-
 // GetOONIHome returns the path to the OONI Home
 func GetOONIHome() (string, error) {
 	if ooniHome := os.Getenv("OONI_HOME"); ooniHome != "" {
@@ -74,5 +68,5 @@ func DidLegacyInformedConsent() bool {
 	}
 
 	path := filepath.Join(filepath.Join(home, ".ooni"), "initialized")
-	return FileExists(path)
+	return fsx.RegularFileExists(path)
 }

internal/cmd/miniooni/consent.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path"
 
+	"github.com/ooni/probe-cli/v3/internal/fsx"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
@@ -18,7 +19,7 @@ func acquireUserConsent(miniooniDir string, currentOptions *Options) {
 	err := maybeWriteConsentFile(currentOptions.Yes, consentFile)
 	runtimex.PanicOnError(err, "cannot write informed consent file")
 	runtimex.Assert(
-		regularFileExists(consentFile),
+		fsx.RegularFileExists(consentFile),
 		riskOfRunningOONI,
 	)
 }

internal/cmd/miniooni/utils.go

@@ -14,12 +14,6 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
-// regularFileExists returns true if the given filepath exists and is a regular file
-func regularFileExists(filepath string) bool {
-	stat, err := os.Stat(filepath)
-	return err == nil && stat.Mode().IsRegular()
-}
-
 // splitPair takes in input a string in the form KEY=VALUE and splits it. This
 // function returns an error if it cannot find the = character to split the string.
 func splitPair(s string) (string, string, error) {

internal/cmd/oonireport/oonireport.go

@@ -13,6 +13,7 @@ import (
 	"github.com/apex/log"
 
 	"github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/fsx"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/probeservices"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
@@ -52,11 +53,6 @@ func fatalIfFalse(cond bool, msg string) {
 	}
 }
 
-func canOpen(filepath string) bool {
-	stat, err := os.Stat(filepath)
-	return err == nil && stat.Mode().IsRegular()
-}
-
 func readLines(path string) []string {
 	// open measurement file
 	file, err := os.Open(path)
@@ -127,7 +123,7 @@ func submitAll(ctx context.Context, lines []string, subm *probeservices.Submitte
 func mainWithArgs(args []string) {
 	fatalIfFalse(len(args) == 2, "Usage: ./oonireport upload <file>")
 	fatalIfFalse(args[0] == "upload", "Unsupported operation")
-	fatalIfFalse(canOpen(args[1]), "Cannot open measurement file")
+	fatalIfFalse(fsx.RegularFileExists(args[1]), "Cannot open measurement file")
 
 	path = args[1]
 	lines := readLines(path)

internal/cmd/oonireport/oonireport_test.go

@@ -5,13 +5,6 @@ import (
 	"testing"
 )
 
-func TestCanOpen(t *testing.T) {
-	ok := canOpen("testdata/testmeasurement.json")
-	if !ok {
-		t.Fatal("unexpected error")
-	}
-}
-
 func TestReadLines(t *testing.T) {
 	lines := readLines("testdata/testmeasurement.json")
 	if lines == nil {

internal/fsx/fsx.go

@@ -2,22 +2,25 @@
 package fsx
 
 import (
+	"errors"
+	"fmt"
 	"io/fs"
 	"os"
-	"syscall"
 )
 
 // OpenFile is a wrapper for os.OpenFile that ensures that
 // we're opening a file rather than a directory. If you are
-// opening a directory, this func returns an *os.PathError
-// error with Err set to syscall.EISDIR.
+// not opening a regular file, this func returns an error.
 //
 // As mentioned in CONTRIBUTING.md, this is the function
 // you SHOULD be using when opening files.
 func OpenFile(pathname string) (fs.File, error) {
 	return openWithFS(filesystem{}, pathname)
 }
 
+// ErrNotRegularFile indicates you're not opening a regular file.
+var ErrNotRegularFile = errors.New("not a regular file")
+
 // openWithFS is like Open but with explicit file system argument.
 func openWithFS(fs fs.FS, pathname string) (fs.File, error) {
 	file, err := fs.Open(pathname)
@@ -29,13 +32,9 @@ func openWithFS(fs fs.FS, pathname string) (fs.File, error) {
 		file.Close()
 		return nil, err
 	}
-	if info.IsDir() {
+	if !isRegular(info) {
 		file.Close()
-		return nil, &os.PathError{
-			Op:   "openFile",
-			Path: pathname,
-			Err:  syscall.EISDIR,
-		}
+		return nil, fmt.Errorf("%w: %s", ErrNotRegularFile, pathname)
 	}
 	return file, nil
 }
@@ -47,3 +46,17 @@ type filesystem struct{}
 func (filesystem) Open(pathname string) (fs.File, error) {
 	return os.Open(pathname)
 }
+
+func isRegular(info fs.FileInfo) bool {
+	return info.Mode().IsRegular()
+}
+
+// RegularFileExists returns whether the given filename
+// exists and is a regular file.
+func RegularFileExists(filename string) bool {
+	finfo, err := os.Stat(filename)
+	if err != nil {
+		return false
+	}
+	return isRegular(finfo)
+}

internal/fsx/fsx_test.go

@@ -4,6 +4,8 @@ import (
 	"errors"
 	"io/fs"
 	"os"
+	"path/filepath"
+	"runtime"
 	"sync/atomic"
 	"syscall"
 	"testing"
@@ -69,7 +71,7 @@ func TestOpenNonexistentFile(t *testing.T) {
 
 func TestOpenDirectoryShouldFail(t *testing.T) {
 	_, err := OpenFile(baseDir)
-	if !errors.Is(err, syscall.EISDIR) {
+	if !errors.Is(err, ErrNotRegularFile) {
 		t.Fatalf("not the error we expected: %+v", err)
 	}
 }
@@ -81,3 +83,38 @@ func TestOpeningExistingFileShouldWork(t *testing.T) {
 	}
 	defer file.Close()
 }
+
+func TestRegularFileExists(t *testing.T) {
+	t.Run("for existing file", func(t *testing.T) {
+		path := filepath.Join("testdata", "testfile.txt")
+		exists := RegularFileExists(path)
+		if !exists {
+			t.Fatal("should exist")
+		}
+	})
+
+	t.Run("for existing directory", func(t *testing.T) {
+		exists := RegularFileExists("testdata")
+		if exists {
+			t.Fatal("should not exist")
+		}
+	})
+
+	t.Run("for nonexisting file", func(t *testing.T) {
+		path := filepath.Join("testdata", "nonexistent")
+		exists := RegularFileExists(path)
+		if exists {
+			t.Fatal("should not exist")
+		}
+	})
+
+	t.Run("for a special file", func(t *testing.T) {
+		if runtime.GOOS == "windows" {
+			t.Skip("skip test under windows")
+		}
+		exists := RegularFileExists("/dev/null")
+		if exists {
+			t.Fatal("should not exist")
+		}
+	})
+}