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

Update libuv to 1.41.0 #534

Merged
merged 7 commits into from
Apr 18, 2021
Merged

Update libuv to 1.41.0 #534

merged 7 commits into from
Apr 18, 2021

Conversation

creationix
Copy link
Member

No description provided.

@squeek502
Copy link
Member

squeek502 commented Apr 12, 2021

New function docs:

(for some reason .ci/bindcov.sh doesn't report uv_pipe as a missing binding, will try to look into that)

e.g. the uv_pipe function added in 1.41.0 being a substring of uv_pipe_getsockname, etc
Tests are ported from Libuv
Copy link
Member

@zhaozg zhaozg left a comment

Choose a reason for hiding this comment

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

Good PR.

Them being at index 3 and 4 was left over from my initial misguided attempt at binding this function. Whoops!
@zhaozg
Copy link
Member

zhaozg commented Apr 17, 2021

Are you plan to do uv_socketpair?

@squeek502
Copy link
Member

Yes, but I haven't decided how to bind it yet, since the type and protocol parameters are slightly weird. Here's what I'm thinking currently:

type:

protocol:

  • This one seems tough to bind in a user-friendly way, since it relies on type and domain, so we might just need to leave it as an integer
  • The Libuv test suite only uses 0 for this parameter
  • Note: on Windows, AF_INET is used as the domain when creating the sockets; on Unix, AF_UNIX is used as the domain

So maybe something like this?

uv.socketpair([type], [protocol], [flags0], [flags1])

**Parameters:**
- `type`: `string` or `nil` (default: `stream`)
- `protocol`: `integer` or `nil` (default: 0)
- `flags0`: `table` or `nil`
  - `nonblock`: `boolean` (default: `false`)
- `flags1`: `table` or `nil`
  - `nonblock`: `boolean` (default: `false`)

Let me know if you have any better ideas.

@zhaozg
Copy link
Member

zhaozg commented Apr 17, 2021

Look https://man7.org/linux/man-pages/man2/socketpair.2.html, and https://man7.org/linux/man-pages/man2/socket.2.html, I prefer to reuse the code for type and protocol in

luv/src/dns.c

Lines 118 to 148 in b3a7e09

// Process `socktype` hint
lua_getfield(L, 3, "socktype");
if (lua_isnumber(L, -1)) {
hints->ai_socktype = lua_tointeger(L, -1);
}
else if (lua_isstring(L, -1)) {
hints->ai_socktype = luv_sock_string_to_num(lua_tostring(L, -1));
}
else if (!lua_isnil(L, -1)) {
return luaL_argerror(L, 3, "socktype hint must be string if set");
}
lua_pop(L, 1);
// Process the `protocol` hint
lua_getfield(L, 3, "protocol");
if (lua_isnumber(L, -1)) {
hints->ai_protocol = lua_tointeger(L, -1);
}
else if (lua_isstring(L, -1)) {
int protocol = luv_af_string_to_num(lua_tostring(L, -1));
if (protocol) {
hints->ai_protocol = protocol;
}
else {
return luaL_argerror(L, 3, "Invalid protocol hint");
}
}
else if (!lua_isnil(L, -1)) {
return luaL_argerror(L, 3, "protocol hint must be string if set");
}
lua_pop(L, 1);

for socket() we should keep open accpet wide value range, If not right, libuv will report error in runtime.

@squeek502
Copy link
Member

squeek502 commented Apr 17, 2021

I think we're actually binding the protocol incorrectly in getaddrinfo. It treats it like an AF_ constant, but those are separate from the protocol constants.

From socket(2):

The protocol specifies a particular protocol to be used with the socket. Normally only a single protocol exists to support a particular socket type within a given protocol family, in which case protocol can be specified as 0. However, it is possible that many protocols may exist, in which case a particular protocol must be specified in this manner. The protocol number to use is specific to the “communication domain” in which communication is to take place; see protocols(5). See getprotoent(3) on how to map protocol name strings to protocol numbers.

From protocols(5):

This file is a plain ASCII file, describing the various DARPA internet protocols that are available from the TCP/IP subsystem.

From my local /etc/protocols file:

udp 17 UDP # user datagram protocol

And from the output of test-dns.lua

{ addr = "206.189.225.172", protocol = "packet", family = "inet", socktype = "dgram" }

That is, luv is translating the returned protocol 17 into "packet" (because AF_PACKET is 17) here:

luv/src/dns.c

Line 52 in b3a7e09

lua_pushstring(L, luv_af_num_to_string(curr->ai_protocol));

but that is incorrect; it is actually the udp protocol. If we still want to translate between string <-> number, we should probably be using getprotobyname/getprotobynumber instead, but I'm not sure if there's something similar on Windows. EDIT: The functions also exist on Windows

I'll try implementing how I think protocols should be handled when binding socketpair, but I'll leave getaddrinfo alone and submit a PR afterwards to fix how that handles protocols.

On Windows, socketpair creates TCP sockets that don't get detected as "pipe" from guess_handle
@squeek502 squeek502 changed the title Update libuv to 1.41.0, but not add new bindings yet Update libuv to 1.41.0 Apr 18, 2021
Comment on lines +140 to +146
if (lua_type(L, 1) == LUA_TTABLE) {
lua_getfield(L, 1, "nonblock");
if (lua_toboolean(L, -1)) read_flags |= UV_NONBLOCK_PIPE;
lua_pop(L, 1);
} else if (!lua_isnoneornil(L, 1)) {
luv_arg_type_error(L, 1, "table or nil expected, got %s");
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to wrap it as a static function to reuse the code.

@zhaozg zhaozg merged commit 2f0f3d3 into master Apr 18, 2021
@creationix
Copy link
Member Author

Nice work guys! Also I'm not surprised I got the protocol stuff wrong. I barely understood it at the time.

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