ames: libnatpmp for real this time #646
Conversation
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.
There was a problem hiding this comment.
This PR never disposes of this new uv_poll_t and uv_timer_t handles. It should uv_close() them in _ames_io_exit(). The fact that we can't unconditionally init the uv_poll_t means we need some state to tell us if we should close it. It might be best to just heap allocate it so we can check if the pointer is null.
pkg/vere/io/ames.c
Outdated
| return; | ||
| } | ||
|
|
||
| uv_poll_init(u3L, &sam_u->nat_u.pol_u, sam_u->nat_u.req_u.s); |
There was a problem hiding this comment.
It looks like uv_poll_init() is categorically different from other libuv init functions, and we need to check for errors here.
| natpmp_cb(uv_poll_t* handle, | ||
| c3_i status, | ||
| c3_i events) | ||
| { |
There was a problem hiding this comment.
We're not checking status for errors here. And I'm not sure if readnatpmpresponseorretry() can/will handle all cases for us.
|
|
||
| if (uv_is_active((uv_handle_t*)&sam_u->nat_u.pol_u)) { | ||
| uv_close((uv_handle_t*)&sam_u->nat_u.pol_u, 0); | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is quite right -- the definition of "active" for different handle types is fairly involved. What we actually want is just "has this handle every been initialized?". We can obviously track that ourselves, but I think this will also work:
if ( UV_UNKNOWN_HANDLE != uv_handle_get_type(&sam_u->nat_u.pol_u) ) {
uv_close(...)
}
UV_UNKNOWN_HANDLE is 0, and we zero-initialize the uv_poll_t struct by virtue of allocating the u3_ames struct with c3_calloc().
This is #593 that got reverted in #644 because it broke
urbit/urbitCI. This PR fixes the issue which was unconditionally callinguv_poll_initon a socket that was initialized withinitnatpmpwithout checking ifinitnatpmpreturned an error first.