~adnano/go-gemini-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH go-gemini] code review of error handling

~hugo_wetterberg <hugo_wetterberg@git.sr.ht>
Details
Message ID
<161005871500.20569.13636852121513173060-0@git.sr.ht>
DKIM signature
missing
Download raw message
Patch: +116 -46
From: Hugo Wetterberg <hugo@wetterberg.nu>

Error handling is currently missing is a couple of places. Most of
them are i/o related.

This change adds checks, an therefore sometimes also has to change
function signatures by adding an error return value. In the case of
the response writer the status and meta handling is changed and this
also breaks the API.

In some places where we don't have any reasonable I've added
assignment to a blank identifier to make it clear that we're ignoring
an error.

text: read the Err() that can be set by the scanner.

client: check if conn.SetDeadline() returns an error.

client: check if req.Write() returns an error.

fs: panic if mime type registration fails.

server: stop performing i/o in Header/Status functions

By deferring the actual header write to the first Write() or Flush()
call we don't have to do any error handling in Header() or Status().

As Server.respond() now defers a ResponseWriter.Flush() instead of
directly flushing the underlying bufio.Writer this has the added
benefit of ensuring that we always write a header
to the client, even if the responder is a complete NOOP.

tofu: return an error if we fail to write to the known hosts writer.
---
 client.go | 15 +++++++--
 fs.go     | 23 ++++++++++----
 mux.go    |  6 ++--
 server.go | 94 ++++++++++++++++++++++++++++++++++++++-----------------
 text.go   | 15 ++++++---
 tofu.go   |  9 ++++--
 6 files changed, 116 insertions(+), 46 deletions(-)

