Go command-line tool to do some simple byte manipulation

Question

I have some files containing 16-bit words in big-endian order, and a tool to process them that assumes little-endian order. I therefore need to swap the order of each pair of bytes in my files.

This is almost a one-liner in Python (data[0::2], data[1::2] = data[1::2], data[0::2]). However, since I need to do this manipulation often, I decided to write a standalone tool to do it. Also, for learning purposes, I decided to write this simple tool as my first Go program.

I look forward to any comments you may have about this code:

byteswap16.go

package main

import "fmt"
import "io/ioutil"
import "os"

func parseArgs(args []string) (string, string, error) {
    if len(args) != 3 {
        return "", "", fmt.Errorf("Usage: %s infile outfile", args[0])
    }

    return args[1], args[2], nil
}

func byteswap16(data []byte) ([]byte, error) {
    length := len(data)
    if length%2 != 0 {
        return nil, fmt.Errorf("Length is not a multiple of 2 bytes")
    }

    swapped := make([]byte, length)
    for i := 0; i < length; i++ {
        swapped[i] = data[i^1]
    }

    return swapped, nil
}

func run() error {
    infile, outfile, err := parseArgs(os.Args)
    if err != nil {
        return err
    }

    data, err := ioutil.ReadFile(infile)
    if err != nil {
        return err
    }

    swapped, err := byteswap16(data)
    if err != nil {
        return fmt.Errorf("%s: %v", infile, err)
    }

    if err := ioutil.WriteFile(outfile, swapped, 0666); err != nil {
        return err
    }

    return nil
}

func main() {
    if err := run(); err != nil {
        fmt.Fprintf(os.Stderr, "Error: %v\n", err)
        os.Exit(1)
    }
}

A couple of comments of my own:

1. As with the Python, the core algorithm is very little code, but it's written very differently - an explicit for loop over individual bytes rather than higher-level manipulation of many values at a time. I think I like the explicit loop more, because it's a more general solution - changing to byteswap32 wouldn't require much more than changing i^1 to i^3.

2. A common complaint about Go, but I don't really like the repetitive if err != nil in run. However, it does provide an obvious place to wrap err to add context, as is done after the call to the byteswap16 function. If exceptions were used instead, the code would be more concise but the error message would be worse, because I don't think I would catch and re-throw to add context.

In general, would this be considered good-quality, idiomatic Go code?

Answer 1

Several things come to mind right away:

• I wouldn't make a custom parseArgs function, I'd use flag.Parse. For just a pair of positional arguments it might be overkill but it makes it trivial to add more options, usage output, etc. And either way, it's better to only access os.Args from main

• Instead of fmt.Fprint to os.Stderr followed by os.Exit it's more idomatic to just use log.Fatal (or Fatalf or Fatalln).

• Avoid reading and writing entire files with ioutil if you can do work while streaming data. If the files are guarenteed to be small it's just a bad habit but if someone ever runs your tool on gigabytes or terabytes of data they'll be in for unpleasant surprise.

• In the same vein, I'd make your function API steam-able. Either by implementing an io.Reader wrapper or by a function that takes an io.Writer and an io.Reader (like io.Copy).

  You could also implementing it as a transform.Transformer; but that's probably overkill, although perhaps an interesting exercise.

• Typically such command line tools in unix act as pipes, that is, if not given file arguments they read from stdin and write to stdout. Either you only work with stdin/stdout and make callers use shell redirection if they want to work to work on files or you make the filenames optional. Just initialise an io.Reader (or io.ReadCloser) variable to os.Stdin and an io.WriteCloesr variable to os.Stdout. Then when processing the arguments, if provided, open the relevant files and change the variables. Your choice if you use positional arguments as you've done or option arguments (as dd does and as I did below). Make sure to check for errors closing the destination.

• By the way, on unix you can use the standard dd command to do the byte swapping for you with the conv=swab operand:

  dd conv=swab if=someinput_file of=someoutput_file
  

  using files, or in a pipe like this:

  bigendian_command | dd conv=swab | littleendian_command
  

A quick stab at implementing it this way gave me the following.

(Also available at https://gist.github.com/dchapes/9d795a04e471319abbc5ff016afbbee9 the gist also has your version as revision 1, and a version using a function with an io.Copy like signature as revision 2).

swab.go:

package main

import (
    "flag"
    "fmt"
    "io"
    "log"
    "os"
)

func main() {
    log.SetPrefix("swab: ")
    log.SetFlags(0)
    infile := flag.String("in", "", "input `path`, blank for stdin")
    outfile := flag.String("out", "", "output `path`, blank for stdout")
    flag.Usage = func() {
        fmt.Fprintf(flag.CommandLine.Output(),
            "Usage: %s [options]\n", os.Args[0],
        )
        flag.PrintDefaults()
    }
    flag.Parse()
    if flag.NArg() > 0 {
        flag.Usage()
        os.Exit(2) // To match the exit code flag.Parse uses.
    }

    var src io.ReadCloser = os.Stdin
    var dst io.WriteCloser = os.Stdout
    if *infile != "" {
        f, err := os.Open(*infile)
        if err != nil {
            log.Fatal(err)
        }
        src = f
    }
    // Closing the input isn't strictly required in main
    // nor for stdio, but it's a good habit. No need to
    // check any error; we rely on Read reporting errors of interest.
    defer src.Close()

    if *outfile != "" {
        f, err := os.Create(*outfile)
        if err != nil {
            log.Fatal(err)
        }
        dst = f
    }

    if _, err := io.Copy(dst, NewSwabReader(src)); err != nil {
        // Not this calls os.Exit so no defers get run
        // and we don't close the output either, not
        // an issue from main.
        log.Fatal(err)
    }
    if err := dst.Close(); err != nil {
        log.Fatal(err)
    }
}

type SwabReader struct {
    r        io.Reader
    b        byte // extra byte, not yet swapped
    haveByte bool // true if b is valid
    err      error
}

// NewSwabReader returns an io.Reader that reads from r
// swapping adjacent bytes. The trailing odd byte, if any,
// is left as-is.
func NewSwabReader(r io.Reader) *SwabReader {
    return &SwabReader{r: r}
}

func (sr *SwabReader) Read(p []byte) (n int, err error) {
    if len(p) == 0 || sr.err != nil {
        return 0, sr.err
    }
    i := 0
    if sr.haveByte {
        // Copy in the previous saved byte.
        p[0] = sr.b
        i = 1
        //sr.haveByte = false // not strictly required
    }
    n, sr.err = sr.r.Read(p[i:])
    n += i
    p = p[:n]
    for i := 1; i < len(p); i += 2 {
        p[i-1], p[i] = p[i], p[i-1]
    }
    // Remove and save any non-swapped trailing odd byte.
    if sr.err == nil {
        if sr.haveByte = (n&1 != 0); sr.haveByte {
            n--
            sr.b = p[n]
            //p = p[:n] // not strictly required
        }
    }
    return n, sr.err
}

And a simple test, swab_test.go:

package main

import (
    "io"
    "strings"
    "testing"
    "testing/iotest"
)

var readFilters = []struct {
    name string
    fn   func(io.Reader) io.Reader
}{
    {"", nil},
    {"DataErrReader", iotest.DataErrReader},
    {"HalfReader", iotest.HalfReader},
    {"OneByteReader", iotest.OneByteReader},
    //{"TimeoutReader", iotest.TimeoutReader},
}

func TestSwab(t *testing.T) {
    const sz = 32<<10 + 1
    cases := []struct{ in, out string }{
        {"", ""},
        {"a", "a"},
        {"ab", "ba"},
        {"abc", "bac"},
        {"abcd", "badc"},
        {strings.Repeat("\x01\x80", sz) + "x",
            strings.Repeat("\x80\x01", sz) + "x"},
    }
    var dst strings.Builder
    var r io.Reader
    for _, rf := range readFilters {
        for _, tc := range cases {
            r = strings.NewReader(tc.in)
            if rf.fn != nil {
                r = rf.fn(r)
            }
            dst.Reset()
            //t.Logf("swabbing %s %.16q", rf.name, tc.in)
            //r = iotest.NewReadLogger("<<< src", r)
            n, err := io.Copy(&dst, NewSwabReader(r))
            if err != nil {
                t.Errorf("swab on %s %q failed: %v",
                    rf.name, tc.in, err,
                )
                continue
            }
            if want := int64(len(tc.out)); n != want {
                t.Errorf("swab on %s %q returned n=%d, want %d",
                    rf.name, tc.in, n, want,
                )
            }
            if got := dst.String(); got != tc.out {
                t.Errorf("swab on %s %q\n\tgave %q\n\twant %q",
                    rf.name, tc.in, got, tc.out,
                )
            }
        }
    }
}