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
Add network namespace ID management. #370
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit and also small question and rest lgtm.
netns_unspecified.go
Outdated
package netlink | ||
|
||
func GetNetNsIdByPid(pid int) (int, error) { | ||
return -1, ErrNotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's more common to return a zero value instead of -1 in case of error.
Setting there -1 would be more like double error indication with false information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The challenge is that 0 is a valid namespace ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zero value in many other places can be a valid value and there is no rule to pass invalid value in error situation.
But, there is also any rule forcing to use zero value in a combination with error, so I'm not insisting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @jellonek comment is more about current diff overloads the int
return param having it signal an error. Which is unnecessary and confusing for the reader of the code.
Given the function (correctly in go world) returns an error param, caller must honor it and not even look or use the other return params in case err != nil. The double error marking is unnecessary and confusing.
netns_unspecified.go
Outdated
} | ||
|
||
func GetNetNsIdByFd(fd int) (int, error) { | ||
return -1, ErrNotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
netns_linux.go
Outdated
return 0, fmt.Errorf("unexpected empty result") | ||
} | ||
|
||
// getNetNsId sets the netnsid for a given type-val pair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setNetNsId
OK, updated. I added some tests and changed the error value. |
LGTM. Failure in tests build is unrelated to this PR. |
Adds the ability to set and retrieve network namespace IDs. This is useful, for example, for determining the "other side" of a veth pair.
Adds the ability to set and retrieve network namespace IDs. This is useful, for example, for determining the "other side" of a veth pair.