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
netlink: release v1.0.0 #123
Comments
You've probably already thought of this, but any possibility of using an internal package that both
I can confirm I'm not using these functions with our internal clients. |
In my own code, I generally like to do this when I feel like I can get away with it.
The one slightly iffy case seems to be I will also note that No strong opinions on this matter either way.
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:
Assume this package is
The nice thing about the existing One final point: Look at In summary, I think
I think that's a good idea. The
Throughout the standard library, errors from system calls are decorated with additional details, e.g. Perhaps this is something worth thinking about. For example, when does This circles back to the 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 |
Good initiative, doesn't look like any major changes, maybe apart from removing
Definitely, IDEs (at least VSCode) will show autocomplete hints in the form of Agreed with @terinjokes' and @acln0's feedback, great input 👍 |
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. |
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 👍
This change will improve readability of code more than it could confuse - I think. So, I really like it 👍 |
@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? |
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 |
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. |
Sorry for mixing this up with the Attributes. For keeping |
The 1.13 error handling stuff might shake things up a bit, so let's hold off until at least that point. |
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:
Config.Groups
andConn.Join/LeaveGroup
to passgroup int
rather thangroup uint32
This is passed as a 32-bit C
int
to the kernel, so Go's 64-bitint
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).Message.Marshal/UnmarshalBinary
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
andHeaderFlags
prefixes from typed constantsI'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 remainsRequest 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?Socket
type andNewConn
constructorsThese 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
The text was updated successfully, but these errors were encountered: