[tarantool-patches] Re: [PATCH v2 1/2] lua: new function luaT_return_error()

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. I made changes you mentioned and
renamed function to luaT_push_nil_and_error().

Also, I noted that in function lbox_fio_cwd() before this
patch something was not right. Diff:

@@ -650,20 +622,12 @@ lbox_fio_cwd(struct lua_State *L)
 	char *buf = (char *)lua_newuserdata(L, PATH_MAX);
 	if (!buf) {
 		errno = ENOMEM;
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
-
-
-	if (getcwd(buf, PATH_MAX)) {
-		lua_pushstring(L, buf);
-		lua_remove(L, -2);
-	} else {
-		lbox_fio_pushsyserror(L);
-		lua_pushnil(L);
-		return 2;
+		return lbox_fio_pushsyserror(L);
 	}
+	if (getcwd(buf, PATH_MAX) == NULL)
+		return lbox_fio_pushsyserror(L);
+	lua_pushstring(L, buf);
+	lua_remove(L, -2);
 	return 1;
 }

As you can see, in the first case nil was the first and
error was the second, but it wasn't so in the other case.
I think this was a bug.

My answer and new patch below.

On Wed, Jul 31, 2019 at 07:15:13PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
>> diff --git a/src/lua/fio.c b/src/lua/fio.c
>> index 55fa667..cb7e31e 100644
>> --- a/src/lua/fio.c
>> +++ b/src/lua/fio.c
>> @@ -47,11 +47,9 @@
>>  #include "lua/utils.h"
>>  #include "coio_file.h"
>>  
>> -static inline void
>> -lbox_fio_pushsyserror(struct lua_State *L)
>> -{
>> -	diag_set(SystemError, "fio: %s", strerror(errno));
>> -	luaT_pusherror(L, diag_get()->last);
>> +#define lbox_fio_pushsyserror(L) {				\
>> +	diag_set(SystemError, "fio: %s", strerror(errno));	\
>> +	return luaT_return_error(L);				\
>>  }
>
> Please, don't return here. It makes the code below
> confusing, when you call 'pushsyserror, pushinteger', and
> return 1. Make this macro return value of the last block,
> and surround it with '()' or with 'do {...} while(false)'
> to be force ';' after it, and be able to use it in
> 'if ... else ...' constructions. It is a standard practice.
>
> #define lbox_fio_pushsyserror(L) ({				\
> 	diag_set(SystemError, "fio: %s", strerror(errno));	\
> 	luaT_return_error(L);					\
> })
>
> and use 'return lbox_fio_pushsyserror(L);' in the code
> below.
>
Thank you! Fixed.


New patch:

From 06f6a8f4e103e759233823ea508f9764a371f026 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 31 Jul 2019 12:43:58 +0300
Subject: [PATCH] lua: new function luaT_push_nil_and_error()

Currently, if we have to return errors using the format
'return nil, error', we must do it manually. Since this error
return method is described in our Lua coding style, it makes sense
to create a function that will return an error in this format.
This patch creates mentioned function.

Needed for #4390

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c0ac4917b..b9495e7a6 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -395,14 +395,10 @@ lbox_session_push(struct lua_State *L)
 	}
 	struct port port;
 	port_lua_create(&port, L);
-	if (session_push(session, sync, &port) != 0) {
-		lua_pushnil(L);
-		luaT_pusherror(L, box_error_last());
-		return 2;
-	} else {
-		lua_pushboolean(L, true);
-		return 1;
-	}
+	if (session_push(session, sync, &port) != 0)
+		return luaT_push_nil_and_error(L);
+	lua_pushboolean(L, true);
+	return 1;
 }
 
 /**
diff --git a/src/lua/error.c b/src/lua/error.c
index fca87f9d6..d82e78dc4 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -104,6 +104,16 @@ luaT_error(lua_State *L)
 	return 0;
 }
 
+int
+luaT_push_nil_and_error(lua_State *L)
+{
+	struct error *e = diag_last_error(&fiber()->diag);
+	assert(e != NULL);
+	lua_pushnil(L);
+	luaT_pusherror(L, e);
+	return 2;
+}
+
 void
 tarantool_lua_error_init(struct lua_State *L)
 {
diff --git a/src/lua/error.h b/src/lua/error.h
index 67a43da86..64fa5eba3 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -49,6 +49,15 @@ struct error;
 LUA_API int
 luaT_error(lua_State *L);
 
+/**
+ * Return nil as the first return value and an error as the
+ * second. The error is received using box_error_last().
+ *
+ * @param L Lua stack.
+ */
+LUA_API int
+luaT_push_nil_and_error(lua_State *L);
+
 void
 luaT_pusherror(struct lua_State *L, struct error *e);
 /** \endcond public */
diff --git a/src/lua/fio.c b/src/lua/fio.c
index 55fa66762..e5d3458fd 100644
--- a/src/lua/fio.c
+++ b/src/lua/fio.c
@@ -47,12 +47,10 @@
 #include "lua/utils.h"
 #include "coio_file.h"
 
