Skip to content

ames: add libnatpmp for automatic port forwarding#593

Merged
pkova merged 4 commits intodevelopfrom
pkova/natpmp
Apr 24, 2024
Merged

ames: add libnatpmp for automatic port forwarding#593
pkova merged 4 commits intodevelopfrom
pkova/natpmp

Conversation

@pkova
Copy link
Copy Markdown
Collaborator

@pkova pkova commented Jan 29, 2024

Same as urbit/urbit#3261 but for vere. From what I can tell this NAT-PMP stuff is fairly well supported by routers, works on my machine at least.

@pkova pkova requested a review from a team as a code owner January 29, 2024 13:22
@pkova pkova requested a review from joemfb January 29, 2024 13:22
Copy link
Copy Markdown
Collaborator

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this will do

Copy link
Copy Markdown
Collaborator

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

I've noted a couple things that could be cleaner with the libuv handle lifetime.

Also, the function signatures should be adjusted in the normal style.

Comment thread pkg/vere/io/ames.c Outdated

if ( c3n == sam_u->pir_u->fak_o ) {
sam_u->nat_u.tim_u.data = sam_u;
uv_timer_init(u3L, &sam_u->nat_u.tim_u);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would do this initialization unconditionally in u3_ames_io_init().

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then, you should close both of these new handles in _ames_io_exit().

Comment thread pkg/vere/io/ames.c Outdated
sendnewportmappingrequest(&sam_u->nat_u.req_u, NATPMP_PROTOCOL_UDP, por_s, por_s, 7200);

sam_u->nat_u.pol_u.data = sam_u;
uv_poll_init(u3L, &sam_u->nat_u.pol_u, sam_u->nat_u.req_u.s);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one too

Comment thread pkg/vere/io/ames.c Outdated

closenatpmp(&sam_u->nat_u.req_u);
sam_u->nat_u.tim_u.data = sam_u;
uv_timer_init(u3L, &sam_u->nat_u.tim_u);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This initialization is unnecessary

Comment thread pkg/vere/io/ames.c Outdated
closenatpmp(&sam_u->nat_u.req_u);
return;
}
uv_poll_stop(handle);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You could move this line before the error check and only have one call to stop()

Comment thread pkg/vere/io/ames.c Outdated
u3_ames* sam_u = handle->data;

natpmpresp_t response;
c3_w r = readnatpmpresponseorretry(&sam_u->nat_u.req_u, &response);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This assignment converts a signed int to an unsigned c3_w. You should c3_i (and suffix the variable name with _i).

Copy link
Copy Markdown
Collaborator

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this will do

@pkova pkova merged commit ae89983 into develop Apr 24, 2024
@pkova pkova deleted the pkova/natpmp branch April 24, 2024 16:15
pkova added a commit that referenced this pull request May 1, 2024
Reverts #593

This is very likely what is breaking `urbit/urbit` CI.
pkova added a commit that referenced this pull request May 10, 2024
Same as urbit/urbit#3261 but for vere. From what
I can tell this NAT-PMP stuff is fairly well supported by routers, works
on my machine at least.
pkova added a commit that referenced this pull request Jun 24, 2024
This is #593 that got reverted in #644 because it broke `urbit/urbit`
CI. This PR fixes the issue which was unconditionally calling
`uv_poll_init` on a socket that was initialized with `initnatpmp`
without checking if `initnatpmp` returned an error first.
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.

2 participants