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

Support binding specified IP #51

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Support binding specified IP #51

merged 1 commit into from
Jan 16, 2024

Conversation

xlch88
Copy link
Contributor

@xlch88 xlch88 commented Jan 3, 2024

No description provided.

main.go Outdated
@@ -24,7 +24,8 @@ import (

func main() {
var serverAddr = flag.String("s", stun.DefaultServerAddr, "STUN server address")
var localPort = flag.Int("p", 0, "The port on which to bind requests, set to 0 to pick a random port")
var localPort = flag.Int("p", 0, "The ip on which to bind requests, set to empty will keep default")
var localIP = flag.String("i", "", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have one addreess that takes an address with a port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in some cases you only need to specify the port, not the IP?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could still do "192.168.0.10:0" or ":5738" or both etc.

stun/client.go Outdated
if c.localPort != 0 {
laddr, err = net.ResolveUDPAddr("udp", fmt.Sprintf(":%d", c.localPort))
address += strconv.Itoa(c.localPort)
Copy link
Owner

Choose a reason for hiding this comment

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

i think we need a : between ip and port

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, I forgot about this

stun/client.go Outdated
address += strconv.Itoa(c.localPort)
}

if c.localPort != 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

why do we not put the two if c.localPort != 0 { together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken care of it and look better now

@xlch88 xlch88 force-pushed the master branch 2 times, most recently from f107d31 to 4095b02 Compare January 4, 2024 11:18
stun/client.go Outdated
laddr, err = net.ResolveUDPAddr("udp", fmt.Sprintf(":%d", c.localPort))

if c.localPort != 0 || c.localIP != "" {
var address = c.localIP + ":" + strconv.Itoa(c.localPort)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
var address = c.localIP + ":" + strconv.Itoa(c.localPort)
var address = fmt.Sprintf("%s:%d", c.localIP, c.localPort)

stun/client.go Outdated
if c.localPort != 0 {
laddr, err = net.ResolveUDPAddr("udp", fmt.Sprintf(":%d", c.localPort))
if c.localPort != 0 || c.localIP != "" {
var address = c.localIP + ":" + strconv.Itoa(c.localPort)
Copy link
Owner

Choose a reason for hiding this comment

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

same here

@ccding
Copy link
Owner

ccding commented Jan 4, 2024

lgtm after fixing the above minor issues

@AudriusButkevicius
Copy link
Contributor

What about ipv6 addresses?

@ccding
Copy link
Owner

ccding commented Jan 4, 2024

ipv6 doesn't have NAT

@xlch88
Copy link
Contributor Author

xlch88 commented Jan 16, 2024

merge ?

@ccding ccding merged commit d32c135 into ccding:master Jan 16, 2024
1 check passed
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.

3 participants