Avoid cgo in lib/overlay/conn to simplify cross-compilation

[matzf]

cgo was only required to get a portable struct timespec.
The syscall.Timespec struct seems to provide this type just as well, without requiring cgo.

---

This change is 

[matzf]

@kormat asked for documentation that unsafe.Sizeof(syscall.Timespec) is guaranteed to be the same as what C considers.

I could not find any actual documentation (I find golang documentation to be rather lackluster). Still, this seems rather safe. Looking at the various syscall/types_xxx.go source files (see https://golang.org/src/syscall), Timespec is defined as type Timespec C.struct_timespec on most platforms. It's explicitly defined for windows, plan9 and nacl.

This is the complete list of unsafe.Sizeof(syscall.Timespec) for all supported GOOS/GOARCH combinations:

darwin/386: 8
darwin/amd64: 16
dragonfly/amd64: 16
freebsd/386: 8
freebsd/amd64: 16
freebsd/arm: 16
linux/386: 8
linux/amd64: 16
linux/arm: 8
linux/arm64: 16
linux/ppc64: 16
linux/ppc64le: 16
linux/mips: 8
linux/mipsle: 8
linux/mips64: 16
linux/mips64le: 16
linux/s390x: 16
nacl/386: 12
nacl/amd64p32: 16
nacl/arm: 12
netbsd/386: 12
netbsd/amd64: 16
netbsd/arm: 16
openbsd/386: 12
openbsd/amd64: 16
openbsd/arm: 16
plan9/386: 8
plan9/amd64: 8
plan9/arm: 8
solaris/amd64: 16
windows/386: 16
windows/amd64: 16

This corresponds to sizeof(struct timespec) in C for all platforms that I could check: see this compiler comparison on godbolt: https://godbolt.org/z/Lo3ekb

[kormat]

I'm still not 100% comfortable. For example:

• The go std lib seems to always (or at least as far as i could find from a quick search) use C.sizeof... for this.
• I've found at least one issue in the past where the unsafe.Sizeof value for a given C struct changed between go versions: cmd/cgo: size of struct with 0 length trailing field has changed golang/go#11925

Reviewable status: 0 of 3 files reviewed, all discussions resolved

[matzf]

It seems unlikely to me that this could be an issue as timespec is a particularly simple data type.
The issue you refer to with zero-length struct members really seems different; the cgo documentation (now) explicitly mentions that this isn't supported.

Regardless, as the in our case the sizeof is only used to allocate a big enough buffer, maybe a compromise could be to explicitly use a constant size that can be expected to be >= sizeof(struct timespec) on any platform? Like, e.g. 16?

Btw. looking at the documentation for SO_RXQ_OVFL (the other value that is requested into/read from the oob), the value is defined as a 32-bit integer but it's currently read as a golang int. Is that intentional or a typo?

[kormat]

Thanks for the work you've done looking into this. <3

I've been doing some research, and discovered some interesting things

• time_t isn't guaranteed to be an integer(!) in the spec (though practically that's not a concern)
• struct timespec isn't uniformly a struct containing two same-sized ints:
  • https://github.com/golang/go/blob/master/src/syscall/ztypes_netbsd_arm.go#L23-L27
  • https://github.com/golang/go/blob/master/src/syscall/ztypes_netbsd_386.go#L23-L26
• syscall.Timespec is not a C struct. cgo -godefs is used to generate native Go struct: https://github.com/golang/go/blob/03ac39ce5e6af4c4bca58b54d5b160a154b7aa0e/src/syscall/ztypes_linux_amd64.go#L24-L27

So, yeah, i'm happy to go with your approach.

Btw. looking at the documentation for SO_RXQ_OVFL (the other value that is requested into/read from the oob), the value is defined as a 32-bit integer but it's currently read as a golang int. Is that intentional or a typo?

That's definitely my mistake. If you could fix that in this PR too, that would be great.

(I even double-checked the kernel source, because the docs for the option also say that it's:

the number of packets dropped by the socket between the last received packet and this received packet

which is incorrect. The counter is never reset)

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

---

go/lib/overlay/conn/conn.go, line 42 at r2 (raw file):

const ReceiveBufferSize = 1 << 20

var sizeOfInt64 = int(unsafe.Sizeof(int64(0)))

Proposal:
const sizeOfRxOverflow = 4 // Defined to be uint32

[matzf]

That's definitely my mistake. If you could fix that in this PR too, that would be great.

Done.

• struct timespec isn't uniformly a struct containing two same-sized ints
  I was surprised by that too, but then it actually makes a lot of sense (on 32-bit systems anyway), as the number of nanoseconds per second is somewhat fixed ;)

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @kormat)

---

go/lib/overlay/conn/conn.go, line 42 at r2 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Proposal:
const sizeOfRxOverflow = 4 // Defined to be uint32

Done.

[kormat]

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[kormat]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved