soypat / seqs

seqs: the hottest, most idiomatic userspace TCP/IP implementation on the internet. lwip in go basically
BSD 3-Clause "New" or "Revised" License
45 stars 5 forks source link

More descriptive name for HandleEth() #30

Open nickaxgit opened 4 months ago

nickaxgit commented 4 months ago

Brainstorm:-

func (ps *PortStack) handleEth(dst []byte) (n int, err error) { //might be better named as getNextOutboundPacket or fillNextOutboundPacket

soypat commented 3 months ago

Note, the Put term comes from Go's encoding/binary package terminology which refers to writing data of length L into a buffer that needs to be at least of length L. See More ideas

nickaxgit commented 3 months ago

I like PutNextOutboundPacket - that would be a huge improvement

On Fri, 19 Jul 2024 at 09:49, Patricio Whittingslow < @.***> wrote:

Note, the Put term comes from Go's encoding/binary package terminology which refers to writing data of length L into a buffer that needs to be at least of length L. See More ideas

  • PutNextOutboundPacket
  • PutOutbound
  • PutNextPacket

— Reply to this email directly, view it on GitHub https://github.com/soypat/seqs/issues/30#issuecomment-2238687066, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALR2VA3P5IPYMR557FSJW2LZNDHLJAVCNFSM6AAAAABK43BHCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZYGY4DOMBWGY . You are receiving this because you authored the thread.Message ID: @.***>

soypat commented 3 months ago

So it occured to me that to better match with RecvEth maybe we could do PutOutboundEth, what do you think? Packet is pretty generic and Eth makes it clear it refers to link layer packets.

nickaxgit commented 3 months ago

I think the 'next' in the word is descriptive - it helps me imagine how it works .. and the 'packet' again .. descriptive and useful - the Eth.. sure if it's specifically Ethernet - add it

which would take us to PutNextOutboundEthPacket .. which might appear comically long, but honestly I don't think that's a bad thing (for something so fundamental)

HandleEth appears a lot in comments too .. so 'rename symbol' isn't going to get all of them

On Sat, 20 Jul 2024 at 13:20, Patricio Whittingslow < @.***> wrote:

So it occured to me that to better match with RecvEth maybe we could do PutOutboundEth, what do you think? Packet is pretty generic and Eth makes it clear it refers to link layer packets.

— Reply to this email directly, view it on GitHub https://github.com/soypat/seqs/issues/30#issuecomment-2241115398, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALR2VA6SXDGXWJQTDC3WN5TZNJIYFAVCNFSM6AAAAABK43BHCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGEYTKMZZHA . You are receiving this because you authored the thread.Message ID: @.***>

soypat commented 3 months ago

I agree the naming should be something between PutOutboundEth and PutNextOutboundEthPacket. I tend to err on the side of shortness since this will form part of a "famous" and fundamental interface: netif.InterfaceEthPoller.

This interface should be the first thing developers and users study when beginning working with any Go link-layer API. And I tend to disagree on the point of fundamental things needing be more explicit. In any case it being so fundamental should be more reason for the name to be short and memorable since it will be something outward facing and well known. We don't do Vector.AddVector(v Vector), in Go we prefer Vector.Add(v Vector). This is what gonum does, and that's a project I have huge respect for in terms of API design

I also feel giving it a long name would not be consistent with the naming we have given RecvEth. Even though RecvEth has a short name and omits the Next and Packet words in the name (even though they'd serve to describe it better) the purpose is quite clear- the Stack receives an Ethernet (packet! what else would it receive!). There is no question about whether it is the next or previous packet :)

nickaxgit commented 3 months ago

It is your baby Patricio

And I don’t have the context or experience you do

My fundamental point really was always that it should be clear that it is sending, and ideally, what it is sending

Happy to go with whatever you decide

On Sat, 20 Jul 2024 at 15:49, Patricio Whittingslow < @.***> wrote:

I agree the naming should be something between PutOutboundEth and PutNextOutboundEthPacket. I tend to err on the side of shortness since this will form part of a "famous" and fundamental interface: netif.InterfaceEthPoller https://github.com/soypat/netif/blob/e7dfef0020a669328a65683e3e6115b4b7426af0/netif.go#L34 .

This interface should be the first thing developers and users study when beginning working with any Go link-layer API. And I tend to disagree on the point of fundamental things needing be more explicit. In any case it being so fundamental should be more reason for the name to be short and memorable since it will be something outward facing and well known. We don't do Vector.AddVector(v Vector), in Go we prefer Vector.Add(v Vector). This is what gonum https://github.com/gonum/gonum/blob/0c62273e338b91cd9578ed93572c693ba55e1eaa/spatial/r3/vector.go#L15 does, and that's a project I have huge respect for in terms of API design

I also feel giving it a long name would not be consistent with the naming we have given RecvEth. Even though RecvEth has a short name and omits the Next and Packet words in the name (even though they'd serve to describe it better) the purpose is quite clear- the Stack receives an Ethernet (packet! what else would it receive!). There is no question about whether it is the next or previous packet :)

— Reply to this email directly, view it on GitHub https://github.com/soypat/seqs/issues/30#issuecomment-2241173519, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALR2VAZ47IS7DD7WV7XUFKTZNJ2IVAVCNFSM6AAAAABK43BHCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGE3TGNJRHE . You are receiving this because you authored the thread.Message ID: @.***>

soypat commented 3 months ago

Hey nick, I applied the changes in this commit: https://github.com/soypat/seqs/pull/31/commits/74e7a6bc7a705fc113a59f65165f64c47b2c9a44

Also centralized the documentation at the handlers to avoid duplicating comments. Should be easily navigable in VSCode since the [iudphandler] bracket tagging allows for Ctrl+clicking in VSCode to easily go to the interface definition with the documentation.