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

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, Aug 04, 2021 at 01:07:42AM +0200, Vladislav Shpilevoy wrote:
> > +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.

Let's leave response_len to emphasize that response is a string.

> > +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?

No reason. Fixed.

> 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.

I honestly don't know, but we always define them as static:

~/src/tarantool$ git grep 'luaL_Reg.*{' src | wc -l
60
~/src/tarantool$ git grep 'luaL_Reg.*{' src | grep -c -v static
0

Maybe because we are afraid of running out of stack?
I woulnd't bother anyway.

> > +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.

Fixed in a separate patch:

https://lists.tarantool.org/pipermail/tarantool-patches/2021-August/025208.html

Incremental diff:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index eec5e5bcc519..d41a8d654a8b 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -1762,12 +1762,11 @@ netbox_dispatch_response_console(struct lua_State *L,
 static int
 netbox_console_setup(struct lua_State *L)
 {
-	static const char setup_delimiter_cmd[] =
+	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;
+	const size_t setup_delimiter_cmd_len = strlen(setup_delimiter_cmd);
+	const char *ok_response = "---\n...\n";
+	const size_t ok_response_len = strlen(ok_response);
 	int fd = lua_tointeger(L, 1);
 	struct ibuf *send_buf = (struct ibuf *)lua_topointer(L, 2);
 	struct ibuf *recv_buf = (struct ibuf *)lua_topointer(L, 3);
@@ -1782,7 +1781,8 @@ netbox_console_setup(struct lua_State *L)
 		luaL_testcancel(L);
 		goto error;
 	}
-	if (strncmp(response, ok_response, ok_response_len) != 0) {
+	if (response_len != ok_response_len ||
+	    strncmp(response, ok_response, ok_response_len) != 0) {
 		box_error_raise(ER_NO_CONNECTION, "Unexpected response");
 		goto error;
 	}