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

Set outbound interface, like ping -I option #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ilolicon
Copy link

Hello,

I attempted to implement a useful feature - sending data packets from a specific network interface, similar to what the "ping" command's "-I" option does in Linux.
Hoping I did it correctly. Test successfully passed on Linux OS.

Thanks!

The issues related to this feature:

go-ping/ping#111

go-ping/ping#204

@ilolicon ilolicon changed the title Set outbound interface name, like ping -I option Set outbound interface, like ping -I option Apr 12, 2023
@SuperQ
Copy link
Contributor

SuperQ commented Apr 12, 2023

This needs a DCO sign-off. You can use git commit -s --amend to add it.

ping.go Outdated
Comment on lines 185 to 186
// Iface used to send/recv ICMP messages
Iface string
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the short name

Suggested change
// Iface used to send/recv ICMP messages
Iface string
// Interface used to send/recv ICMP messages
Interface string

Copy link
Author

Choose a reason for hiding this comment

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

The original intention was just to avoid confusion with the go interface keyword

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe InterfaceName then if needed?

Copy link
Author

@ilolicon ilolicon Apr 12, 2023

Choose a reason for hiding this comment

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

I adopted Interface to make a new commit but the ci test still fails
I use the same ci docker image locally and it works fine. Am I doing something wrong?
I hope you can give me some help, thank you very much

circleci@3b386f984da0:~/project/pro-bing$ git branch -vv
* feature-interface-binding 60b7449 [origin/feature-interface-binding] Refactoring the variable name `Iface` to `Interface` and `ifaceIndex` to `ifIndex`(keeping it consistent with `ControlMessage`)
  main                      a17ba03 [origin/main] Merge pull request #29 from TheRushingWookie/add_context
circleci@3b386f984da0:~/project/pro-bing$ make style vet test
>> checking code style
>> vetting code
GO111MODULE= go vet  ./...
>> running all tests
GO111MODULE= go test -race -cover  ./...
ok  	github.com/prometheus-community/pro-bing	(cached)	coverage: 76.9% of statements
?   	github.com/prometheus-community/pro-bing/cmd/ping	[no test files]

ci error message: socket: permission denied

Signed-off-by: ilolicon <97431110@qq.com>
… to `ifIndex`(keeping it consistent with `ControlMessage`)

Signed-off-by: ilolicon <97431110@qq.com>
@ilolicon ilolicon requested a review from Hipska April 15, 2023 07:55
@@ -29,6 +29,9 @@ Examples:
# ping google for 10 seconds
ping -t 10s www.google.com

# ping google specified interface
ping -I eth1 www.goole.com
Copy link

Choose a reason for hiding this comment

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

there is a typo here: goo[g]le.com

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ping -I eth1 www.goole.com
ping -I eth1 www.google.com

@bluecmd
Copy link

bluecmd commented Nov 10, 2023

FWIW, I would like to see this feature added in order to use VRF with this library.

@HsimWong
Copy link

I need this feature really bad. Can anyone merge this?

@bluecmd
Copy link

bluecmd commented Jul 19, 2024

The PR needs to be rebased if it is to have any chance of getting merged.

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.

6 participants