maildir: Provide nicer error message on invalid url v2 PROPOSED

Tero Koskinen: 1
 maildir: Provide nicer error message on invalid url

 2 files changed, 31 insertions(+), 4 deletions(-)

Hi,

On Sat, 25 Jul 2020 08:23:16 +0200
Reto <reto@labrat.space> wrote:

I added the error checking in v3.

My initial assumption was that fstat() shouldn't fail if we have
successfully opened the directory. But now, after checking the manual
pages, I noticed at least on OpenBSD fstat() can return EIO if there
is a file system reading error during fstat() call.

[PATCH v2] maildir: Provide nicer error message on invalid url Export this patch

If accounts.conf contains an invalid maildir url, return a nice
error instead of panicking.

Log a couple of different error cases to provide extra
information about the error to the user.
---
 worker/maildir/container.go | 18 +++++++++++++++---
 worker/maildir/worker.go    | 17 ++++++++++++++++-
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/worker/maildir/container.go b/worker/maildir/container.go
index cd9a447..fd46fd8 100644
--- a/worker/maildir/container.go
+++ b/worker/maildir/container.go
@@ -21,15 +21,27 @@ type Container struct {
 }
 
 // NewContainer creates a new container at the specified directory
-// TODO: return an error if the provided directory is not accessible
-func NewContainer(dir string, l *log.Logger) *Container {
-	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}
+func NewContainer(dir string, l *log.Logger) (*Container, error) {
+	f, err := os.Open(dir)
+	if err != nil {
+		return nil, err
+	}
+	s, _ := f.Stat()

Reto <reto@labrat.space>

4 years ago

Why are you ignoring the error instead of returning it?
If the error is non nil, chances are that s is the zero valued struct, which
isn't overly useful but you still try to call methods on it later.

Next

+	f.Close()

Reto <reto@labrat.space>

4 years ago

defer please, and right after opening it (after the error check).
Not that important here, but you might as well get into the habit

Tero Koskinen <tero.koskinen@iki.fi>

4 years ago

Added defer in v3.

Yours,
 Tero

Next

+	if !s.IsDir() {
+		return nil, fmt.Errorf("Given maildir '%s' not a directory", dir)
+	}
+	return &Container{dir: dir, uids: uidstore.NewStore(), log: l}, nil
 }

Reto <reto@labrat.space>

4 years ago

Greetings,
Reto

 
 // ListFolders returns a list of maildir folders in the container
 func (c *Container) ListFolders() ([]string, error) {
 	folders := []string{}
 	err := filepath.Walk(c.dir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return fmt.Errorf("Invalid path '%s': error: %v", path, err)
+
+		}
 		if !info.IsDir() {
 			return nil
 		}
diff --git a/worker/maildir/worker.go b/worker/maildir/worker.go
index ce548ff..d1ff3c2 100644
--- a/worker/maildir/worker.go
+++ b/worker/maildir/worker.go
@@ -1,6 +1,7 @@
 package maildir
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"net/url"
@@ -221,7 +222,15 @@ func (w *Worker) handleConfigure(msg *types.Configure) error {
 		}
 		dir = filepath.Join(home, u.Path)
 	}
-	w.c = NewContainer(dir, w.worker.Logger)
+	if len(dir) == 0 {
+		return fmt.Errorf("could not resolve maildir from URL '%s'", msg.Config.Source)
+	}
+	c, err := NewContainer(dir, w.worker.Logger)
+	if err != nil {
+		w.worker.Logger.Printf("could not configure maildir: %s", dir)
+		return err
+	}
+	w.c = c
 	w.worker.Logger.Printf("configured base maildir: %s", dir)
 	return nil
 }
@@ -231,6 +240,12 @@ func (w *Worker) handleConnect(msg *types.Connect) error {
 }
 
 func (w *Worker) handleListDirectories(msg *types.ListDirectories) error {
+	// TODO If handleConfigure has returned error, w.c is nil.
+	// It could be better if we skip directory listing completely
+	// when configure fails.
+	if w.c == nil {
+		return errors.New("Incorrect maildir directory")
+	}
 	dirs, err := w.c.ListFolders()
 	if err != nil {
 		w.worker.Logger.Printf("error listing directories: %v", err)
-- 
2.27.0

Reto <reto@labrat.space>

4 years ago

Hi,
Thanks for the update