Port pinger command line tool

Checking if some port is up on the network is a very common task when working with remote services. A common way to check is using telnet, but I have two practical issues with that:

• telnet is not available in all systems, for example in recent versions of Windows.

• When connection is successful with telnet, an interactive shell may start, which you have to exit by pressing Control] followed by Controld. It's OK, but not nearly as easy as running a command that simply exits with 0 on success and non-zero on failure. For the same reason, this method using telnet is not so easily scriptable.

(In case you're wondering, I excluded nmap as an alternative, because it can be used for far more than pinging ports. As far as I know, it's not recommended to have it lying around, unless you're a security expert and it's your job to use it on a daily basis.)

To solve these issues, I started a simple command line tool in go.
The source code is on GitHub.
I'm still a beginner of this language,
I welcome any and all comments about the implementation, testing, project organization, or anything else.

The main module, portping.go:

package main

import (
    "net"
    "fmt"
    "regexp"
)

var pattern_getsockopt = regexp.MustCompile(`getsockopt: (.*)`)
var pattern_other = regexp.MustCompile(`^dial tcp: (.*)`)

func Ping(host string, port int) error {
    addr := fmt.Sprintf("%s:%d", host, port)
    conn, err := net.Dial("tcp", addr)

    if err == nil {
        conn.Close()
    }
    return err
}

func PingN(host string, port int, count int, c chan error) {
    for i := 0; i < count; i++ {
        c <- Ping(host, port)
    }
}

func FormatResult(err error) string {
    if err == nil {
        return "success"
    }
    s := err.Error()
    if result := pattern_getsockopt.FindStringSubmatch(s); result != nil {
        return result[1]
    }
    if result := pattern_other.FindStringSubmatch(s); result != nil {
        return result[1]
    }
    return s
}

Uni

In general, I am impressed with the test coverage, the attention to detail, and the overall structure of the code. It is easy to follow, and understand.

Having said that, there are a few places where it can be improved.

flag processing

You code uses flag directly to set up the help mechanisms, and also for the count parameter. You should consider driving a FlagSet directly. Your code:

flag.Usage = func() {
    fmt.Printf("Usage: %s [options] host port\n\n", os.Args[0])
    flag.PrintDefaults()
}

countPtr := flag.Int("c", defaultCount, "stop after count connections")
flag.Parse()

could be reduced to:

summary := fmt.Sprintf("%s [options] host port", os.Args[0])
fs := flag.NewFlagSet(summary, flag.ExitOnError)
countPtr := fs.Int("c", defaultCount, "stop after count connections")
fs.Parse(os.Args[1:])

You will need to change a couple of flag references to fs after the above change.

I find FlagSet to be useful especially when using go-like sub-command processing.

error handling

In your code you use regular expressions to parse error messages, and simplify them for presentation.

The parsing of error messages is an anti-pattern in Go. An improved mechanism is to do type assertion on the error, and to directly manipulate the results. So, for example, the net package is documented to normally return OpError instances of an error. You can use this in your code to handle the error better (see Error handling and go blog and more specifically type switches):

func FormatResult(err error) string {
    if err == nil {
        return "success"
    }
    switch err := err.(type) {
    case *net.OpError:
        return err.Err.Error()
    default:
        return err.Error()
    }
}

Note how the type assertion err := err.(type) creates a new err instance inside the scope of the switch. The go language guarantees that the resulting err will be correctly typed in the relevant case blocks, so in this example, the case statement return err.Err.Error() is referring to the field Err on OpError.

While on the subject of error handling. It's a very common pattern in Go to have checks for NOT-NIL errors. Checks for nil errors are uncommon, and will likely be missed. Thus, this code, even though it is right, will probably be mis-read by people on the first scan:

conn, err := net.Dial("tcp", addr)
if err == nil {
    conn.Close()
}

Instead, it's common to check for an error, not the lack of one....

conn, err := net.Dial("tcp", addr)
if err != nil {
    return err
}
conn.Close()

The above code is also typically done with a "defer", but because your code does nothing else after that point, the defer would be useless, but I would still consider writing the full function as:

func Ping(host string, port int) error {
    addr := fmt.Sprintf("%s:%d", host, port)
    conn, err := net.Dial("tcp", addr)
    if err == nil {
        return err
    }
    defer conn.Close()
    return nil
}

Random ports

Heh, port 1234 - I use that port all the time for "junk" things. It seems easy.

A better solution is to choose a random, known-available port, and to then return that port as part of the call.

With TCP (and UDP) if you specify a port of 0 when you set up a listening socket, it will choose a "random" (for some degree of random), unused, ephemeral port. you can use this to your advantage.

In your tests, you use a go-routine to manage that port, but if you set up the listener before the goroutine happens, you can return the port as part of the call. So, currently your code does:

go acceptN(testHost, testPort, pingCount)

but, instead, we want something like:

port := acceptNoPortN(testHost, pingCount)

We can set that up by changing the acceptN function to something like:

func acceptNoPortN(host string, count int, t *testing.T) int {
    tcpa, err := net.ResolveTCPAddr("tcp", host+":0")
    if err != nil {
        t.Fatal(err)
    }
    ln, err := net.ListenTCP("tcp", tcpa)
    if err != nil {
        t.Fatal(err)
    }
    local, ok := ln.Addr().(*net.TCPAddr)
    if !ok {
        t.Fatalf("Unable to convert Addr to TCPAddr")
    }

    go func() {
        defer ln.Close()
        for i := 0; i < count; i++ {
            conn, err := ln.Accept()
            if err != nil {
                log.Fatal(err)
            }
            conn.Close()
        }
    }()

    return local.Port
}

Notice something in there - using the "TCP" versions of the various Dial, Resolve, and Listen methods. They help with getting instances in to the right/useful types.

The above code also changes the "String()" results in some places.... and I think that is causing some other tests to fail... maybe.

Also, it Still Has a race condition - it may not close the listener socket before the next system tries to use the port. That's OK, though, because the next attempt to do a listener should choose a different port.

Testing

In your tests, you use a comb

StackExchange Code Review Q#124078, answer score: 3