Return better errors from Mount, instead of dumping them to stderr.

For googlecloudplatform/gcsfuse#161.

mount_linux.go

@@ -1,37 +1,14 @@
 package fuse
 
 import (
-	"bufio"
+	"bytes"
 	"fmt"
-	"io"
-	"log"
 	"net"
 	"os"
 	"os/exec"
-	"sync"
 	"syscall"
 )
 
-func lineLogger(wg *sync.WaitGroup, prefix string, r io.ReadCloser) {
-	defer wg.Done()
-
-	scanner := bufio.NewScanner(r)
-	for scanner.Scan() {
-		switch line := scanner.Text(); line {
-		case `fusermount: failed to open /etc/fuse.conf: Permission denied`:
-			// Silence this particular message, it occurs way too
-			// commonly and isn't very relevant to whether the mount
-			// succeeds or not.
-			continue
-		default:
-			log.Printf("%s: %s", prefix, line)
-		}
-	}
-	if err := scanner.Err(); err != nil {
-		log.Printf("%s, error reading: %v", prefix, err)
-	}
-}
-
 // Begin the process of mounting at the given directory, returning a connection
 // to the kernel. Mounting continues in the background, and is complete when an
 // error is written to the supplied channel. The file system may need to
@@ -57,7 +34,9 @@ func mount(
 	readFile := os.NewFile(uintptr(fds[1]), "fusermount-parent-reads")
 	defer readFile.Close()
 
-	// Start fusermount, passing it pipes for stdout and stderr.
+	// Start fusermount, passing it a buffer in which to write stderr.
+	var stderr bytes.Buffer
+
 	cmd := exec.Command(
 		"fusermount",
 		"-o", cfg.toOptionsString(),
@@ -67,36 +46,12 @@ func mount(
 
 	cmd.Env = append(os.Environ(), "_FUSE_COMMFD=3")
 	cmd.ExtraFiles = []*os.File{writeFile}
+	cmd.Stderr = &stderr
 
-	stdout, err := cmd.StdoutPipe()
+	// Run the command.
+	err = cmd.Run()
 	if err != nil {
-		err = fmt.Errorf("StdoutPipe: %v", err)
-		return
-	}
-
-	stderr, err := cmd.StderrPipe()
-	if err != nil {
-		err = fmt.Errorf("StderrPipe: %v", err)
-		return
-	}
-
-	err = cmd.Start()
-	if err != nil {
-		err = fmt.Errorf("Starting fusermount: %v", err)
-		return
-	}
-
-	// Log fusermount output until it closes stdout and stderr.
-	var wg sync.WaitGroup
-	wg.Add(2)
-	go lineLogger(&wg, "mount helper output", stdout)
-	go lineLogger(&wg, "mount helper error", stderr)
-	wg.Wait()
-
-	// Wait for the command.
-	err = cmd.Wait()
-	if err != nil {
-		err = fmt.Errorf("fusermount: %v", err)
+		err = fmt.Errorf("running fusermount: %v\n\nstderr:\n%s", err, stderr.Bytes())
 		return
 	}
 

mount_test.go

@@ -0,0 +1,142 @@
+package fuse_test
+
+import (
+	"io/ioutil"
+	"os"
+	"path"
+	"runtime"
+	"strings"
+	"testing"
+
+	"golang.org/x/net/context"
+
+	"github.com/jacobsa/fuse"
+	"github.com/jacobsa/fuse/fuseops"
+	"github.com/jacobsa/fuse/fuseutil"
+)
+
+////////////////////////////////////////////////////////////////////////
+// minimalFS
+////////////////////////////////////////////////////////////////////////
+
+// A minimal fuseutil.FileSystem that can successfully mount but do nothing
+// else.
+type minimalFS struct {
+	fuseutil.NotImplementedFileSystem
+}
+
+func (fs *minimalFS) StatFS(
+	ctx context.Context,
+	op *fuseops.StatFSOp) (err error) {
+	return
+}
+
+////////////////////////////////////////////////////////////////////////
+// Tests
+////////////////////////////////////////////////////////////////////////
+
+func TestSuccessfulMount(t *testing.T) {
+	ctx := context.Background()
+
+	// Set up a temporary directory.
+	dir, err := ioutil.TempDir("", "mount_test")
+	if err != nil {
+		t.Fatal("ioutil.TempDir: %v", err)
+	}
+
+	defer os.RemoveAll(dir)
+
+	// Mount.
+	fs := &minimalFS{}
+	mfs, err := fuse.Mount(
+		dir,
+		fuseutil.NewFileSystemServer(fs),
+		&fuse.MountConfig{})
+
+	if err != nil {
+		t.Fatalf("fuse.Mount: %v", err)
+	}
+
+	defer func() {
+		if err := mfs.Join(ctx); err != nil {
+			t.Errorf("Joining: %v", err)
+		}
+	}()
+
+	defer fuse.Unmount(mfs.Dir())
+}
+
+func TestNonEmptyMountPoint(t *testing.T) {
+	ctx := context.Background()
+
+	// osxfuse appears to be happy to mount over a non-empty mount point.
+	//
+	// We leave this test in for Linux, because it tickles the behavior of
+	// fusermount writing to stderr and exiting with an error code. We want to
+	// make sure that a descriptive error makes it back to the user.
+	if runtime.GOOS == "darwin" {
+		return
+	}
+
+	// Set up a temporary directory.
+	dir, err := ioutil.TempDir("", "mount_test")
+	if err != nil {
+		t.Fatal("ioutil.TempDir: %v", err)
+	}
+
+	defer os.RemoveAll(dir)
+
+	// Add a file within it.
+	err = ioutil.WriteFile(path.Join(dir, "foo"), []byte{}, 0600)
+	if err != nil {
+		t.Fatalf("ioutil.WriteFile: %v", err)
+	}
+
+	// Attempt to mount.
+	fs := &minimalFS{}
+	mfs, err := fuse.Mount(
+		dir,
+		fuseutil.NewFileSystemServer(fs),
+		&fuse.MountConfig{})
+
+	if err == nil {
+		fuse.Unmount(mfs.Dir())
+		mfs.Join(ctx)
+		t.Fatal("fuse.Mount returned nil")
+	}
+
+	const want = "not empty"
+	if got := err.Error(); !strings.Contains(got, want) {
+		t.Errorf("Unexpected error: %v", got)
+	}
+}
+
+func TestNonexistentMountPoint(t *testing.T) {
+	ctx := context.Background()
+
+	// Set up a temporary directory.
+	dir, err := ioutil.TempDir("", "mount_test")
+	if err != nil {
+		t.Fatal("ioutil.TempDir: %v", err)
+	}
+
+	defer os.RemoveAll(dir)
+
+	// Attempt to mount into a sub-directory that doesn't exist.
+	fs := &minimalFS{}
+	mfs, err := fuse.Mount(
+		path.Join(dir, "foo"),
+		fuseutil.NewFileSystemServer(fs),
+		&fuse.MountConfig{})
+
+	if err == nil {
+		fuse.Unmount(mfs.Dir())
+		mfs.Join(ctx)
+		t.Fatal("fuse.Mount returned nil")
+	}
+
+	const want = "no such file"
+	if got := err.Error(); !strings.Contains(got, want) {
+		t.Errorf("Unexpected error: %v", got)
+	}
+}