search

pin / tftp

TFTP server and client library for Golang
MIT License
288 stars 74 forks source link

Add LocalAddr() to Incoming and Outgoing transfer interfaces. #32

Closed VictorLowther closed 7 years ago

VictorLowther commented 7 years ago

This facilitates more detailed logging along with facilitating a usecase I have which involves dynamically generating TFTP content based on both the address of the system making the request and the address that the host system is using to handle the request. This is useful in multi-homed environnments where we cannot assume that all of the systems we act as a TFTP server for can see us at a single IP address.

VictorLowther commented 7 years ago

:/ Fails on darwin, and I have no idea why.

pin commented 7 years ago

I've tested that locally and it works as expected in my linux environment! Cool! I think it can go in, we just need to figure out some details:

Please give me a bit more time, I will respond with more details and proper review, hopefully tomorrow.

P.S. Do not worry about OS X test failure, I have a host to debug that on, will let you know.

VictorLowther commented 7 years ago

1.1 and 1.2: Those are reasonable. While it would be possible to implement the same-ish logic for the client (use the same interfaces, but grab the Src address instead of the Dst one in the ipvx code), I don't have a need for it and it is probably not as useful. I will point out for 1.1 that on the client side, we just fail to fill out localAddr in the current codepath, so if we leave the OutgoingTransfer and IncomingTransfer interfaces as they are in the current PR, they will give valid (if annoying) answers until they get wired up by someone that finds a usecase for the client discovering the local IP they are sending packets from.

  1. I will go ahead an turn those panic() back into Error returns -- I had them as panics as more of a debugging aide than anything else.

3.and 4. x/net/ipvx has control message implementations for all the POSIX'ish oses (linux, darwin, *bsd, etc), and stubbed out ones for Windows. I think that checking for nil control packets and errors handling the ControlMessage should probably suffice -- I am using this as a low-level primitive to build a cached mapping of remote IPs -> local IPs, and as long as I get either nil or something accurate here I can cope.

As for vendoring, I use glide to vendor this package, and have it redirect to my copy in the glide.yaml -- it is pretty easy for me to maintain a working copy in my repo while more detailed code review etc. is carried out here.

VictorLowther commented 7 years ago

OK, I will make a new interface to work with the LocalIP() changes.

If you want to see where I am using the new functionality, it is at https://github.com/rackn/rocket-skates/pull/143/files#diff-b59f5ad842e34814840702217980e20fR26

pin commented 7 years ago

Thank you for addressing API change requests! LGTM!