Skip to content

Add partial support for ipset#387

Merged
vishvananda merged 2 commits intovishvananda:masterfrom
digineo:ipset
Sep 25, 2020
Merged

Add partial support for ipset#387
vishvananda merged 2 commits intovishvananda:masterfrom
digineo:ipset

Conversation

@corny
Copy link
Copy Markdown
Contributor

@corny corny commented Sep 27, 2018

Looking forward to read your comments and improve my code.

jellonek
jellonek suggested changes Sep 28, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Great contribution!
Only minor documentation nits.

ipset_linux.go
"golang.org/x/sys/unix"
)

type IPSetEntry struct {
Copy link
Copy Markdown
Contributor

@jellonek jellonek Sep 28, 2018

Choose a reason for hiding this comment

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

Exported name without docstring.
Please add docstrings to all names which are started with upper case, using convention described in https://blog.golang.org/godoc-documenting-go-code

Copy link
Copy Markdown
Contributor Author

@corny corny Oct 31, 2018

Choose a reason for hiding this comment

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

done

aboch
aboch reviewed Sep 28, 2018
View reviewed changes
@corny corny force-pushed the ipset branch 4 times, most recently from f56c1f0 to bb40bd5 Compare September 30, 2018 23:35
@vishvananda
Copy link
Copy Markdown
Owner

vishvananda commented Oct 18, 2018

This is looking good. Can you please squash it down into a single commit?

@corny corny force-pushed the ipset branch 3 times, most recently from f8761a6 to 0df1233 Compare October 31, 2018 12:33
@aboch
Copy link
Copy Markdown
Collaborator

aboch commented Nov 8, 2018

You may want to remove these two binaries from the diffset

image

@corny
Copy link
Copy Markdown
Contributor Author

corny commented Dec 8, 2018

You may want to remove these two binaries from the diffset

These are netlink responses. They are required by ipset_linux_test.go.

@corny corny force-pushed the ipset branch 2 times, most recently from bbde686 to f0450b3 Compare December 9, 2018 03:36
@aboch aboch mentioned this pull request Dec 11, 2018
Closed
@onokonem
Copy link
Copy Markdown

onokonem commented Dec 11, 2018

hi @corny

I have a problem with this patch

$uname -a
Darwin AF6438.local 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64

$go version
go version go1.11.2 darwin/amd64

$GOOS=linux go build ./netmanager/cmd/netmanager
# github.com/AnchorFreePartner/services/vendor/github.com/vishvananda/netlink
../../vendor/github.com/vishvananda/netlink/ipset_linux.go:211:34: undefined: unix.NFNL_SUBSYS_IPSET

any idea why?

@corny corny force-pushed the ipset branch 2 times, most recently from 7b3d101 to e15c8f8 Compare April 28, 2020 17:18
@corny
Copy link
Copy Markdown
Contributor Author

corny commented Apr 28, 2020

@onokonem You don't have the proper header files on your system.

@corny
Copy link
Copy Markdown
Contributor Author

corny commented Sep 14, 2020

@vishvananda ping

@vishvananda
Copy link
Copy Markdown
Owner

vishvananda commented Sep 14, 2020

@corny Thanks for updating this. Our policy is for prs to be a single commit. Can you squash this down? You can add a co-authored-by line for the original author so we don't lose their contributions.

@corny
Copy link
Copy Markdown
Contributor Author

corny commented Sep 17, 2020

I squashed it

@vishvananda vishvananda merged commit 1e3d26b into vishvananda:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aboch aboch aboch left review comments

+1 more reviewer

@jellonek jellonek jellonek requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@corny @vishvananda @aboch @onokonem @jellonek