diff --git a/client.go b/client.go
index 8c34242..5afebdd 100644
--- a/client.go
+++ b/client.go
@@ -6,6 +6,7 @@ import (
	"crypto/tls"
	"crypto/x509"
	"errors"
	"fmt"
	"net"
	"strings"
	"time"
@@ -74,12 +75,22 @@ func (c *Client) Do(req *Request) (*Response, error) {
	conn := tls.Client(netConn, config)
	// Set connection deadline
	if c.Timeout != 0 {
		conn.SetDeadline(time.Now().Add(c.Timeout))
		err := conn.SetDeadline(time.Now().Add(c.Timeout))
		if err != nil {
			return nil, fmt.Errorf(
				"failed to set connection deadline: %w", err)
		}
	}

	// Write the request
	w := bufio.NewWriter(conn)
	req.Write(w)

	err = req.Write(w)
	if err != nil {
		return nil, fmt.Errorf(
			"failed to write request data: %w", err)
	}

	if err := w.Flush(); err != nil {
		return nil, err
	}
diff --git a/fs.go b/fs.go
index c4cf3d3..c08922d 100644
--- a/fs.go
+++ b/fs.go
@@ -1,6 +1,7 @@
package gemini

import (
	"fmt"
	"io"
	"mime"
	"os"
@@ -9,8 +10,18 @@ import (

func init() {
	// Add Gemini mime types
	mime.AddExtensionType(".gmi", "text/gemini")
	mime.AddExtensionType(".gemini", "text/gemini")
	err := mime.AddExtensionType(".gmi", "text/gemini")
	must("register .gmi extension mimetype", err)

	err = mime.AddExtensionType(".gemini", "text/gemini")
	must("register .gemini extension mimetype", err)

}

func must(action string, err error) {
	if err != nil {
		panic(fmt.Errorf("failed to %s: %w", action, err))
	}
}

// FileServer takes a filesystem and returns a Responder which uses that filesystem.
@@ -27,7 +38,7 @@ func (fsh fsHandler) Respond(w *ResponseWriter, r *Request) {
	p := path.Clean(r.URL.Path)
	f, err := fsh.Open(p)
	if err != nil {
		w.WriteStatus(StatusNotFound)
		w.Status(StatusNotFound)
		return
	}
	// Detect mimetype
@@ -35,7 +46,7 @@ func (fsh fsHandler) Respond(w *ResponseWriter, r *Request) {
	mimetype := mime.TypeByExtension(ext)
	w.SetMediaType(mimetype)
	// Copy file to response writer
	io.Copy(w, f)
	_, _ = io.Copy(w, f)
}

// TODO: replace with io/fs.FS when available
@@ -66,7 +77,7 @@ func (d Dir) Open(name string) (File, error) {
func ServeFile(w *ResponseWriter, fs FS, name string) {
	f, err := fs.Open(name)
	if err != nil {
		w.WriteStatus(StatusNotFound)
		w.Status(StatusNotFound)
		return
	}
	// Detect mimetype
@@ -74,7 +85,7 @@ func ServeFile(w *ResponseWriter, fs FS, name string) {
	mimetype := mime.TypeByExtension(ext)
	w.SetMediaType(mimetype)
	// Copy file to response writer
	io.Copy(w, f)
	_, _ = io.Copy(w, f)
}

func openFile(p string) (File, error) {
diff --git a/mux.go b/mux.go
index 81a4ee7..0c565ea 100644
--- a/mux.go
+++ b/mux.go
@@ -138,14 +138,14 @@ func (mux *ServeMux) Respond(w *ResponseWriter, r *Request) {
	// If the given path is /tree and its handler is not registered,
	// redirect for /tree/.
	if u, ok := mux.redirectToPathSlash(path, r.URL); ok {
		w.WriteHeader(StatusRedirect, u.String())
		w.Header(StatusRedirect, u.String())
		return
	}

	if path != r.URL.Path {
		u := *r.URL
		u.Path = path
		w.WriteHeader(StatusRedirect, u.String())
		w.Header(StatusRedirect, u.String())
		return
	}

@@ -154,7 +154,7 @@ func (mux *ServeMux) Respond(w *ResponseWriter, r *Request) {

	resp := mux.match(path)
	if resp == nil {
		w.WriteStatus(StatusNotFound)
		w.Status(StatusNotFound)
		return
	}
	resp.Respond(w, r)
diff --git a/server.go b/server.go
index 1f9078a..8d09382 100644
--- a/server.go
+++ b/server.go
@@ -4,10 +4,10 @@ import (
	"bufio"
	"crypto/tls"
	"errors"
	"fmt"
	"io"
	"log"
	"net"
	"strconv"
	"strings"
	"time"
)
@@ -176,18 +176,20 @@ func (s *Server) getCertificateFor(hostname string) (*tls.Certificate, error) {
func (s *Server) respond(conn net.Conn) {
	defer conn.Close()
	if d := s.ReadTimeout; d != 0 {
		conn.SetReadDeadline(time.Now().Add(d))
		_ = conn.SetReadDeadline(time.Now().Add(d))
	}
	if d := s.WriteTimeout; d != 0 {
		conn.SetWriteDeadline(time.Now().Add(d))
		_ = conn.SetWriteDeadline(time.Now().Add(d))
	}

	w := NewResponseWriter(conn)
	defer w.b.Flush()
	defer func() {
		_ = w.Flush()
	}()

	req, err := ReadRequest(conn)
	if err != nil {
		w.WriteStatus(StatusBadRequest)
		w.Status(StatusBadRequest)
		return
	}

@@ -206,7 +208,7 @@ func (s *Server) respond(conn net.Conn) {

	resp := s.responder(req)
	if resp == nil {
		w.WriteStatus(StatusNotFound)
		w.Status(StatusNotFound)
		return
	}

@@ -236,6 +238,8 @@ func (s *Server) logf(format string, args ...interface{}) {

// ResponseWriter is used by a Gemini handler to construct a Gemini response.
type ResponseWriter struct {
	status      Status
	meta        string
	b           *bufio.Writer
	bodyAllowed bool
	wroteHeader bool
@@ -249,34 +253,28 @@ func NewResponseWriter(w io.Writer) *ResponseWriter {
	}
}

// WriteHeader writes the response header.
// If the header has already been written, WriteHeader does nothing.
// Header sets the response header.
//
// Meta contains more information related to the response status.
// For successful responses, Meta should contain the mimetype of the response.
// For failure responses, Meta should contain a short description of the failure.
// Meta should not be longer than 1024 bytes.
func (w *ResponseWriter) WriteHeader(status Status, meta string) {
	if w.wroteHeader {
		return
	}
	w.b.WriteString(strconv.Itoa(int(status)))
	w.b.WriteByte(' ')
	w.b.WriteString(meta)
	w.b.Write(crlf)
func (w *ResponseWriter) Header(status Status, meta string) {
	w.status = status
	w.meta = meta
}

// Status sets the response header to the given status code.
//
// Status is equivalent to Header(status, status.Message())
func (w *ResponseWriter) Status(status Status) {
	meta := status.Message()

	// Only allow body to be written on successful status codes.
	if status.Class() == StatusClassSuccess {
		w.bodyAllowed = true
		meta = w.mediatype
	}
	w.wroteHeader = true
}

// WriteStatus writes the response header with the given status code.
//
// WriteStatus is equivalent to WriteHeader(status, status.Message())
func (w *ResponseWriter) WriteStatus(status Status) {
	w.WriteHeader(status, status.Message())
	w.Header(status, meta)
}

// SetMediaType sets the media type that will be written for a successful response.
@@ -293,20 +291,58 @@ func (w *ResponseWriter) SetMediaType(mediatype string) {
// with StatusSuccess and the mimetype set in SetMimetype.
func (w *ResponseWriter) Write(b []byte) (int, error) {
	if !w.wroteHeader {
		mediatype := w.mediatype
		if mediatype == "" {
			mediatype = "text/gemini"
		err := w.writeHeader()
		if err != nil {
			return 0, err
		}
		w.WriteHeader(StatusSuccess, mediatype)
	}

	if !w.bodyAllowed {
		return 0, ErrBodyNotAllowed
	}

	return w.b.Write(b)
}

func (w *ResponseWriter) writeHeader() error {
	status := w.status
	if status == 0 {
		status = StatusSuccess
	}

	meta := w.meta

	if status.Class() == StatusClassSuccess {
		w.bodyAllowed = true

		if meta == "" {
			meta = w.mediatype
		}

		if meta == "" {
			meta = "text/gemini"
		}
	}

	_, err := fmt.Fprintf(w.b, "%d %s\r\n", int(status), meta)
	if err != nil {
		return fmt.Errorf("failed to write response header: %w", err)
	}

	w.wroteHeader = true

	return nil
}

// Flush writes any buffered data to the underlying io.Writer.
func (w *ResponseWriter) Flush() error {
	if !w.wroteHeader {
		err := w.writeHeader()
		if err != nil {
			return err
		}
	}

	return w.b.Flush()
}

diff --git a/text.go b/text.go
index 9b77982..c77b642 100644
--- a/text.go
+++ b/text.go
@@ -88,17 +88,22 @@ func (l LineText) line()                {}
type Text []Line

// ParseText parses Gemini text from the provided io.Reader.
func ParseText(r io.Reader) Text {
func ParseText(r io.Reader) (Text, error) {
	var t Text
	ParseLines(r, func(line Line) {

	err := ParseLines(r, func(line Line) {
		t = append(t, line)
	})
	return t
	if err != nil {
		return nil, err
	}

	return t, nil
}

// ParseLines parses Gemini text from the provided io.Reader.
// It calls handler with each line that it parses.
func ParseLines(r io.Reader, handler func(Line)) {
func ParseLines(r io.Reader, handler func(Line)) error {
	const spacetab = " \t"
	var pre bool
	scanner := bufio.NewScanner(r)
@@ -149,6 +154,8 @@ func ParseLines(r io.Reader, handler func(Line)) {
		}
		handler(line)
	}

	return scanner.Err()
}

// String writes the Gemini text response to a string and returns it.
diff --git a/tofu.go b/tofu.go
index 9e93ac2..9a252d6 100644
--- a/tofu.go
+++ b/tofu.go
@@ -52,12 +52,17 @@ func (k *KnownHostsFile) Lookup(hostname string) (Fingerprint, bool) {
}

// Write writes a known hosts entry to the configured output.
func (k *KnownHostsFile) Write(hostname string, fingerprint Fingerprint) {
func (k *KnownHostsFile) Write(hostname string, fingerprint Fingerprint) error {
	k.mu.RLock()
	defer k.mu.RUnlock()
	if k.out != nil {
		k.writeKnownHost(k.out, hostname, fingerprint)
		_, err := k.writeKnownHost(k.out, hostname, fingerprint)
		if err != nil {
			return fmt.Errorf("failed to write to known host file: %w", err)
		}
	}

	return nil
}

// WriteAll writes all of the known hosts to the provided io.Writer.
-- 
2.26.2
Details
Message ID
<C8E06H3YWJTP.1DWDIMLUH98YL@nitro>
In-Reply-To
<161005871500.20569.13636852121513173060-0@git.sr.ht> (view parent)
DKIM signature
pass
Download raw message
Thanks for the patch! I left some feedback below.

On Thu Jan 7, 2021 at 5:08 PM EST, ~hugo_wetterberg wrote:
> + err := mime.AddExtensionType(".gmi", "text/gemini")
> + must("register .gmi extension mimetype", err)
> +
> + err = mime.AddExtensionType(".gemini", "text/gemini")
> + must("register .gemini extension mimetype", err)

I would rather inline the calls to panic here. A new function is not
really needed if it only saves a few lines of code.

> + _, err := fmt.Fprintf(w.b, "%d %s\r\n", int(status), meta)

It might be better not to use fmt here for performance reasons.

> +func ParseText(r io.Reader) (Text, error) {
> var t Text
> - ParseLines(r, func(line Line) {
> +
> + err := ParseLines(r, func(line Line) {
> t = append(t, line)
> })
> - return t
> + if err != nil {
> + return nil, err
> + }
> +
> + return t, nil

I think it would be better to remove the if branch and return t, err.
That way the function will return whatever text it parsed so far, along
with the error that interrupted it.
Details
Message ID
<C8F7KFPEDQI4.36CH9I2DYL7SZ@nitro>
In-Reply-To
<C8E06H3YWJTP.1DWDIMLUH98YL@nitro> (view parent)
DKIM signature
pass
Download raw message
I've pushed the patch with some minor changes. Thanks!

To git.sr.ht:~adnano/go-gemini
   efef44c..f2921a3  master -> master
Details
Message ID
<7f83f7e7fc04f44c2d396720263e92b0789cff00.camel@wetterberg.nu>
In-Reply-To
<C8F7KFPEDQI4.36CH9I2DYL7SZ@nitro> (view parent)
DKIM signature
missing
Download raw message
Looks great. Good call on unifying meta/mimetype.

I didn't know that the bufio.Writer held on to potential errors for all
subsequent write/flush calls. That's really neat.

On Sun, 2021-01-10 at 04:55 +0000, Adnan Maolood wrote:
> I've pushed the patch with some minor changes. Thanks!
> 
> To git.sr.ht:~adnano/go-gemini
>    efef44c..f2921a3  master -> master
Reply to thread Export thread (mbox)