Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: *Conn.ReadMsgXXX documentation could be improved #30346

Open
dciliske opened this Issue Feb 22, 2019 · 5 comments

Comments

Projects
None yet
6 participants
@dciliske
Copy link

dciliske commented Feb 22, 2019

Unfortunately, this is documentation/API request more than a 'bug'. I started this needing to know what interface a UDP message was received on, and immediately ran into an interface limitation.

Summary: ReadMsgXXX is impossible to use without a priori knowledge of the underlying network API that Go uses. Furthermore, the documentation for said methods refers the user to another package, without explaining what to look for in said package.*

Either the documentation for UdpConn.ReadMsgUDP and/or the function's mere existence in it's current form are at best useless (I understand how frustrating this statement is, but unfortunately this is the case). Additionally, while this message is primarily regarding UdpConn.ReadMsgUDP, it is likely equally applicable to all the net/x ReadMsgXXX (and possibly the WriteMsgXXX) functions as well.

The fundamental problem, which I'm certain you are aware of, is that every different OS stack behaves differently and reports different pieces of information in different ways. However, as written, the documentation does not explain anything about what is the user needs to access the out-of-band data. It only says:

The packages golang.org/x/net/ipv4 and golang.org/x/net/ipv6 can be used to manipulate IP-level socket options in oob.

But not anything about how to do that.

The user has to already understand the underlying network stack enough to connect the 'out-of-band' comment in the documentation for UdpConn.ReadMsgUDP to 'control message' in the overview examples of x/net/ipv4:

The application might set per packet control message transmissions between the protocol stack within the kernel. When the application needs a destination address on an incoming packet, SetControlMessage of PacketConn is used to enable control message transmissions.

Finally, after determining that these are related the user is still required to manually call Parse on the out-of-band data array to finally convert it to the ControlMessage that they require.

(As an aside, I'm fairly certain @bradfitz is correct here: #28900 (comment))

*I believe this redirection is referring to SetControlMessage in x/net/ipvX. However, there is no indication that this will do anything to configure the 'out-of-band' data.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Feb 22, 2019

/cc @mikioh

@mikioh mikioh changed the title net/udp: ReadMsgXXX documentation is useless net: ReadMsgXXX documentation is useless Feb 22, 2019

@mikioh

This comment has been minimized.

Copy link
Contributor

mikioh commented Feb 22, 2019

in it's current form are at best useless

I'm relieved to see the word "useless" instead of "considered harmful" because the package net is a sort of intersection between various operating system kernels, various network- and transport-layer protocols. The API surfaces and docs are able to propagate wrong information easily, to mislead API users into doing wrong things. I agree that the documentation update is necessary; see also #29978.

Requests for more better API surfaces should be addressed in #28900. The current API surfaces have a history, for example, @rsc vs. @mikioh :), wandering ipv4 and ipv6 packages and settlement of the x/net repository. #28900 is a new hope.

@odeke-em odeke-em changed the title net: ReadMsgXXX documentation is useless net: *Conn.ReadMsgXXX documentation could be improved Feb 22, 2019

@antong

This comment has been minimized.

Copy link
Contributor

antong commented Feb 22, 2019

I think the documentation for ReadMsgUDP() is OK, and definitely not useless. I see no solution to that use of low-level, platform dependent networking features requires knowing about the network stack (how do you get the incoming interface for a UDP datagram on Windows?!). But x/net/ipv4 could definitely have improved documentation and a complete example on enabling, receiving and parsing the oob data.

For func (c *PacketConn) SetControlMessage(cf ControlFlags, on bool) error, the on flag is nowhere documented. It seems that true means to set the flags and false means to clear the provided flags. And, it is not supported on Windows.

@dciliske

This comment has been minimized.

Copy link
Author

dciliske commented Feb 22, 2019

@antong As both a library developer and library user, there is a reason why I hold that the documentation is not ok and is in fact useless. If we look at the actual purpose of a library, we find that it exists to provide an abstraction of commonly repeated behaviors and simplify the job of the developer using the library. Examining the net package, the majority of the API revolves around interacting with network protocols and paradigms; not the underlying OS level API.

The net library exists for the explicit purpose of allowing a developer to work with network communications without understanding the intimate details of the network API of every platform. I understand now (after digging through the entirety of the net and x/net/ipv4 source) that the ReadMsgXXX methods are nothing more than a mapping layer on top of 'recvmsg', but there is no way to make that leap with only the documentation.

If ReadMsgXXX is a method that cannot be used without understanding the underlying network stack, fine! Tell me that in the documentation! Tell me what I will need to know to understand what to do! There are references to various RFCs scattered throughout the docs(!), but when we get to a function which is essentially a direct mapping onto the OS syscall, there's nothing; no mention of that, no direct pointer of what to look for in other documentation, no "Here be dragons", just some vague mention of "these other packages can be used to manipulate what is returned in oob".

@dciliske dciliske closed this Feb 22, 2019

@dciliske

This comment has been minimized.

Copy link
Author

dciliske commented Feb 22, 2019

@mikioh The API split between net and x/net makes a fair bit of sense, but I'll admit to not understanding the reasoning for placing SetControlMessage in x/net and not duplicated in net as well (a la ReadMsgXXX); I'm guessing there was a bit of a contentious debate on the subject.

@dciliske dciliske reopened this Feb 22, 2019

@bcmills bcmills added the NeedsFix label Feb 28, 2019

@bcmills bcmills added this to the Unplanned milestone Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.