Skip to content

p2p/nat: improve AddMapping code #31486

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

Merged
merged 3 commits into from
Apr 1, 2025
Merged

Conversation

gazzua
Copy link
Contributor

@gazzua gazzua commented Mar 25, 2025

It introduces a new variable to store the external port returned by the addAnyPortMapping function and ensures that the correct external port is returned even in case of an error.

@gazzua gazzua requested review from fjl and zsfelfoldi as code owners March 25, 2025 10:44
@gazzua gazzua changed the title p2p/nat: fix AddMapping to handle errors and return new external port p2p/nat: fix to ensure the newly assigned port from AddAnyPortMapping is returned Mar 25, 2025
@gazzua gazzua force-pushed the fix-port-mapping branch from 394a5ab to 7befd0e Compare March 25, 2025 10:48
@cskiraly
Copy link
Collaborator

cskiraly commented Apr 1, 2025

I don't think there is such a thing as a "correct external port" when there was an error. Callers are supposed to check the err, and disregard the port if not nil.

@fjl fjl changed the title p2p/nat: fix to ensure the newly assigned port from AddAnyPortMapping is returned p2p/nat: improve AddMapping code Apr 1, 2025
@fjl fjl merged commit 1bd70ba into ethereum:master Apr 1, 2025
3 of 4 checks passed
thinkAfCod pushed a commit to thinkAfCod/go-ethereum that referenced this pull request Apr 2, 2025
It introduces a new variable to store the external port returned by the
addAnyPortMapping function and ensures that the correct external port is
returned even in case of an error.

---------

Co-authored-by: Felix Lange <fjl@twurst.com>
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