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

Improve behaviour of SetReadBuffer / SetWriteBuffer #165

Merged
merged 4 commits into from
May 11, 2020

Conversation

MarkusBauer
Copy link
Contributor

(see issue #164 for a detailed explanation)

This PR allows users to use their privileges regarding buffer sizes: root and users with CAP_NET_ADMIN can ignore the system's limit on buffer sizes. Unprivileged users are not affected, there's a fallback. The testcase has been adjusted.

While I was working on this PR I noticed that the library has no method to read back the actual buffer size of the socket. I added these methods (GetReadBuffer and GetWriteBuffer) which are similar to their Write pendants.

@mdlayher
Copy link
Owner

mdlayher commented May 7, 2020

Hey, my apologies for the delay here, I totally lost track of this. I will review now.

bytes,
))
if err != nil {
// If SO_SNDBUFFORCE fails, try SO_RCVBUF
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific syscall error we could inspect to determine if fallback is needed? Would something like os.IsPermission be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The man page does not require a specific error for that case, but the kernel uses EPERM (which is the only logical one of course). Also the permission check is the first thing to happen. I don't think it will ever make a difference if we check for permission errors or not, libnfnetlink does not check for it. Your choice.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with leaving it be then, thanks for the info.

conn_linux.go Outdated Show resolved Hide resolved
conn_linux.go Outdated Show resolved Hide resolved
conn_linux.go Show resolved Hide resolved
conn_linux.go Outdated Show resolved Hide resolved
conn_linux.go Show resolved Hide resolved
@MarkusBauer
Copy link
Contributor Author

Thank you for your review. I adressed your remarks.

Copy link
Owner

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

bytes,
))
if err != nil {
// If SO_SNDBUFFORCE fails, try SO_RCVBUF
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with leaving it be then, thanks for the info.

@mdlayher mdlayher merged commit 5a2be09 into mdlayher:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants