search

mdlayher / netlink

Package netlink provides low-level access to Linux netlink sockets (AF_NETLINK). MIT Licensed.
MIT License
926 stars 97 forks source link

netlink: release v1.0.0 #123

Closed mdlayher closed 4 years ago

mdlayher commented 5 years ago

Once 1.12 hits and we're able to land runtime network poller support with proper timeouts, I think it's finally time to embrace semver and Go modules, and tag v1.0.0.

Before we do that, I'm debating if it's worth revising some previous interface decisions, such as:

This is passed as a 32-bit C int to the kernel, so Go's 64-bit int on 64-bit systems would need a sanity check. int is generally a good default and the most common integer type in Go by far; uint32 requires an explicit cast unless you're using an untyped constant (which is generally the case).

If you're importing this package, you're almost certainly using Conn which takes care of these internally. We could also do faster and more clever things if we didn't have to worry about allocating for individual messages, or making a copy of byte slices during unmarshaling.

DONE: consider removing HeaderType and HeaderFlags prefixes from typed constants

I'm admittedly a little iffy on this, but in recent projects I tend to prefer more succinct names like netlink.Request, even if the type remains Request HeaderFlags = 1. Is there a possibility of conflict in header types/flags with other current or future exported identifiers? Would this be too confusing in calling code?

These are really only meant for nltest; I had considered doing unsafe //go:linkname at one point but can't remember why I didn't go with it. It's not great, but it might be better than adding generally useless stuff to the exported 1.0.0 API.

DONE: - consider dropping Conn.ReadWriteCloser

This is only used in github.com/mdlayher/kobject as far as I know, and I think that its use there could be superseded by Conn.SyscallConn and methods there.

I think that about covers my thoughts. I know it's a lot, and I don't expect others to have strong opinions on all or any of these, but I would appreciate your feedback. In addition, if there are other things you think are worth revisiting for 1.0.0, please do let me know.

I'm going to tag some of the regular contributors and users of this package in hopes that folks can help me work things out, although I encourage feedback from all who come across this issue.

/cc @florianl @ti-mo @terinjokes @jsimonetti @stapelberg @acln0

terinjokes commented 5 years ago
  • finding another way to hide/unexport the Socket type and NewConn constructors

You've probably already thought of this, but any possibility of using an internal package that both netlink and nltest could import? I'd feel more comfortable with this change than using //go:linkname.

  • unexporting or removing Message.Marshal/UnmarshalBinary

I can confirm I'm not using these functions with our internal clients.

acln0 commented 5 years ago
  • consider removing HeaderType and HeaderFlags prefixes from typed constants

In my own code, I generally like to do this when I feel like I can get away with it.

netlink.Acknowledge is certainly a lot nicer than netlink.HeaderFlagsAcknowledge. I don't think it would be very confusing for callers, because they're probably not using the netlink package without some level of domain knowledge about netlink itself, in which case it wouldn't be hard to figure out what the names Acknowledge or Request refer to.

The one slightly iffy case seems to be netlink.HeaderTypeError, which becomes netlink.Error under the proposed change. Perhaps this identifier is valuable in another context. I discuss this more in the final paragraph.

I will also note that ConnOption constants don't use the prefixes, while HeaderFlags and HeaderType constants do. Perhaps a consistent choice should be made throughout.

No strong opinions on this matter either way.

  • finding another way to hide/unexport the Socket type and NewConn constructors

This is a tough one. I've thought about it quite a bit, but I don't think it's really possible. Linker trickery doesn't feel like it belongs in such a place, and other alternatives don't seem to work. For example:

You've probably already thought of this, but any possibility of using an internal package that both netlink and nltest could import? I'd feel more comfortable with this change than using //go:linkname.

Assume this package is github.com/mdlayher/netlink/internal/nltransport, and it declares nltransport.Socket, which is the current netlink.Socket. The problem is that netlink.Socket references netlink.Message, so the hypothetical nltransport must either:

The nice thing about the existing Socket interface is that it's high level, pretty small, and easy for nltest to implement. To get rid of it, it feels like you have to either do very ugly things like linker tricks, or make netlink swallow nltest completely. I don't think either option is worth the cost.

One final point: Look at crypto/tls and net/smtp. Both have a higher level func Dial and a lower level func Client, func Server, func NewClient etc. In the lower level functions, the argument is net.Conn, which represents a network transport. By analogy, a netlink.Socket is a transport for a netlink.Conn: it can send and receive messages, which Conn builds on top of. I think that's totally fine, even if the only two implementations ever used are netlink.conn and nltest.socket.

In summary, I think Socket and NewConn should stay, because the alternatives are not nice at all.

  • consider dropping Conn.ReadWriteCloser

I think that's a good idea. The SyscallConn API is more flexible, and does everything the ReadWriteCloser API can do, and more.

In addition, if there are other things you think are worth revisiting for 1.0.0, please do let me know.

Throughout the standard library, errors from system calls are decorated with additional details, e.g. os.SyscallError, net.OpError. Package netlink doesn't do any of this at the moment, as far as I can tell. It seems to return syscall.Errno values up to the callers.

Perhaps this is something worth thinking about. For example, when does sendmsg(2) fail on a netlink socket? If and when it does, is the syscall.Errno value enough for the caller to be able to diagnose the cause of the error?

This circles back to the netlink.Error naming issue I mentioned above. Is there any point in having a type that captures operational errors and decorates them with additional context? If there is, what would the type be named? Would it be netlink.Error? Or perhaps netlink.OpError?

Personally, all the errors I've gotten while using netlink have been in the form of netlink error messages, rather than system call errors from operations such as calling conn.Send, so I haven't had this problem. That being said, I have not used netlink in very advanced ways, so perhaps I have missed something. But having noticed this detail about error handling while I was working on the code, I thought I would at least bring it up for discussion.

ti-mo commented 5 years ago

Good initiative, doesn't look like any major changes, maybe apart from removing Conn.RWC. I don't use it in any of my packages, though wouldn't this play nice with the new 1.12 runtime poller? Admittedly I haven't followed up on the changes here since before Conn.SyscallConn was introduced.

  • consider removing HeaderType and HeaderFlags prefixes from typed constants

Definitely, IDEs (at least VSCode) will show autocomplete hints in the form of HeaderFlagsRequest HeaderFlags, which stutters. Request HeaderFlags would be easier to read there. Could this conflict with other identifiers in the future? I'd argue this is unlikely, since the term 'request' is already used for this header flag in Netlink lingo (we're just copying kernel terminology), and the protocol is inherently message-oriented without any request/response abstraction.

Agreed with @terinjokes' and @acln0's feedback, great input :+1:

mdlayher commented 5 years ago

Thanks for the comments so far! I'm going to be traveling for a week or so, but I will revisit this in a week's time and review everything again.

florianl commented 5 years ago
  • unexporting or removing Message.Marshal/UnmarshalBinary
  • changing signature of Config.Groups and Conn.Join/LeaveGroup to pass group int rather than group uint32

Some of my libraries are using these functions and its on my todo list, to replace them. But these libraries also use go modules. So improving here your API is just fine :+1:

  • consider removing HeaderType and HeaderFlags prefixes from typed constants

This change will improve readability of code more than it could confuse - I think. So, I really like it :+1:

mdlayher commented 5 years ago

Some of my libraries are using these functions and its on my todo list, to replace them. But these libraries also use go modules. So improving here your API is just fine

@florianl if you have a valid use case for the Message marshaling functions, I'm happy to not remove them; I just assumed it was unnecessary to have them exported. Can you link me to your code?

florianl commented 5 years ago

One example in my code is https://github.com/florianl/go-nfqueue/blob/master/nfqueue.go#L83 - here I set the verdict for nfqueue. But this can be rewritten with AttributeDecoder.

mdlayher commented 5 years ago

Ah! So I don't think MarshalAttributes is going anywhere because it can be convenient sometimes; I was talking about the marshaling methods on the Message type.

florianl commented 5 years ago

Sorry for mixing this up with the Attributes. For keeping Message.Marshal/UnmarshalBinary exported, I don't see a use case.

mdlayher commented 5 years ago

The 1.13 error handling stuff might shake things up a bit, so let's hold off until at least that point.