From 7b0703eaa0582f4af7a127c756cded8405cf59a8 Mon Sep 17 00:00:00 2001
From: Alberto Bertogli <albertito@blitiri.com.ar>
Date: Mon, 13 Apr 2020 14:57:32 +0100
Subject: [PATCH] queue: Check that we can create the queue directory

When creating a new Queue instance, we os.MkdirAll the queue directory.

Currently we don't check if it fails, which will cause us to find out
about problems when the queue is first used, where it is more annoying
to troubleshoot.

This patch adjusts the code so that we check and propagate the error.
That way, problems with the queue directory will be more evident and
easier to handle.
---
 internal/queue/queue.go      |  8 ++++----
 internal/queue/queue_test.go | 22 ++++++++++++++++------
 internal/smtpsrv/server.go   |  8 ++++++--
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/internal/queue/queue.go b/internal/queue/queue.go
index 1a90fe8..2d0b6b7 100644
--- a/internal/queue/queue.go
+++ b/internal/queue/queue.go
@@ -108,11 +108,10 @@ type Queue struct {
 
 // New creates a new Queue instance.
 func New(path string, localDomains *set.String, aliases *aliases.Resolver,
-	localC, remoteC courier.Courier) *Queue {
+	localC, remoteC courier.Courier) (*Queue, error) {
 
-	os.MkdirAll(path, 0700)
-
-	return &Queue{
+	err := os.MkdirAll(path, 0700)
+	q := &Queue{
 		q:            map[string]*Item{},
 		localC:       localC,
 		remoteC:      remoteC,
@@ -120,6 +119,7 @@ func New(path string, localDomains *set.String, aliases *aliases.Resolver,
 		path:         path,
 		aliases:      aliases,
 	}
+	return q, err
 }
 
 // Load the queue and launch the sending loops on startup.
diff --git a/internal/queue/queue_test.go b/internal/queue/queue_test.go
index cb9167b..9782c68 100644
--- a/internal/queue/queue_test.go
+++ b/internal/queue/queue_test.go
@@ -17,7 +17,7 @@ func TestBasic(t *testing.T) {
 	defer testlib.RemoveIfOk(t, dir)
 	localC := testlib.NewTestCourier()
 	remoteC := testlib.NewTestCourier()
-	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	localC.Expect(2)
@@ -67,7 +67,7 @@ func TestDSNOnTimeout(t *testing.T) {
 	remoteC := testlib.NewTestCourier()
 	dir := testlib.MustTempDir(t)
 	defer testlib.RemoveIfOk(t, dir)
-	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	// Insert an expired item in the queue.
@@ -111,7 +111,7 @@ func TestAliases(t *testing.T) {
 	remoteC := testlib.NewTestCourier()
 	dir := testlib.MustTempDir(t)
 	defer testlib.RemoveIfOk(t, dir)
-	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		localC, remoteC)
 
 	q.aliases.AddDomain("loco")
@@ -155,7 +155,7 @@ func TestAliases(t *testing.T) {
 func TestFullQueue(t *testing.T) {
 	dir := testlib.MustTempDir(t)
 	defer testlib.RemoveIfOk(t, dir)
-	q := New(dir, set.NewString(), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString(), aliases.NewResolver(),
 		testlib.DumbCourier, testlib.DumbCourier)
 
 	// Force-insert maxQueueSize items in the queue.
@@ -197,7 +197,7 @@ func TestFullQueue(t *testing.T) {
 func TestPipes(t *testing.T) {
 	dir := testlib.MustTempDir(t)
 	defer testlib.RemoveIfOk(t, dir)
-	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		testlib.DumbCourier, testlib.DumbCourier)
 	item := &Item{
 		Message: Message{
@@ -215,6 +215,16 @@ func TestPipes(t *testing.T) {
 	}
 }
 
+func TestBadPath(t *testing.T) {
+	// A new queue will attempt to os.MkdirAll the path.
+	// We expect this path to fail.
+	_, err := New("/proc/doesnotexist", set.NewString("loco"),
+		aliases.NewResolver(), testlib.DumbCourier, testlib.DumbCourier)
+	if err == nil {
+		t.Errorf("could create queue, expected permission denied")
+	}
+}
+
 func TestNextDelay(t *testing.T) {
 	cases := []struct{ since, min time.Duration }{
 		{10 * time.Second, 1 * time.Minute},
@@ -260,7 +270,7 @@ func TestSerialization(t *testing.T) {
 	// Create the queue; should load the
 	remoteC := testlib.NewTestCourier()
 	remoteC.Expect(1)
-	q := New(dir, set.NewString("loco"), aliases.NewResolver(),
+	q, _ := New(dir, set.NewString("loco"), aliases.NewResolver(),
 		testlib.DumbCourier, remoteC)
 	q.Load()
 
diff --git a/internal/smtpsrv/server.go b/internal/smtpsrv/server.go
index 25b851a..b688ff1 100644
--- a/internal/smtpsrv/server.go
+++ b/internal/smtpsrv/server.go
@@ -147,8 +147,12 @@ func (s *Server) InitDomainInfo(dir string) *domaininfo.DB {
 
 // InitQueue initializes the queue.
 func (s *Server) InitQueue(path string, localC, remoteC courier.Courier) {
-	q := queue.New(path, s.localDomains, s.aliasesR, localC, remoteC)
-	err := q.Load()
+	q, err := queue.New(path, s.localDomains, s.aliasesR, localC, remoteC)
+	if err != nil {
+		log.Fatalf("Error initializing queue: %v:", err)
+	}
+
+	err = q.Load()
 	if err != nil {
 		log.Fatalf("Error loading queue: %v", err)
 	}