-static inline void
-lbox_fio_pushsyserror(struct lua_State *L)
-{
-	diag_set(SystemError, "fio: %s", strerror(errno));
-	luaT_pusherror(L, diag_get()->last);
-}
+#define lbox_fio_pushsyserror(L) ({				\
+	diag_set(SystemError, "fio: %s", strerror(errno));	\
+	luaT_push_nil_and_error(L);					\
+})
 
 static int
 lbox_fio_open(struct lua_State *L)
@@ -69,11 +67,8 @@ usage:
 	int mode = lua_tointeger(L, 3);
 
 	int fh = coio_file_open(pathname, flags, mode);
-	if (fh < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (fh < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushinteger(L, fh);
 	return 1;
 }
@@ -90,11 +85,8 @@ lbox_fio_pwrite(struct lua_State *L)
 	size_t offset = lua_tonumber(L, 4);
 
 	int res = coio_pwrite(fh, buf, len, offset);
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushinteger(L, res);
 	return 1;
 }
@@ -115,11 +107,8 @@ lbox_fio_pread(struct lua_State *L)
 
 	int res = coio_pread(fh, buf, len, offset);
 
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushinteger(L, res);
 	return 1;
 }
@@ -129,7 +118,10 @@ lbox_fio_pushbool(struct lua_State *L, bool res)
 {
 	lua_pushboolean(L, res);
 	if (!res) {
-		lbox_fio_pushsyserror(L);
+		diag_set(SystemError, "fio: %s", strerror(errno));
+		struct error *e = diag_last_error(diag_get());
+		assert(e != NULL);
+		luaT_pusherror(L, e);
 		return 2;
 	}
 	return 1;
@@ -211,11 +203,8 @@ lbox_fio_write(struct lua_State *L)
 	size_t len = lua_tonumber(L, 3);
 
 	int res = coio_write(fh, buf, len);
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushinteger(L, res);
 	return 1;
 }
@@ -298,11 +287,8 @@ lbox_fio_read(struct lua_State *L)
 
 	int res = coio_read(fh, buf, len);
 
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushinteger(L, res);
 	return 1;
 }
@@ -371,11 +357,8 @@ DEF_STAT_METHOD(is_sock, S_ISSOCK);
 static int
 lbox_fio_pushstat(struct lua_State *L, int res, const struct stat *stat)
 {
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_newtable(L);
 
 	PUSHTABLE("dev", lua_pushinteger, stat->st_dev);
@@ -518,11 +501,8 @@ lbox_fio_listdir(struct lua_State *L)
 	}
 	pathname = lua_tostring(L, 1);
 	char *buf;
-	if (coio_readdir(pathname, &buf) < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (coio_readdir(pathname, &buf) < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushstring(L, buf);
 	free(buf);
 	return 1;
@@ -613,11 +593,8 @@ usage:
 		goto usage;
 	char *path = (char *)lua_newuserdata(L, PATH_MAX);
 	int res = coio_readlink(pathname, path, PATH_MAX);
-	if (res < 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (res < 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushlstring(L, path, res);
 	lua_remove(L, -2);
 	return 1;
@@ -629,16 +606,11 @@ lbox_fio_tempdir(struct lua_State *L)
 	char *buf = (char *)lua_newuserdata(L, PATH_MAX);
 	if (!buf) {
 		errno = ENOMEM;
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
+		return lbox_fio_pushsyserror(L);
 	}
 
-	if (coio_tempdir(buf, PATH_MAX) != 0) {
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
+	if (coio_tempdir(buf, PATH_MAX) != 0)
+		return lbox_fio_pushsyserror(L);
 	lua_pushstring(L, buf);
 	lua_remove(L, -2);
 	return 1;
@@ -650,20 +622,12 @@ lbox_fio_cwd(struct lua_State *L)
 	char *buf = (char *)lua_newuserdata(L, PATH_MAX);
 	if (!buf) {
 		errno = ENOMEM;
-		lua_pushnil(L);
-		lbox_fio_pushsyserror(L);
-		return 2;
-	}
-
-
-	if (getcwd(buf, PATH_MAX)) {
-		lua_pushstring(L, buf);
-		lua_remove(L, -2);
-	} else {
-		lbox_fio_pushsyserror(L);
-		lua_pushnil(L);
-		return 2;
+		return lbox_fio_pushsyserror(L);
 	}
+	if (getcwd(buf, PATH_MAX) == NULL)
+		return lbox_fio_pushsyserror(L);
+	lua_pushstring(L, buf);
+	lua_remove(L, -2);
 	return 1;
 }
 
diff --git a/src/lua/swim.c b/src/lua/swim.c
index 26646f41f..ae916bf78 100644
--- a/src/lua/swim.c
+++ b/src/lua/swim.c
@@ -69,11 +69,10 @@ lua_swim_new(struct lua_State *L)
 {
 	uint64_t generation = luaL_checkuint64(L, 1);
 	struct swim *s = swim_new(generation);
+	if (s == NULL)
+		return luaT_push_nil_and_error(L);
 	*(struct swim **) luaL_pushcdata(L, ctid_swim_ptr) = s;
-	if (s != NULL)
-		return 1;
-	luaT_pusherror(L, diag_last_error(diag_get()));
-	return 2;
+	return 1;
 }
 
 /**