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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

>  
>  static int
> @@ -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);
> +	if (fh < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (coio_readdir(pathname, &buf) < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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);
> +	if (res < 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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;
>  	}
>  
> -	if (coio_tempdir(buf, PATH_MAX) != 0) {
> -		lua_pushnil(L);
> +	if (coio_tempdir(buf, PATH_MAX) != 0)
>  		lbox_fio_pushsyserror(L);
> -		return 2;
> -	}
>  	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 {
> +	if (getcwd(buf, PATH_MAX) == NULL)
>  		lbox_fio_pushsyserror(L);
> -		lua_pushnil(L);
> -		return 2;
> -	}
> +	lua_pushstring(L, buf);
> +	lua_remove(L, -2);
>  	return 1;
>  }