Making a ping library. Should I follow the actual ping behaviour?

Question

I am making a ping library, mainly for fun.

I recently found a bug in my implementation, where I did not checked the seq of the received packed. I have now fixed it by discarding a packet if the timeout have occured.

But today, I saw that the ping utility print the received reply packet, even if they got a timeout.

Request timeout for icmp_seq 2
Request timeout for icmp_seq 3
64 bytes from 80.67.169.18: icmp_seq=2 ttl=58 time=2216.104 ms
64 bytes from 80.67.169.18: icmp_seq=3 ttl=58 time=1216.559 ms

I don't know what to do in my library. Should I keep the actual behaviour, or do I need to adjust it to the "old" ping way?

/*
Package libping provide the ability to send ICMP packets easily.
*/
package libping

import (
    "bytes"
    "net"
    "os"
    "time"
)

const (
    ICMP_ECHO_REQUEST = 8
    ICMP_ECHO_REPLY   = 0
)

// The struct Response is the data returned by Pinguntil.
type Response struct {
    Delay       time.Duration
    Error       error
    Destination string
    Seq         int
    Readsize    int
    Writesize   int
}

func makePingRequest(id, seq, pktlen int, filler []byte) []byte {
    p := make([]byte, pktlen)
    copy(p[8:], bytes.Repeat(filler, (pktlen-8)/len(filler)+1))

    p[0] = ICMP_ECHO_REQUEST // type
    p[1] = 0                 // code
    p[2] = 0                 // cksum
    p[3] = 0                 // cksum
    p[4] = uint8(id >> 8)    // id
    p[5] = uint8(id & 0xff)  // id
    p[6] = uint8(seq >> 8)   // sequence
    p[7] = uint8(seq & 0xff) // sequence

    // calculate icmp checksum
    cklen := len(p)
    s := uint32(0)
    for i := 0; i < (cklen - 1); i += 2 {
        s += uint32(p[i+1])<<8 | uint32(p[i])
    }
    if cklen&1 == 1 {
        s += uint32(p[cklen-1])
    }
    s = (s >> 16) + (s & 0xffff)
    s = s + (s >> 16)

    // place checksum back in header; using ^= avoids the
    // assumption the checksum bytes are zero
    p[2] ^= uint8(^s & 0xff)
    p[3] ^= uint8(^s >> 8)

    return p
}

func parsePingReply(p []byte) (id, seq, code int) {
    id = int(p[24])<<8 | int(p[25])
    seq = int(p[26])<<8 | int(p[27])
    code = int(p[21])
    return
}

// Pingonce send one ICMP echo packet to the destination, and return the latency.
// The function is made to be simple. Simple request, simple reply.
func Pingonce(destination string) (time.Duration, error) {
    response := make(chan Response)
    go Pinguntil(destination, 1, response, time.Second)
    answer := <-response
    return answer.Delay, answer.Error
}

// Pinguntil will send ICMP echo packets to the destination until the counter is reached, or forever if the counter is set to 0.
// The replies are given in the Response format.
// You can also adjust the delay between two ICMP echo packets with the variable delay.
func Pinguntil(destination string, count int, response chan Response, delay time.Duration) {
    raddr, err := net.ResolveIPAddr("ip", destination)
    if err != nil {
        response <- Response{Delay: 0, Error: err, Destination: destination, Seq: 0}
        close(response)
        return
    }

    ipconn, err := net.Dial("ip:icmp", raddr.IP.String())
    if err != nil {
        response <- Response{Delay: 0, Error: err, Destination: raddr.IP.String(), Seq: 0}
        close(response)
        return
    }

    sendid := os.Getpid() & 0xffff
    pingpktlen := 64
    seq := 0
    var elapsed time.Duration = 0

    for ; seq < count || count == 0; seq++ {
        elapsed = 0
        if seq > 65535 { // The two bytes for seq. Don't overflow!
            seq = 0
        }
        sendpkt := makePingRequest(sendid, seq, pingpktlen, []byte("Go Ping"))

        start := time.Now()

        writesize, err := ipconn.Write(sendpkt)
        if err != nil || writesize != pingpktlen {
            response <- Response{Delay: 0, Error: err, Destination: raddr.IP.String(), Seq: seq, Writesize: writesize, Readsize: 0}
            time.Sleep(delay)
            continue
        }

        ipconn.SetReadDeadline(time.Now().Add(time.Second * 1)) // 1 second

        resp := make([]byte, 1024)
        for {
            readsize, err := ipconn.Read(resp)

            elapsed = time.Now().Sub(start)

            rid, rseq, rcode := parsePingReply(resp)

            if err != nil {
                response <- Response{Delay: 0, Error: err, Destination: raddr.IP.String(), Seq: seq, Writesize: writesize, Readsize: readsize}
                break
            } else if rcode != ICMP_ECHO_REPLY || rseq != seq || rid != sendid {
                continue
            } else {
                response <- Response{Delay: elapsed, Error: err, Destination: raddr.IP.String(), Seq: seq, Writesize: writesize, Readsize: readsize}
                break
            }
        }
        time.Sleep(delay - elapsed)
    }
    close(response)
}

The library is not made for a specitif usage. I will use it to do a few projects, but I want to know the arguments for each choices.

And as I looked, implementing the second option will be way harder.

Thanks! (If my post is not clear, don't hesitate to ask me to clarify, it's late.)

If you want to check the project address : here

Answer 1

I understood your question to be: 'Should I report packets to the user that I have already reported as being timed out'.

No, I would not do this. In an application I would not expect packets twice and I would have to do the bookkeeping for these manually. If your library does the bookkeeping and I can ask on a later point in time whether the packet was received some later time, that would be OK.

So either no or some API like this:

notifyReceivedLostPacket(seqId int) chan Packet

Answer 2

I'll make the opposite vote:

Yes, you should do this. We got a response back; it should be reported. The application will have to figure out what to do with it.

Ask