Skip to content

Conversation

@oyaya
Copy link
Contributor

@oyaya oyaya commented Dec 2, 2024

bugfix: make it possible to bind ip:port or ip from support only binding ip.

@chensunny
Copy link
Contributor

chensunny commented Apr 18, 2025

  1. Which cases will use the ip: port ,It looks very strange.
  2. It may cause a Breaking Change in some cases

Because, as a TCP client, the port will be assigned by the Linux kernel;

@oyaya

ngx_http_lua_socket_tcp_get_peer(ngx_peer_connection_t *pc, void *data)
{
/* empty */
pc->type = SOCK_STREAM;
Copy link
Contributor

Choose a reason for hiding this comment

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

@mergify
Copy link

mergify bot commented Apr 18, 2025

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Apr 18, 2025

len -= plen + 1;

addr = ngx_http_lua_parse_addr(L, text, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeated parsing of addr from above

@chensunny
Copy link
Contributor

chensunny commented Apr 18, 2025

Just have a discussion:
Maybe sock:bind(ip,port) is better for Breaking Change?

@zhuizhuhaomeng

Comment on lines +4602 to +4610
addr = ngx_http_lua_parse_addr(L, text, len);
if (addr == NULL) {
return NULL;
}

ngx_inet_set_port(addr->sockaddr, (in_port_t) port);

return addr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong indentation

text = (u_char *) luaL_checklstring(L, 2, &len);

local = ngx_http_lua_parse_addr(L, text, len);
local = ngx_http_lua_parse_addr_port(L, text, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

please add test cases

@mergify mergify bot removed the conflict label May 6, 2025
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.

3 participants