Skip to content

Conversation

@soffokl
Copy link
Member

@soffokl soffokl commented Aug 11, 2021

// FlagTraversalOrder order of NAT traversal methods to be used for providing service.
FlagTraversal = cli.StringFlag{
Name: "traversal",
Usage: "Comma separated order of NAT traversal methods to be used for providing service",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to provide possible values here?

Copy link
Member Author

Choose a reason for hiding this comment

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

we are including all values as default. Here is how it looks in help:

   --traversal value                                      Comma separated order of NAT traversal methods to be used for providing service (default: "holepunching,upnp,manual")

p2p/listener.go Outdated
// acquiring extra ports for nat pinger if provider is behind nat, port mapping failed
// and no manual port forwarding is enabled.
func (m *listener) prepareLocalPorts(id, outboundIP string, tracer *trace.Tracer) (string, []int, []func(), error) {
func (m *listener) prepareLocalPorts(id string, tracer *trace.Tracer) (string, []int, []func(), nat.StartPorts, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point the returns values here should be remade. Adding one more return here is just more confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be wrapped into struct with meaningful names attached

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to change it with the next couple of PRs. So merging like this now.

}

return &natHolePunchingPort{
pool: port.NewFixedRangePool(*udpPortRange),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wont this panic when ParseRange returns error and nil for udpPortRange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, faced it with tests too, changed.

@codecov-commenter
Copy link

Codecov Report

Merging #3758 (4246469) into master (0a14b19) will decrease coverage by 0.13%.
The diff coverage is 43.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3758      +/-   ##
==========================================
- Coverage   43.61%   43.47%   -0.14%     
==========================================
  Files         325      325              
  Lines       16689    16669      -20     
==========================================
- Hits         7279     7247      -32     
- Misses       8655     8674      +19     
+ Partials      755      748       -7     
Impacted Files Coverage Δ
config/flags_network.go 0.00% <0.00%> (ø)
core/port/range.go 11.76% <0.00%> (+1.23%) ⬆️
p2p/common.go 66.10% <ø> (ø)
p2p/channel.go 72.72% <33.33%> (-1.82%) ⬇️
p2p/dialer.go 56.06% <50.00%> (-4.75%) ⬇️
p2p/listener.go 58.58% <55.17%> (-5.51%) ⬇️
requests/dialer_swarm.go 73.28% <0.00%> (-1.53%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a14b19...4246469. Read the comment docs.

@soffokl soffokl merged commit 9ba4f14 into master Aug 12, 2021
@soffokl soffokl deleted the traversal-order branch August 12, 2021 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants