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

Two functions: one for adding bond slave, one for getting veth peer index #307

Merged
merged 1 commit into from
Dec 14, 2017

Conversation

jjastrze-ovh
Copy link

As in topic. Please review this.

ioctl_linux.go Outdated
// IfreqSlave is a struct for ioctl bond manipulation syscalls.
// It is used to assign slave to bond interface with Name.
type IfreqSlave struct {
Name [syscall.IFNAMSIZ]byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you switch to the unix pkg instead of syscall ?

link_linux.go Outdated
func VethPeerIndex(link *Veth) (int, error) {
fd, err := getSocketUDP()
if err != nil {
return 0, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not returning -1 as in all other error cases below

@aboch
Copy link
Collaborator

aboch commented Dec 1, 2017

Thanks for the contributions.

Couple of minor comments. Also, ideally this should be two commits: One for VethPeerIndex and its testing code, and one commit for LinkSetBondSlave and its testing code.

@jjastrze-ovh
Copy link
Author

Thanks @aboch for review. I have fixed the two files you requested :)

@aboch
Copy link
Collaborator

aboch commented Dec 6, 2017

Please do not add a new commit to take care of the review comments.
Please use git rebase -i and fix the target commit or just squash them into one before pushing the PR.

@jjastrze-ovh
Copy link
Author

@aboch I have put everything into one commit.

@aboch
Copy link
Collaborator

aboch commented Dec 14, 2017

Thanks @jjastrze-ovh / @phob0s-pl

LGTM

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