[Tarantool-patches] [PATCH 17/20] net.box: rewrite console handlers in C

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 1a615797d485..85a45c54b979 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -850,30 +850,25 @@ netbox_send_and_recv_iproto(lua_State *L)
>  
>  /*
>   * Sends and receives data over a console connection.
> - * Takes socket fd, send_buf (ibuf), recv_buf (ibuf), timeout.
> - * On success returns response (string).
> - * On error returns nil, error.
> + * Returns a pointer to a response string and its len.
> + * On error returns NULL.
>   */
> -static int
> -netbox_send_and_recv_console(lua_State *L)
> +static const char *
> +netbox_send_and_recv_console(int fd, struct ibuf *send_buf,
> +			     struct ibuf *recv_buf, double timeout,
> +			     size_t *response_len)

1. To be consistent with netbox_communicate() I would call the
last argument 'size'. Up to you.

<...>

> +
> +/*
> + * Sets up console delimiter. Should be called before serving any requests.
> + * Takes socket fd, send_buf (ibuf), recv_buf (ibuf), timeout.
> + * Returns none on success, error on failure.
> + */
> +static int
> +netbox_console_setup(struct lua_State *L)
> +{
> +	static const char setup_delimiter_cmd[] =
> +		"require('console').delimiter('$EOF$')\n";
> +	static const size_t setup_delimiter_cmd_len =
> +		sizeof(setup_delimiter_cmd) - 1;
> +	static const char ok_response[] = "---\n...\n";
> +	static const size_t ok_response_len = sizeof(ok_response) - 1;

2. Why do you make them static? Wouldn't it be enough to
just store them on the stack? Btw, the same question for
netbox_registry_meta, netbox_request_meta, net_box_lib.
Why should they be static? It seems Lua anyway copies them
somewhere, so they don't need to stay valid for the entire
process' lifetime.

> +
> +/*
> + * Processes console requests in a loop until an error.
> + * Takes request registry, socket fd, send_buf (ibuf), recv_buf (ibuf), timeout.
> + * Returns the error that broke the loop.
> + */
> +static int
> +netbox_console_loop(struct lua_State *L)
> +{
> +	struct netbox_registry *registry = luaT_check_netbox_registry(L, 1);
> +	int fd = lua_tointeger(L, 2);
> +	struct ibuf *send_buf = (struct ibuf *) lua_topointer(L, 3);
> +	struct ibuf *recv_buf = (struct ibuf *) lua_topointer(L, 4);
> +	double timeout = (!lua_isnoneornil(L, 5) ?
> +			  lua_tonumber(L, 5) : TIMEOUT_INFINITY);

3. You never pass timeout as not nil. It is always TIMEOUT_INFINITY.

> +	uint64_t sync = registry->next_sync;
> +	while (true) {
> +		size_t response_len;
> +		const char *response = netbox_send_and_recv_console(
> +			fd, send_buf, recv_buf, timeout, &response_len);
> +		if (response == NULL) {
> +			luaL_testcancel(L);
> +			luaT_pusherror(L, box_error_last());
> +			return 1;
> +		}
> +		netbox_dispatch_response_console(L, registry, sync++,
> +						 response, response_len);
> +	}
>  }