Follow meta-linter recommendations for all of Inbucket

- Rename BUILD_DATE to BUILDDATE in goxc - Update travis config - Follow linter recommendations for inbucket.go - Follow linter recommendations for config package - Follow linter recommendations for log package - Follow linter recommendations for pop3d package - Follow linter recommendations for smtpd package - Follow linter recommendations for web package - Fix Id -> ID in templates - Add shebang to REST demo scripts - Add or refine many comments
2026-01-07 19:57:06 +00:00 · 2016-02-20 23:20:22 -08:00
parent 83f9c6aa49
commit e6b7e335cb
34 changed files with 607 additions and 411 deletions
--- a/smtpd/datastore.go
+++ b/smtpd/datastore.go
@@ -9,13 +9,16 @@ import (
 	"github.com/jhillyerd/go.enmime"
 )

+// ErrNotExist indicates the requested message does not exist
 var ErrNotExist = errors.New("Message does not exist")

+// DataStore is an interface to get Mailboxes stored in Inbucket
 type DataStore interface {
 	MailboxFor(emailAddress string) (Mailbox, error)
 	AllMailboxes() ([]Mailbox, error)
 }

+// Mailbox is an interface to get and manipulate messages in a DataStore
 type Mailbox interface {
 	GetMessages() ([]Message, error)
 	GetMessage(id string) (Message, error)
@@ -24,8 +27,9 @@ type Mailbox interface {
 	String() string
 }

+// Message is an interface for a single message in a Mailbox
 type Message interface {
-	Id() string
+	ID() string
 	From() string
 	Date() time.Time
 	Subject() string
--- a/smtpd/filestore.go
+++ b/smtpd/filestore.go
@@ -19,16 +19,23 @@ import (
 )

 // Name of index file in each mailbox
-const INDEX_FILE = "index.gob"
+const indexFileName = "index.gob"

-// We lock this when reading/writing an index file, this is a bottleneck because
-// it's a single lock even if we have a million index files
-var indexLock = new(sync.RWMutex)
+var (
+	// indexLock is locked while reading/writing an index file
+	//
+	// NOTE: This is a bottleneck because  it's a single lock even if we have a
+	// million index files
+	indexLock = new(sync.RWMutex)

-var ErrNotWritable = errors.New("Message not writable")
+	// TODO Consider moving this to the Message interface
+	errNotWritable = errors.New("Message not writable")

-// Global because we only want one regardless of the number of DataStore objects
-var countChannel = make(chan int, 10)
+	// countChannel is filled with a sequential numbers (0000..9999), which are
+	// used by generateID() to generate unique message IDs.  It's global
+	// because we only want one regardless of the number of DataStore objects
+	countChannel = make(chan int, 10)
+)

 func init() {
 	// Start generator
@@ -42,8 +49,8 @@ func countGenerator(c chan int) {
 	}
 }

-// A DataStore is the root of the mail storage hiearchy.  It provides access to
-// Mailbox objects
+// FileDataStore implements DataStore aand is the root of the mail storage
+// hiearchy.  It provides access to Mailbox objects
 type FileDataStore struct {
 	path       string
 	mailPath   string
@@ -54,13 +61,15 @@ type FileDataStore struct {
 func NewFileDataStore(cfg config.DataStoreConfig) DataStore {
 	path := cfg.Path
 	if path == "" {
-		log.LogError("No value configured for datastore path")
+		log.Errorf("No value configured for datastore path")
 		return nil
 	}
 	mailPath := filepath.Join(path, "mail")
 	if _, err := os.Stat(mailPath); err != nil {
 		// Mail datastore does not yet exist
-		os.MkdirAll(mailPath, 0770)
+		if err = os.MkdirAll(mailPath, 0770); err != nil {
+			log.Errorf("Error creating dir %q: %v", mailPath, err)
+		}
 	}
 	return &FileDataStore{path: path, mailPath: mailPath, messageCap: cfg.MailboxMsgCap}
 }
@@ -72,7 +81,7 @@ func DefaultFileDataStore() DataStore {
 	return NewFileDataStore(cfg)
 }

-// Retrieves the Mailbox object for a specified email address, if the mailbox
+// MailboxFor retrieves the Mailbox object for a specified email address, if the mailbox
 // does not exist, it will attempt to create it.
 func (ds *FileDataStore) MailboxFor(emailAddress string) (Mailbox, error) {
 	name, err := ParseMailboxName(emailAddress)
@@ -83,7 +92,7 @@ func (ds *FileDataStore) MailboxFor(emailAddress string) (Mailbox, error) {
 	s1 := dir[0:3]
 	s2 := dir[0:6]
 	path := filepath.Join(ds.mailPath, s1, s2, dir)
-	indexPath := filepath.Join(path, INDEX_FILE)
+	indexPath := filepath.Join(path, indexFileName)

 	return &FileMailbox{store: ds, name: name, dirName: dir, path: path,
 		indexPath: indexPath}, nil
@@ -117,7 +126,7 @@ func (ds *FileDataStore) AllMailboxes() ([]Mailbox, error) {
 						if inf3.IsDir() {
 							mbdir := inf3.Name()
 							mbpath := filepath.Join(ds.mailPath, l1, l2, mbdir)
-							idx := filepath.Join(mbpath, INDEX_FILE)
+							idx := filepath.Join(mbpath, indexFileName)
 							mb := &FileMailbox{store: ds, dirName: mbdir, path: mbpath,
 								indexPath: idx}
 							mailboxes = append(mailboxes, mb)
@@ -131,8 +140,8 @@ func (ds *FileDataStore) AllMailboxes() ([]Mailbox, error) {
 	return mailboxes, nil
 }

-// A Mailbox manages the mail for a specific user and correlates to a particular
-// directory on disk.
+// FileMailbox implements Mailbox, manages the mail for a specific user and
+// correlates to a particular directory on disk.
 type FileMailbox struct {
 	store       *FileDataStore
 	name        string
@@ -180,7 +189,7 @@ func (mb *FileMailbox) GetMessage(id string) (Message, error) {
 	return nil, ErrNotExist
 }

-// Delete all messages in this mailbox
+// Purge deletes all messages in this mailbox
 func (mb *FileMailbox) Purge() error {
 	mb.messages = mb.messages[:0]
 	return mb.writeIndex()
@@ -196,7 +205,7 @@ func (mb *FileMailbox) readIndex() error {
 	// Check if index exists
 	if _, err := os.Stat(mb.indexPath); err != nil {
 		// Does not exist, but that's not an error in our world
-		log.LogTrace("Index %v does not exist (yet)", mb.indexPath)
+		log.Tracef("Index %v does not exist (yet)", mb.indexPath)
 		mb.indexLoaded = true
 		return nil
 	}
@@ -204,7 +213,11 @@ func (mb *FileMailbox) readIndex() error {
 	if err != nil {
 		return err
 	}
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil {
+			log.Errorf("Failed to close %q: %v", mb.indexPath, err)
+		}
+	}()

 	// Decode gob data
 	dec := gob.NewDecoder(bufio.NewReader(file))
@@ -219,7 +232,7 @@ func (mb *FileMailbox) readIndex() error {
 			return fmt.Errorf("While decoding message: %v", err)
 		}
 		msg.mailbox = mb
-		log.LogTrace("Found: %v", msg)
+		log.Tracef("Found: %v", msg)
 		mb.messages = append(mb.messages, msg)
 	}

@@ -231,7 +244,7 @@ func (mb *FileMailbox) readIndex() error {
 func (mb *FileMailbox) createDir() error {
 	if _, err := os.Stat(mb.path); err != nil {
 		if err := os.MkdirAll(mb.path, 0770); err != nil {
-			log.LogError("Failed to create directory %v, %v", mb.path, err)
+			log.Errorf("Failed to create directory %v, %v", mb.path, err)
 			return err
 		}
 	}
@@ -253,7 +266,11 @@ func (mb *FileMailbox) writeIndex() error {
 		if err != nil {
 			return err
 		}
-		defer file.Close()
+		defer func() {
+			if err := file.Close(); err != nil {
+				log.Errorf("Failed to close %q: %v", mb.indexPath, err)
+			}
+		}()
 		writer := bufio.NewWriter(file)

 		// Write each message and then flush
@@ -264,18 +281,20 @@ func (mb *FileMailbox) writeIndex() error {
 				return err
 			}
 		}
-		writer.Flush()
+		if err := writer.Flush(); err != nil {
+			return err
+		}
 	} else {
 		// No messages, delete index+maildir
-		log.LogTrace("Removing mailbox %v", mb.path)
+		log.Tracef("Removing mailbox %v", mb.path)
 		return os.RemoveAll(mb.path)
 	}

 	return nil
 }

-// Message contains a little bit of data about a particular email message, and
-// methods to retrieve the rest of it from disk.
+// FileMessage implements Message and contains a little bit of data about a
+// particular email message, and methods to retrieve the rest of it from disk.
 type FileMessage struct {
 	mailbox *FileMailbox
 	// Stored in GOB
@@ -290,7 +309,7 @@ type FileMessage struct {
 	writer     *bufio.Writer
 }

-// NewMessage creates a new Message object and sets the Date and Id fields.
+// NewMessage creates a new FileMessage object and sets the Date and Id fields.
 // It will also delete messages over messageCap if configured.
 func (mb *FileMailbox) NewMessage() (Message, error) {
 	// Load index
@@ -303,7 +322,7 @@ func (mb *FileMailbox) NewMessage() (Message, error) {
 	// Delete old messages over messageCap
 	if mb.store.messageCap > 0 {
 		for len(mb.messages) >= mb.store.messageCap {
-			log.LogInfo("Mailbox %q over configured message cap", mb.name)
+			log.Infof("Mailbox %q over configured message cap", mb.name)
 			if err := mb.messages[0].Delete(); err != nil {
 				return nil, err
 			}
@@ -311,30 +330,37 @@ func (mb *FileMailbox) NewMessage() (Message, error) {
 	}

 	date := time.Now()
-	id := generateId(date)
+	id := generateID(date)
 	return &FileMessage{mailbox: mb, Fid: id, Fdate: date, writable: true}, nil
 }

-func (m *FileMessage) Id() string {
+// ID gets the ID of the Message
+func (m *FileMessage) ID() string {
 	return m.Fid
 }

+// Date returns the date of the Message
+// TODO Is this the create timestamp, or from the Date header?
 func (m *FileMessage) Date() time.Time {
 	return m.Fdate
 }

+// From returns the value of the Message From header
 func (m *FileMessage) From() string {
 	return m.Ffrom
 }

+// Subject returns the value of the Message Subject header
 func (m *FileMessage) Subject() string {
 	return m.Fsubject
 }

+// String returns a string in the form: "Subject()" from From()
 func (m *FileMessage) String() string {
 	return fmt.Sprintf("\"%v\" from %v", m.Fsubject, m.Ffrom)
 }

+// Size returns the size of the Message on disk in bytes
 func (m *FileMessage) Size() int64 {
 	return m.Fsize
 }
@@ -349,10 +375,14 @@ func (m *FileMessage) ReadHeader() (msg *mail.Message, err error) {
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil {
+			log.Errorf("Failed to close %q: %v", m.rawPath(), err)
+		}
+	}()
+
 	reader := bufio.NewReader(file)
-	msg, err = mail.ReadMessage(reader)
-	return msg, err
+	return mail.ReadMessage(reader)
 }

 // ReadBody opens the .raw portion of a Message and returns a MIMEBody object
@@ -361,7 +391,12 @@ func (m *FileMessage) ReadBody() (body *enmime.MIMEBody, err error) {
 	if err != nil {
 		return nil, err
 	}
-	defer file.Close()
+	defer func() {
+		if err := file.Close(); err != nil {
+			log.Errorf("Failed to close %q: %v", m.rawPath(), err)
+		}
+	}()
+
 	reader := bufio.NewReader(file)
 	msg, err := mail.ReadMessage(reader)
 	if err != nil {
@@ -371,7 +406,7 @@ func (m *FileMessage) ReadBody() (body *enmime.MIMEBody, err error) {
 	if err != nil {
 		return nil, err
 	}
-	return mime, err
+	return mime, nil
 }

 // RawReader opens the .raw portion of a Message as an io.ReadCloser
@@ -386,10 +421,15 @@ func (m *FileMessage) RawReader() (reader io.ReadCloser, err error) {
 // ReadRaw opens the .raw portion of a Message and returns it as a string
 func (m *FileMessage) ReadRaw() (raw *string, err error) {
 	reader, err := m.RawReader()
-	defer reader.Close()
 	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if err := reader.Close(); err != nil {
+			log.Errorf("Failed to close %q: %v", m.rawPath(), err)
+		}
+	}()
+
 	bodyBytes, err := ioutil.ReadAll(bufio.NewReader(reader))
 	if err != nil {
 		return nil, err
@@ -403,7 +443,7 @@ func (m *FileMessage) ReadRaw() (raw *string, err error) {
 func (m *FileMessage) Append(data []byte) error {
 	// Prevent Appending to a pre-existing Message
 	if !m.writable {
-		return ErrNotWritable
+		return errNotWritable
 	}
 	// Open file for writing if we haven't yet
 	if m.writer == nil {
@@ -466,7 +506,8 @@ func (m *FileMessage) Close() error {
 	return m.mailbox.writeIndex()
 }

-// Delete this Message from disk by removing both the gob and raw files
+// Delete this Message from disk by removing it from the index and deleting the
+// raw files.
 func (m *FileMessage) Delete() error {
 	messages := m.mailbox.messages
 	for i, mm := range messages {
@@ -476,16 +517,18 @@ func (m *FileMessage) Delete() error {
 			break
 		}
 	}
-	m.mailbox.writeIndex()
+	if err := m.mailbox.writeIndex(); err != nil {
+		return err
+	}

 	if len(m.mailbox.messages) == 0 {
-		// This was the last message, writeIndex() has removed the entire
-		// directory
+		// This was the last message, thus writeIndex() has removed the entire
+		// directory; we don't need to delete the raw file.
 		return nil
 	}

 	// There are still messages in the index
-	log.LogTrace("Deleting %v", m.rawPath())
+	log.Tracef("Deleting %v", m.rawPath())
 	return os.Remove(m.rawPath())
 }

@@ -498,6 +541,6 @@ func generatePrefix(date time.Time) string {

 // generateId adds a 4-digit unique number onto the end of the string
 // returned by generatePrefix()
-func generateId(date time.Time) string {
+func generateID(date time.Time) string {
 	return generatePrefix(date) + "-" + fmt.Sprintf("%04d", <-countChannel)
 }
--- a/smtpd/filestore_test.go
+++ b/smtpd/filestore_test.go
@@ -95,7 +95,7 @@ func TestFSDirStructure(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -122,7 +122,7 @@ func TestFSAllMailboxes(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -172,7 +172,7 @@ func TestFSDeliverMany(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -201,8 +201,8 @@ func TestFSDelete(t *testing.T) {
 		len(subjects), len(msgs))

 	// Delete a couple messages
-	msgs[1].Delete()
-	msgs[3].Delete()
+	_ = msgs[1].Delete()
+	_ = msgs[3].Delete()

 	// Confirm deletion
 	mb, err = ds.MailboxFor(mbName)
@@ -246,7 +246,7 @@ func TestFSDelete(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -294,7 +294,7 @@ func TestFSPurge(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -332,7 +332,7 @@ func TestFSSize(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -360,7 +360,7 @@ func TestFSMissing(t *testing.T) {
 	msg, err := mb.GetMessage(sentIds[1])
 	assert.Nil(t, err)
 	fmsg := msg.(*FileMessage)
-	os.Remove(fmsg.rawPath())
+	_ = os.Remove(fmsg.rawPath())
 	msg, err = mb.GetMessage(sentIds[1])
 	assert.Nil(t, err)

@@ -374,7 +374,7 @@ func TestFSMissing(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -419,7 +419,7 @@ func TestFSMessageCap(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -454,7 +454,7 @@ func TestFSNoMessageCap(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -490,7 +490,7 @@ func deliverMessage(ds *FileDataStore, mbName string, subject string,
 		panic(err)
 	}
 	// Create message object
-	id = generateId(date)
+	id = generateID(date)
 	msg, err := mb.NewMessage()
 	if err != nil {
 		panic(err)
@@ -498,7 +498,9 @@ func deliverMessage(ds *FileDataStore, mbName string, subject string,
 	fmsg := msg.(*FileMessage)
 	fmsg.Fdate = date
 	fmsg.Fid = id
-	msg.Append(testMsg)
+	if err = msg.Append(testMsg); err != nil {
+		panic(err)
+	}
 	if err = msg.Close(); err != nil {
 		panic(err)
 	}
--- a/smtpd/handler.go
+++ b/smtpd/handler.go
@@ -15,17 +15,23 @@ import (
 	"github.com/jhillyerd/inbucket/log"
 )

+// State tracks the current mode of our SMTP state machine
 type State int

 const (
-	GREET State = iota // Waiting for HELO
-	READY              // Got HELO, waiting for MAIL
-	MAIL               // Got MAIL, accepting RCPTs
-	DATA               // Got DATA, waiting for "."
-	QUIT               // Close session
+	// GREET State: Waiting for HELO
+	GREET State = iota
+	// READY State: Got HELO, waiting for MAIL
+	READY
+	// MAIL State: Got MAIL, accepting RCPTs
+	MAIL
+	// DATA State: Got DATA, waiting for "."
+	DATA
+	// QUIT State: Client requested end of session
+	QUIT
 )

-const STAMP_FMT = "Mon, 02 Jan 2006 15:04:05 -0700 (MST)"
+const timeStampFormat = "Mon, 02 Jan 2006 15:04:05 -0700 (MST)"

 func (s State) String() string {
 	switch s {
@@ -61,6 +67,7 @@ var commands = map[string]bool{
 	"TURN": true,
 }

+// Session holds the state of an SMTP session
 type Session struct {
 	server       *Server
 	id           int
@@ -74,6 +81,7 @@ type Session struct {
 	recipients   *list.List
 }

+// NewSession creates a new Session for the given connection
 func NewSession(server *Server, id int, conn net.Conn) *Session {
 	reader := bufio.NewReader(conn)
 	host, _, _ := net.SplitHostPort(conn.RemoteAddr().String())
@@ -92,10 +100,12 @@ func (ss *Session) String() string {
 *  5. Goto 2
 */
 func (s *Server) startSession(id int, conn net.Conn) {
-	log.LogInfo("SMTP Connection from %v, starting session <%v>", conn.RemoteAddr(), id)
+	log.Infof("SMTP Connection from %v, starting session <%v>", conn.RemoteAddr(), id)
 	expConnectsCurrent.Add(1)
 	defer func() {
-		conn.Close()
+		if err := conn.Close(); err != nil {
+			log.Errorf("Error closing connection for <%v>: %v", id, err)
+		}
 		s.waitgroup.Done()
 		expConnectsCurrent.Add(-1)
 	}()
@@ -321,11 +331,10 @@ func (ss *Session) mailHandler(cmd string, arg string) {
 			// We have recipients, go to accept data
 			ss.enterState(DATA)
 			return
-		} else {
-			// DATA out of sequence
-			ss.ooSeq(cmd)
-			return
 		}
+		// DATA out of sequence
+		ss.ooSeq(cmd)
+		return
 	}
 	ss.ooSeq(cmd)
 }
@@ -333,7 +342,7 @@ func (ss *Session) mailHandler(cmd string, arg string) {
 // DATA
 func (ss *Session) dataHandler() {
 	// Timestamp for Received header
-	stamp := time.Now().Format(STAMP_FMT)
+	stamp := time.Now().Format(timeStampFormat)
 	// Get a Mailbox and a new Message for each recipient
 	mailboxes := make([]Mailbox, ss.recipients.Len())
 	messages := make([]Message, ss.recipients.Len())
@@ -369,9 +378,14 @@ func (ss *Session) dataHandler() {
 				// Generate Received header
 				recd := fmt.Sprintf("Received: from %s ([%s]) by %s\r\n  for <%s>; %s\r\n",
 					ss.remoteDomain, ss.remoteHost, ss.server.domain, recip, stamp)
-				messages[i].Append([]byte(recd))
+				if err := messages[i].Append([]byte(recd)); err != nil {
+					ss.logError("Failed to write received header for %q: %s", local, err)
+					ss.send(fmt.Sprintf("451 Failed to create message for %v", local))
+					ss.reset()
+					return
+				}
 			} else {
-				log.LogTrace("Not storing message for %q", recip)
+				log.Tracef("Not storing message for %q", recip)
 			}
 			i++
 		}
@@ -482,13 +496,17 @@ func (ss *Session) readByteLine(buf *bytes.Buffer) error {
 		if err != nil {
 			return err
 		}
-		buf.Write(line)
+		if _, err = buf.Write(line); err != nil {
+			return err
+		}
 		// Read the next byte looking for '\n'
 		c, err := ss.reader.ReadByte()
 		if err != nil {
 			return err
 		}
-		buf.WriteByte(c)
+		if err = buf.WriteByte(c); err != nil {
+			return err
+		}
 		if c == '\n' {
 			// We've reached the end of the line, return
 			return nil
@@ -570,21 +588,21 @@ func (ss *Session) ooSeq(cmd string) {

 // Session specific logging methods
 func (ss *Session) logTrace(msg string, args ...interface{}) {
-	log.LogTrace("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
+	log.Tracef("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
 }

 func (ss *Session) logInfo(msg string, args ...interface{}) {
-	log.LogInfo("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
+	log.Infof("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
 }

 func (ss *Session) logWarn(msg string, args ...interface{}) {
 	// Update metrics
 	expWarnsTotal.Add(1)
-	log.LogWarn("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
+	log.Warnf("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
 }

 func (ss *Session) logError(msg string, args ...interface{}) {
 	// Update metrics
 	expErrorsTotal.Add(1)
-	log.LogError("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
+	log.Errorf("SMTP[%v]<%v> %v", ss.remoteHost, ss.id, fmt.Sprintf(msg, args...))
 }
--- a/smtpd/handler_test.go
+++ b/smtpd/handler_test.go
@@ -6,7 +6,6 @@ import (
 	"io"

 	"github.com/jhillyerd/inbucket/config"
-	//"io/ioutil"
 	"log"
 	"net"
 	"net/textproto"
@@ -27,8 +26,8 @@ func TestGreetState(t *testing.T) {
 	mb1 := &MockMailbox{}
 	mds.On("MailboxFor").Return(mb1, nil)

-	server, logbuf := setupSmtpServer(mds)
-	defer teardownSmtpServer(server)
+	server, logbuf := setupSMTPServer(mds)
+	defer teardownSMTPServer(server)

 	var script []scriptStep

@@ -77,7 +76,7 @@ func TestGreetState(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -88,8 +87,8 @@ func TestReadyState(t *testing.T) {
 	mb1 := &MockMailbox{}
 	mds.On("MailboxFor").Return(mb1, nil)

-	server, logbuf := setupSmtpServer(mds)
-	defer teardownSmtpServer(server)
+	server, logbuf := setupSMTPServer(mds)
+	defer teardownSMTPServer(server)

 	var script []scriptStep

@@ -142,7 +141,7 @@ func TestReadyState(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -156,8 +155,8 @@ func TestMailState(t *testing.T) {
 	mb1.On("NewMessage").Return(msg1, nil)
 	msg1.On("Close").Return(nil)

-	server, logbuf := setupSmtpServer(mds)
-	defer teardownSmtpServer(server)
+	server, logbuf := setupSMTPServer(mds)
+	defer teardownSMTPServer(server)

 	var script []scriptStep

@@ -252,7 +251,7 @@ func TestMailState(t *testing.T) {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

@@ -266,11 +265,11 @@ func TestDataState(t *testing.T) {
 	mb1.On("NewMessage").Return(msg1, nil)
 	msg1.On("Close").Return(nil)

-	server, logbuf := setupSmtpServer(mds)
-	defer teardownSmtpServer(server)
+	server, logbuf := setupSMTPServer(mds)
+	defer teardownSMTPServer(server)

 	var script []scriptStep
-	pipe := setupSmtpSession(server)
+	pipe := setupSMTPSession(server)
 	c := textproto.NewConn(pipe)

 	// Get us into DATA state
@@ -294,8 +293,8 @@ Subject: test
 Hi!
 `
 	dw := c.DotWriter()
-	io.WriteString(dw, body)
-	dw.Close()
+	_, _ = io.WriteString(dw, body)
+	_ = dw.Close()
 	if code, _, err := c.ReadCodeLine(250); err != nil {
 		t.Errorf("Expected a 250 greeting, got %v", code)
 	}
@@ -304,13 +303,13 @@ Hi!
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)
 		// Dump buffered log data if there was a failure
-		io.Copy(os.Stderr, logbuf)
+		_, _ = io.Copy(os.Stderr, logbuf)
 	}
 }

 // playSession creates a new session, reads the greeting and then plays the script
 func playSession(t *testing.T, server *Server, script []scriptStep) error {
-	pipe := setupSmtpSession(server)
+	pipe := setupSMTPSession(server)
 	c := textproto.NewConn(pipe)

 	if code, _, err := c.ReadCodeLine(220); err != nil {
@@ -319,8 +318,10 @@ func playSession(t *testing.T, server *Server, script []scriptStep) error {

 	err := playScriptAgainst(t, c, script)

-	c.Cmd("QUIT")
-	c.ReadCodeLine(221)
+	// Not all tests leave the session in a clean state, so the following two
+	// calls can fail
+	_, _ = c.Cmd("QUIT")
+	_, _, _ = c.ReadCodeLine(221)

 	return err
 }
@@ -358,11 +359,11 @@ func (m *mockConn) SetDeadline(t time.Time) error      { return nil }
 func (m *mockConn) SetReadDeadline(t time.Time) error  { return nil }
 func (m *mockConn) SetWriteDeadline(t time.Time) error { return nil }

-func setupSmtpServer(ds DataStore) (*Server, *bytes.Buffer) {
+func setupSMTPServer(ds DataStore) (*Server, *bytes.Buffer) {
 	// Test Server Config
-	cfg := config.SmtpConfig{
-		Ip4address:      net.IPv4(127, 0, 0, 1),
-		Ip4port:         2500,
+	cfg := config.SMTPConfig{
+		IP4address:      net.IPv4(127, 0, 0, 1),
+		IP4port:         2500,
 		Domain:          "inbucket.local",
 		DomainNoStore:   "bitbucket.local",
 		MaxRecipients:   5,
@@ -376,12 +377,12 @@ func setupSmtpServer(ds DataStore) (*Server, *bytes.Buffer) {
 	log.SetOutput(buf)

 	// Create a server, don't start it
-	return NewSmtpServer(cfg, ds), buf
+	return NewServer(cfg, ds), buf
 }

 var sessionNum int

-func setupSmtpSession(server *Server) net.Conn {
+func setupSMTPSession(server *Server) net.Conn {
 	// Pair of pipes to communicate
 	serverConn, clientConn := net.Pipe()
 	// Start the session
@@ -392,6 +393,6 @@ func setupSmtpSession(server *Server) net.Conn {
 	return clientConn
 }

-func teardownSmtpServer(server *Server) {
+func teardownSMTPServer(server *Server) {
 	//log.SetOutput(os.Stderr)
 }
--- a/smtpd/listener.go
+++ b/smtpd/listener.go
@@ -13,7 +13,7 @@ import (
 	"github.com/jhillyerd/inbucket/log"
 )

-// Real server code starts here
+// Server holds the configuration and state of our SMTP server
 type Server struct {
 	domain          string
 	domainNoStore   string
@@ -46,37 +46,37 @@ var expConnectsHist = new(expvar.String)
 var expErrorsHist = new(expvar.String)
 var expWarnsHist = new(expvar.String)

-// Init a new Server object
-func NewSmtpServer(cfg config.SmtpConfig, ds DataStore) *Server {
+// NewServer creates a new Server instance with the specificed config
+func NewServer(cfg config.SMTPConfig, ds DataStore) *Server {
 	return &Server{dataStore: ds, domain: cfg.Domain, maxRecips: cfg.MaxRecipients,
 		maxIdleSeconds: cfg.MaxIdleSeconds, maxMessageBytes: cfg.MaxMessageBytes,
 		storeMessages: cfg.StoreMessages, domainNoStore: strings.ToLower(cfg.DomainNoStore),
 		waitgroup: new(sync.WaitGroup)}
 }

-// Main listener loop
+// Start the listener and handle incoming connections
 func (s *Server) Start() {
-	cfg := config.GetSmtpConfig()
+	cfg := config.GetSMTPConfig()
 	addr, err := net.ResolveTCPAddr("tcp4", fmt.Sprintf("%v:%v",
-		cfg.Ip4address, cfg.Ip4port))
+		cfg.IP4address, cfg.IP4port))
 	if err != nil {
-		log.LogError("Failed to build tcp4 address: %v", err)
+		log.Errorf("Failed to build tcp4 address: %v", err)
 		// TODO More graceful early-shutdown procedure
 		panic(err)
 	}

-	log.LogInfo("SMTP listening on TCP4 %v", addr)
+	log.Infof("SMTP listening on TCP4 %v", addr)
 	s.listener, err = net.ListenTCP("tcp4", addr)
 	if err != nil {
-		log.LogError("SMTP failed to start tcp4 listener: %v", err)
+		log.Errorf("SMTP failed to start tcp4 listener: %v", err)
 		// TODO More graceful early-shutdown procedure
 		panic(err)
 	}

 	if !s.storeMessages {
-		log.LogInfo("Load test mode active, messages will not be stored")
+		log.Infof("Load test mode active, messages will not be stored")
 	} else if s.domainNoStore != "" {
-		log.LogInfo("Messages sent to domain '%v' will be discarded", s.domainNoStore)
+		log.Infof("Messages sent to domain '%v' will be discarded", s.domainNoStore)
 	}

 	// Start retention scanner
@@ -96,12 +96,12 @@ func (s *Server) Start() {
 				if max := 1 * time.Second; tempDelay > max {
 					tempDelay = max
 				}
-				log.LogError("SMTP accept error: %v; retrying in %v", err, tempDelay)
+				log.Errorf("SMTP accept error: %v; retrying in %v", err, tempDelay)
 				time.Sleep(tempDelay)
 				continue
 			} else {
 				if s.shutdown {
-					log.LogTrace("SMTP listener shutting down on request")
+					log.Tracef("SMTP listener shutting down on request")
 					return
 				}
 				// TODO Implement a max error counter before shutdown?
@@ -119,15 +119,17 @@ func (s *Server) Start() {

 // Stop requests the SMTP server closes it's listener
 func (s *Server) Stop() {
-	log.LogTrace("SMTP shutdown requested, connections will be drained")
+	log.Tracef("SMTP shutdown requested, connections will be drained")
 	s.shutdown = true
-	s.listener.Close()
+	if err := s.listener.Close(); err != nil {
+		log.Errorf("Failed to close SMTP listener: %v", err)
+	}
 }

 // Drain causes the caller to block until all active SMTP sessions have finished
 func (s *Server) Drain() {
 	s.waitgroup.Wait()
-	log.LogTrace("SMTP connections drained")
+	log.Tracef("SMTP connections drained")
 }

 // When the provided Ticker ticks, we update our metrics history
--- a/smtpd/retention.go
+++ b/smtpd/retention.go
@@ -21,20 +21,22 @@ var expRetainedCurrent = new(expvar.Int)
 var retentionDeletesHist = list.New()
 var retainedHist = list.New()

-// History rendered as comma delim string
+// History rendered as comma delimited string
 var expRetentionDeletesHist = new(expvar.String)
 var expRetainedHist = new(expvar.String)

+// StartRetentionScanner launches a go-routine that scans for expired
+// messages, following the configured interval
 func StartRetentionScanner(ds DataStore) {
 	cfg := config.GetDataStoreConfig()
 	expRetentionPeriod.Set(int64(cfg.RetentionMinutes * 60))
 	if cfg.RetentionMinutes > 0 {
 		// Retention scanning enabled
-		log.LogInfo("Retention configured for %v minutes", cfg.RetentionMinutes)
+		log.Infof("Retention configured for %v minutes", cfg.RetentionMinutes)
 		go retentionScanner(ds, time.Duration(cfg.RetentionMinutes)*time.Minute,
 			time.Duration(cfg.RetentionSleep)*time.Millisecond)
 	} else {
-		log.LogInfo("Retention scanner disabled")
+		log.Infof("Retention scanner disabled")
 	}
 }

@@ -45,21 +47,21 @@ func retentionScanner(ds DataStore, maxAge time.Duration, sleep time.Duration) {
 		since := time.Since(start)
 		if since < time.Minute {
 			dur := time.Minute - since
-			log.LogTrace("Retention scanner sleeping for %v", dur)
+			log.Tracef("Retention scanner sleeping for %v", dur)
 			time.Sleep(dur)
 		}
 		start = time.Now()

 		// Kickoff scan
 		if err := doRetentionScan(ds, maxAge, sleep); err != nil {
-			log.LogError("Error during retention scan: %v", err)
+			log.Errorf("Error during retention scan: %v", err)
 		}
 	}
 }

 // doRetentionScan does a single pass of all mailboxes looking for messages that can be purged
 func doRetentionScan(ds DataStore, maxAge time.Duration, sleep time.Duration) error {
-	log.LogTrace("Starting retention scan")
+	log.Tracef("Starting retention scan")
 	cutoff := time.Now().Add(-1 * maxAge)
 	mboxes, err := ds.AllMailboxes()
 	if err != nil {
@@ -74,11 +76,11 @@ func doRetentionScan(ds DataStore, maxAge time.Duration, sleep time.Duration) er
 		}
 		for _, msg := range messages {
 			if msg.Date().Before(cutoff) {
-				log.LogTrace("Purging expired message %v", msg.Id())
+				log.Tracef("Purging expired message %v", msg.ID())
 				err = msg.Delete()
 				if err != nil {
 					// Log but don't abort
-					log.LogError("Failed to purge message %v: %v", msg.Id(), err)
+					log.Errorf("Failed to purge message %v: %v", msg.ID(), err)
 				} else {
 					expRetentionDeletesTotal.Add(1)
 				}
--- a/smtpd/retention_test.go
+++ b/smtpd/retention_test.go
@@ -36,7 +36,9 @@ func TestDoRetentionScan(t *testing.T) {
 	mb3.On("GetMessages").Return([]Message{new3}, nil)

 	// Test 4 hour retention
-	doRetentionScan(mds, 4*time.Hour, 0)
+	if err := doRetentionScan(mds, 4*time.Hour, 0); err != nil {
+		t.Error(err)
+	}

 	// Check our assertions
 	mds.AssertExpectations(t)
@@ -58,7 +60,7 @@ func TestDoRetentionScan(t *testing.T) {
 // Make a MockMessage of a specific age
 func mockMessage(ageHours int) *MockMessage {
 	msg := &MockMessage{}
-	msg.On("Id").Return(fmt.Sprintf("MSG[age=%vh]", ageHours))
+	msg.On("ID").Return(fmt.Sprintf("MSG[age=%vh]", ageHours))
 	msg.On("Date").Return(time.Now().Add(time.Duration(ageHours*-1) * time.Hour))
 	msg.On("Delete").Return(nil)
 	return msg
@@ -114,7 +116,7 @@ type MockMessage struct {
 	mock.Mock
 }

-func (m *MockMessage) Id() string {
+func (m *MockMessage) ID() string {
 	args := m.Called()
 	return args.String(0)
 }
--- a/smtpd/utils.go
+++ b/smtpd/utils.go
@@ -9,9 +9,10 @@ import (
 	"strings"
 )

-// Take "user+ext" and return "user", aka the mailbox we'll store it in
-// Return error if it contains invalid characters, we don't accept anything
-// that must be quoted according to RFC3696.
+// ParseMailboxName takes a localPart string (ex: "user+ext" without "@domain")
+// and returns just the mailbox name (ex: "user").  Returns an error if
+// localPart contains invalid characters; it won't accept any that must be
+// quoted according to RFC3696.
 func ParseMailboxName(localPart string) (result string, err error) {
 	if localPart == "" {
 		return "", fmt.Errorf("Mailbox name cannot be empty")
@@ -41,10 +42,14 @@ func ParseMailboxName(localPart string) (result string, err error) {
 	return result, nil
 }

-// Take a mailbox name and hash it into the directory we'll store it in
+// HashMailboxName accepts a mailbox name and hashes it.  Inbucket uses this as
+// the directory to house the mailbox
 func HashMailboxName(mailbox string) string {
 	h := sha1.New()
-	io.WriteString(h, mailbox)
+	if _, err := io.WriteString(h, mailbox); err != nil {
+		// This shouldn't ever happen
+		return ""
+	}
 	return fmt.Sprintf("%x", h.Sum(nil))
 }

@@ -138,15 +143,15 @@ LOOP:
 		switch {
 		case ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z'):
 			// Letters are OK
-			buf.WriteByte(c)
+			_ = buf.WriteByte(c)
 			inCharQuote = false
 		case '0' <= c && c <= '9':
 			// Numbers are OK
-			buf.WriteByte(c)
+			_ = buf.WriteByte(c)
 			inCharQuote = false
 		case bytes.IndexByte([]byte("!#$%&'*+-/=?^_`{|}~"), c) >= 0:
 			// These specials can be used unquoted
-			buf.WriteByte(c)
+			_ = buf.WriteByte(c)
 			inCharQuote = false
 		case c == '.':
 			// A single period is OK
@@ -154,13 +159,13 @@ LOOP:
 				// Sequence of periods is not permitted
 				return "", "", fmt.Errorf("Sequence of periods is not permitted")
 			}
-			buf.WriteByte(c)
+			_ = buf.WriteByte(c)
 			inCharQuote = false
 		case c == '\\':
 			inCharQuote = true
 		case c == '"':
 			if inCharQuote {
-				buf.WriteByte(c)
+				_ = buf.WriteByte(c)
 				inCharQuote = false
 			} else if inStringQuote {
 				inStringQuote = false
@@ -173,7 +178,7 @@ LOOP:
 			}
 		case c == '@':
 			if inCharQuote || inStringQuote {
-				buf.WriteByte(c)
+				_ = buf.WriteByte(c)
 				inCharQuote = false
 			} else {
 				// End of local-part
@@ -190,7 +195,7 @@ LOOP:
 			return "", "", fmt.Errorf("Characters outside of US-ASCII range not permitted")
 		default:
 			if inCharQuote || inStringQuote {
-				buf.WriteByte(c)
+				_ = buf.WriteByte(c)
 				inCharQuote = false
 			} else {
 				return "", "", fmt.Errorf("Character %q must be quoted", c)