Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/3] Replication status errno
@ 2019-11-05 14:59 Vladislav Shpilevoy
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user Vladislav Shpilevoy
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-05 14:59 UTC (permalink / raw)
  To: tarantool-patches, kostja.osipov

The patchset makes box.info.replication*.upstream/downstream show
errno description for system errors.

I made that it shows strerror(errno) of the latest system error,
but probably it would be better to show errno value, and let a
user to translate it manually using Lua errno module. On the other
hand my way is simpler for a user when he manually calls box.info
in a console to check a replication status. That is discussable.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4402-replication-system-error
Issue: https://github.com/tarantool/tarantool/issues/4402

Vladislav Shpilevoy (3):
  access: fix invalid error type for not found user
  error: move errno into an error object
  replication: show errno in replication info

 src/box/applier.cc                           |  2 +-
 src/box/lua/info.c                           | 29 +++++----
 src/box/schema.cc                            |  5 +-
 src/box/user.cc                              |  5 +-
 src/lib/core/diag.c                          |  1 +
 src/lib/core/diag.h                          |  6 ++
 src/lib/core/exception.cc                    | 22 +++----
 src/lib/core/exception.h                     |  5 --
 src/lua/error.lua                            | 10 ++++
 test/box/access.result                       |  3 +-
 test/box/misc.result                         | 19 ++++++
 test/box/misc.test.lua                       | 10 ++++
 test/replication/gh-4402-info-errno.result   | 63 ++++++++++++++++++++
 test/replication/gh-4402-info-errno.test.lua | 25 ++++++++
 test/replication/suite.cfg                   |  1 +
 15 files changed, 170 insertions(+), 36 deletions(-)
 create mode 100644 test/replication/gh-4402-info-errno.result
 create mode 100644 test/replication/gh-4402-info-errno.test.lua

-- 
2.21.0 (Apple Git-122.2)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user
  2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
@ 2019-11-05 14:59 ` Vladislav Shpilevoy
  2019-11-05 18:15   ` Konstantin Osipov
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 2/3] error: move errno into an error object Vladislav Shpilevoy
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-05 14:59 UTC (permalink / raw)
  To: tarantool-patches, kostja.osipov

Box.session.su() raised 'SystemError' when a user was not found
due to a too long user name. That was obviously wrong, because
SystemError is always something related to libraries (standard,
curl, etc), and it has an errno code.

Now a ClientError is raised.
---
 src/box/schema.cc      | 5 ++---
 src/box/user.cc        | 5 +++--
 test/box/access.result | 3 ++-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 9767207e0..fab7544f2 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -311,9 +311,8 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
 	       const char *name, uint32_t len, uint32_t *object_id)
 {
 	if (len > BOX_NAME_MAX) {
-		diag_set(SystemError,
-			 "name length %d is greater than BOX_NAME_MAX", len);
-		return -1;
+		*object_id = BOX_ID_NIL;
+		return 0;
 	}
 	struct space *space = space_cache_find(system_space_id);
 	if (space == NULL)
diff --git a/src/box/user.cc b/src/box/user.cc
index 03b4b2e3b..15c40f90d 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -521,8 +521,9 @@ user_find_by_name(const char *name, uint32_t len)
 	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
 		return NULL;
 	struct user *user = user_by_id(uid);
-	if (user == NULL || user->def->type != SC_USER) {
-		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
+	if (user == NULL || user->def->type != SC_USER || uid == BOX_ID_NIL) {
+		diag_set(ClientError, ER_NO_SUCH_USER,
+			 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
 		return NULL;
 	}
 	return user;
diff --git a/test/box/access.result b/test/box/access.result
index dc339038d..05e2f4147 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -248,7 +248,8 @@ session.su('does not exist')
 -- allowed name.
 session.su(string.rep('a', 66000))
 ---
-- error: name length 66000 is greater than BOX_NAME_MAX
+- error: User 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' is
+    not found
 ...
 --------------------------------------------------------------------------------
 -- Check if identifiers obey the common constraints
-- 
2.21.0 (Apple Git-122.2)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Tarantool-patches] [PATCH 2/3] error: move errno into an error object
  2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user Vladislav Shpilevoy
@ 2019-11-05 14:59 ` Vladislav Shpilevoy
  2019-11-05 18:18   ` Konstantin Osipov
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info Vladislav Shpilevoy
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-05 14:59 UTC (permalink / raw)
  To: tarantool-patches, kostja.osipov

The only error type having an errno as a part of it was
SystemError (and its descendants SocketError, TimedOut, OOM, ...).
That was used in logs (SystemError::log() method), and exposed to
Lua (if type was SystemError, an error object had 'errno' field).

But actually errno might be useful not only there. For example,
box.info.replication exposes the latest error message of
applier/relay as 'message' field of 'upstream/downstream' fields,
lacking errno description.

Before the patch it was impossible to obtain an errno code from C,
because it was necessary to check whether an error has SystemError
type, cast to SystemError class, and call SystemError::get_errno()
method.

Now errno is available as a part of struct error object (available
from C), and is not 0 for system errors.

Part of #4402
---
 src/box/applier.cc        |  2 +-
 src/lib/core/diag.c       |  1 +
 src/lib/core/diag.h       |  6 ++++++
 src/lib/core/exception.cc | 22 ++++++++--------------
 src/lib/core/exception.h  |  5 -----
 src/lua/error.lua         | 10 ++++++++++
 test/box/misc.result      | 19 +++++++++++++++++++
 test/box/misc.test.lua    | 10 ++++++++++
 8 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index a04d13564..a287f4219 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -181,7 +181,7 @@ applier_writer_f(va_list ap)
 			 * the master closed its end - we would only
 			 * spam the log - so exit immediately.
 			 */
-			if (e->get_errno() == EPIPE)
+			if (e->syserror == EPIPE)
 				break;
 			/*
 			 * Do not exit, if there is a network error,
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index 248277e74..168f08225 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -44,6 +44,7 @@ error_create(struct error *e,
 	e->log = log;
 	e->type = type;
 	e->refs = 0;
+	e->syserror = 0;
 	if (file != NULL) {
 		snprintf(e->file, sizeof(e->file), "%s", file);
 		e->line = line;
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fd3831e66..2f106d33f 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -72,6 +72,12 @@ struct error {
 	error_f log;
 	const struct type_info *type;
 	int refs;
+	/**
+	 * Errno at the moment of the error
+	 * creation. If the error is not related
+	 * to the standard library, then it is 0.
+	 */
+	int syserror;
 	/** Line number. */
 	unsigned line;
 	/* Source file name. */
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index a6999af43..6e725a36c 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -129,27 +129,21 @@ Exception::log() const
 	say_file_line(S_ERROR, file, line, errmsg, "%s", type->name);
 }
 
-static const struct method_info systemerror_methods[] = {
-	make_method(&type_SystemError, "errno", &SystemError::get_errno),
-	METHODS_SENTINEL
-};
-
 const struct type_info type_SystemError =
-	make_type("SystemError", &type_Exception, systemerror_methods);
+	make_type("SystemError", &type_Exception);
 
 SystemError::SystemError(const struct type_info *type,
 			 const char *file, unsigned line)
-	:Exception(type, file, line),
-	m_errno(errno)
+	:Exception(type, file, line)
 {
-	/* nothing */
+	syserror = errno;
 }
 
 SystemError::SystemError(const char *file, unsigned line,
 			 const char *format, ...)
-	: Exception(&type_SystemError, file, line),
-	m_errno(errno)
+	: Exception(&type_SystemError, file, line)
 {
+	syserror = errno;
 	va_list ap;
 	va_start(ap, format);
 	error_vformat_msg(this, format, ap);
@@ -159,7 +153,7 @@ SystemError::SystemError(const char *file, unsigned line,
 void
 SystemError::log() const
 {
-	say_file_line(S_SYSERROR, file, line, strerror(m_errno),
+	say_file_line(S_SYSERROR, file, line, strerror(syserror),
 		      "SystemError %s", errmsg);
 }
 
@@ -192,7 +186,7 @@ OutOfMemory::OutOfMemory(const char *file, unsigned line,
 			 const char *object)
 	: SystemError(&type_OutOfMemory, file, line)
 {
-	m_errno = ENOMEM;
+	syserror = ENOMEM;
 	error_format_msg(this, "Failed to allocate %u bytes in %s for %s",
 			 (unsigned) amount, allocator, object);
 }
@@ -203,7 +197,7 @@ const struct type_info type_TimedOut =
 TimedOut::TimedOut(const char *file, unsigned line)
 	: SystemError(&type_TimedOut, file, line)
 {
-	m_errno = ETIMEDOUT;
+	syserror = ETIMEDOUT;
 	error_format_msg(this, "timed out");
 }
 
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index a29281427..d6154eb32 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -86,17 +86,12 @@ class SystemError: public Exception {
 public:
 	virtual void raise() { throw this; }
 
-	int get_errno() const { return m_errno; }
-
 	virtual void log() const;
 
 	SystemError(const char *file, unsigned line,
 		    const char *format, ...);
 protected:
 	SystemError(const struct type_info *type, const char *file, unsigned line);
-protected:
-	/* system errno */
-	int m_errno;
 };
 
 extern const struct type_info type_SocketError;
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 28fc0377d..575594273 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -17,6 +17,7 @@ struct error {
     error_f _log;
     const struct type_info *_type;
     int _refs;
+    int _syserror;
     /** Line number. */
     unsigned _line;
     /* Source file name. */
@@ -86,10 +87,19 @@ local function error_trace(err)
     }
 end
 
+local function error_errno(err)
+    local e = err._syserror
+    if e == 0 then
+        return nil
+    end
+    return e
+end
+
 local error_fields = {
     ["type"]        = error_type;
     ["message"]     = error_message;
     ["trace"]       = error_trace;
+    ["errno"]       = error_errno;
 }
 
 local function error_unpack(err)
diff --git a/test/box/misc.result b/test/box/misc.result
index b2930515b..d2a20307a 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -196,6 +196,25 @@ box.error.new()
 ---
 - error: 'Usage: box.error.new(code, args)'
 ...
+--
+-- System errors expose errno as a field.
+--
+_, err = require('fio').open('not_existing_file')
+---
+...
+type(err.errno)
+---
+- number
+...
+-- Errors not related to the standard library do
+-- not expose errno.
+err = box.error.new(box.error.PROC_LUA, "errno")
+---
+...
+type(err.errno)
+---
+- nil
+...
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index cc223b2ef..c3e992a48 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -59,6 +59,16 @@ e = box.error.new(box.error.CREATE_SPACE, "space", "error")
 e
 box.error.new()
 
+--
+-- System errors expose errno as a field.
+--
+_, err = require('fio').open('not_existing_file')
+type(err.errno)
+-- Errors not related to the standard library do
+-- not expose errno.
+err = box.error.new(box.error.PROC_LUA, "errno")
+type(err.errno)
+
 ----------------
 -- # box.stat
 ----------------
-- 
2.21.0 (Apple Git-122.2)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info
  2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user Vladislav Shpilevoy
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 2/3] error: move errno into an error object Vladislav Shpilevoy
@ 2019-11-05 14:59 ` Vladislav Shpilevoy
  2019-11-05 18:18   ` Konstantin Osipov
  2019-11-05 18:13 ` [Tarantool-patches] [PATCH 0/3] Replication status errno Konstantin Osipov
  2019-11-21 17:38 ` Kirill Yukhin
  4 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-05 14:59 UTC (permalink / raw)
  To: tarantool-patches, kostja.osipov

Box.info.replication shows applier/relay's latest error message.
But it didn't include errno description for system errors, even
though it was included in the logs. Now box.info shows the errno
description as well, when possible.

Closes #4402
---
 src/box/lua/info.c                           | 29 +++++----
 test/replication/gh-4402-info-errno.result   | 63 ++++++++++++++++++++
 test/replication/gh-4402-info-errno.test.lua | 25 ++++++++
 test/replication/suite.cfg                   |  1 +
 4 files changed, 108 insertions(+), 10 deletions(-)
 create mode 100644 test/replication/gh-4402-info-errno.result
 create mode 100644 test/replication/gh-4402-info-errno.test.lua

diff --git a/src/box/lua/info.c b/src/box/lua/info.c
index 55382fd77..b4475959b 100644
--- a/src/box/lua/info.c
+++ b/src/box/lua/info.c
@@ -50,6 +50,7 @@
 #include "box/box.h"
 #include "lua/utils.h"
 #include "fiber.h"
+#include "tt_static.h"
 
 static void
 lbox_pushvclock(struct lua_State *L, const struct vclock *vclock)
@@ -65,6 +66,20 @@ lbox_pushvclock(struct lua_State *L, const struct vclock *vclock)
 	luaL_setmaphint(L, -1); /* compact flow */
 }
 
+static inline void
+lbox_push_replication_error_message(struct lua_State *L, struct error *e,
+				    int idx)
+{
+	lua_pushstring(L, "message");
+	lua_pushstring(L, e->errmsg);
+	lua_settable(L, idx - 2);
+	if (e->syserror == 0)
+		return;
+	lua_pushstring(L, "system_message");
+	lua_pushstring(L, strerror(e->syserror));
+	lua_settable(L, idx - 2);
+}
+
 static void
 lbox_pushapplier(lua_State *L, struct applier *applier)
 {
@@ -103,11 +118,8 @@ lbox_pushapplier(lua_State *L, struct applier *applier)
 		lua_settable(L, -3);
 
 		struct error *e = diag_last_error(&applier->reader->diag);
-		if (e != NULL) {
-			lua_pushstring(L, "message");
-			lua_pushstring(L, e->errmsg);
-			lua_settable(L, -3);
-		}
+		if (e != NULL)
+			lbox_push_replication_error_message(L, e, -1);
 	}
 }
 
@@ -135,11 +147,8 @@ lbox_pushrelay(lua_State *L, struct relay *relay)
 		lua_settable(L, -3);
 
 		struct error *e = diag_last_error(relay_get_diag(relay));
-		if (e != NULL) {
-			lua_pushstring(L, "message");
-			lua_pushstring(L, e->errmsg);
-			lua_settable(L, -3);
-		}
+		if (e != NULL)
+			lbox_push_replication_error_message(L, e, -1);
 		break;
 	}
 	default: unreachable();
diff --git a/test/replication/gh-4402-info-errno.result b/test/replication/gh-4402-info-errno.result
new file mode 100644
index 000000000..661eea38b
--- /dev/null
+++ b/test/replication/gh-4402-info-errno.result
@@ -0,0 +1,63 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+--
+-- gh-4402: replication info table should contain not only
+-- Tarantool's error, but also a system error (errno's message)
+-- when possible.
+--
+
+box.schema.user.grant('guest', 'replication')
+ | ---
+ | ...
+
+test_run:cmd('create server replica with rpl_master=default, script="replication/replica.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica')
+ | ---
+ | - true
+ | ...
+i = box.info
+ | ---
+ | ...
+replica_id = i.id % 2 + 1
+ | ---
+ | ...
+d = i.replication[replica_id].downstream
+ | ---
+ | ...
+d ~= nil and d.status == 'follow' or i
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('stop server replica')
+ | ---
+ | - true
+ | ...
+test_run:cmd("cleanup server replica")
+ | ---
+ | - true
+ | ...
+test_run:cmd("delete server replica")
+ | ---
+ | - true
+ | ...
+i = box.info
+ | ---
+ | ...
+d = i.replication[replica_id].downstream
+ | ---
+ | ...
+d ~= nil and d.system_message ~= nil and d.message ~= nil or i
+ | ---
+ | - true
+ | ...
+
+box.schema.user.revoke('guest', 'replication')
+ | ---
+ | ...
diff --git a/test/replication/gh-4402-info-errno.test.lua b/test/replication/gh-4402-info-errno.test.lua
new file mode 100644
index 000000000..1b2d9d814
--- /dev/null
+++ b/test/replication/gh-4402-info-errno.test.lua
@@ -0,0 +1,25 @@
+test_run = require('test_run').new()
+
+--
+-- gh-4402: replication info table should contain not only
+-- Tarantool's error, but also a system error (errno's message)
+-- when possible.
+--
+
+box.schema.user.grant('guest', 'replication')
+
+test_run:cmd('create server replica with rpl_master=default, script="replication/replica.lua"')
+test_run:cmd('start server replica')
+i = box.info
+replica_id = i.id % 2 + 1
+d = i.replication[replica_id].downstream
+d ~= nil and d.status == 'follow' or i
+
+test_run:cmd('stop server replica')
+test_run:cmd("cleanup server replica")
+test_run:cmd("delete server replica")
+i = box.info
+d = i.replication[replica_id].downstream
+d ~= nil and d.system_message ~= nil and d.message ~= nil or i
+
+box.schema.user.revoke('guest', 'replication')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index eb25077d8..4abc93e20 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -11,6 +11,7 @@
     "on_schema_init.test.lua": {},
     "long_row_timeout.test.lua": {},
     "join_without_snap.test.lua": {},
+    "gh-4402-info-errno.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
         "vinyl": {"engine": "vinyl"}
-- 
2.21.0 (Apple Git-122.2)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] Replication status errno
  2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
                   ` (2 preceding siblings ...)
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info Vladislav Shpilevoy
@ 2019-11-05 18:13 ` Konstantin Osipov
  2019-11-21 17:38 ` Kirill Yukhin
  4 siblings, 0 replies; 11+ messages in thread
From: Konstantin Osipov @ 2019-11-05 18:13 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
> The patchset makes box.info.replication*.upstream/downstream show
> errno description for system errors.
> 
> I made that it shows strerror(errno) of the latest system error,
> but probably it would be better to show errno value, and let a
> user to translate it manually using Lua errno module. On the other
> hand my way is simpler for a user when he manually calls box.info
> in a console to check a replication status. That is discussable.

Your approach is fine.


-- 
Konstantin Osipov, Moscow, Russia

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user Vladislav Shpilevoy
@ 2019-11-05 18:15   ` Konstantin Osipov
  2019-11-06 14:48     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: Konstantin Osipov @ 2019-11-05 18:15 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
> +++ b/src/box/user.cc
> @@ -521,8 +521,9 @@ user_find_by_name(const char *name, uint32_t len)
>  	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
>  		return NULL;
>  	struct user *user = user_by_id(uid);
> -	if (user == NULL || user->def->type != SC_USER) {
> -		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
> +	if (user == NULL || user->def->type != SC_USER || uid == BOX_ID_NIL) {
> +		diag_set(ClientError, ER_NO_SUCH_USER,
> +			 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
>  		return NULL;

I would not call user_by_id with BOX_ID_NIL, this is not an error
but it takes an effort to ponder on and realize it's OK to do so.

Otherwise LGTM.


-- 
Konstantin Osipov, Moscow, Russia

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] error: move errno into an error object
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 2/3] error: move errno into an error object Vladislav Shpilevoy
@ 2019-11-05 18:18   ` Konstantin Osipov
  2019-11-06 14:49     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 11+ messages in thread
From: Konstantin Osipov @ 2019-11-05 18:18 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:

syserror is a non-obvious name, I realize you can't use errno
since it is a macro, you could use save_errno, sys_errno,
m_errno, anything that would make it obvious it is a saved errno 
(saved_errno?) instead.

Otherwise, why not have this, LGTM.


-- 
Konstantin Osipov, Moscow, Russia

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info
  2019-11-05 14:59 ` [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info Vladislav Shpilevoy
@ 2019-11-05 18:18   ` Konstantin Osipov
  0 siblings, 0 replies; 11+ messages in thread
From: Konstantin Osipov @ 2019-11-05 18:18 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
> Box.info.replication shows applier/relay's latest error message.
> But it didn't include errno description for system errors, even
> though it was included in the logs. Now box.info shows the errno
> description as well, when possible.
Lgtm


-- 
Konstantin Osipov, Moscow, Russia

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user
  2019-11-05 18:15   ` Konstantin Osipov
@ 2019-11-06 14:48     ` Vladislav Shpilevoy
  0 siblings, 0 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-06 14:48 UTC (permalink / raw)
  To: Konstantin Osipov, tarantool-patches

Hi! Thanks for the review!

On 05/11/2019 21:15, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
>> +++ b/src/box/user.cc
>> @@ -521,8 +521,9 @@ user_find_by_name(const char *name, uint32_t len)
>>  	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
>>  		return NULL;
>>  	struct user *user = user_by_id(uid);
>> -	if (user == NULL || user->def->type != SC_USER) {
>> -		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
>> +	if (user == NULL || user->def->type != SC_USER || uid == BOX_ID_NIL) {
>> +		diag_set(ClientError, ER_NO_SUCH_USER,
>> +			 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
>>  		return NULL;
> 
> I would not call user_by_id with BOX_ID_NIL, this is not an error
> but it takes an effort to ponder on and realize it's OK to do so.
> 
> Otherwise LGTM.
> 
> 

Hm, you are right. I should not search by an invalid ID. Fixed
on the branch.

============================================================================

diff --git a/src/box/user.cc b/src/box/user.cc
index 03b4b2e3b..e2fd32740 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -520,12 +520,14 @@ user_find_by_name(const char *name, uint32_t len)
 	uint32_t uid;
 	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
 		return NULL;
-	struct user *user = user_by_id(uid);
-	if (user == NULL || user->def->type != SC_USER) {
-		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
-		return NULL;
+	if (uid != BOX_ID_NIL) {
+		struct user *user = user_by_id(uid);
+		if (user != NULL && user->def->type == SC_USER)
+			return user;
 	}
-	return user;
+	diag_set(ClientError, ER_NO_SUCH_USER,
+		 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
+	return NULL;
 }

============================================================================

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/3] error: move errno into an error object
  2019-11-05 18:18   ` Konstantin Osipov
@ 2019-11-06 14:49     ` Vladislav Shpilevoy
  0 siblings, 0 replies; 11+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-06 14:49 UTC (permalink / raw)
  To: Konstantin Osipov, tarantool-patches



On 05/11/2019 21:18, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
> 
> syserror is a non-obvious name, I realize you can't use errno
> since it is a macro, you could use save_errno, sys_errno,
> m_errno, anything that would make it obvious it is a saved errno 
> (saved_errno?) instead.

Yes, I tried to use just 'errno', but seems like it can be a macros.

m_errno is rather C++ style. Sys_errno sounds tautological - errno
presumes 'sys'.

I choose 'saved_errno'.

Renamed on the branch.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/3] Replication status errno
  2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
                   ` (3 preceding siblings ...)
  2019-11-05 18:13 ` [Tarantool-patches] [PATCH 0/3] Replication status errno Konstantin Osipov
@ 2019-11-21 17:38 ` Kirill Yukhin
  4 siblings, 0 replies; 11+ messages in thread
From: Kirill Yukhin @ 2019-11-21 17:38 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hello,

On 05 ноя 17:59, Vladislav Shpilevoy wrote:
> The patchset makes box.info.replication*.upstream/downstream show
> errno description for system errors.
> 
> I made that it shows strerror(errno) of the latest system error,
> but probably it would be better to show errno value, and let a
> user to translate it manually using Lua errno module. On the other
> hand my way is simpler for a user when he manually calls box.info
> in a console to check a replication status. That is discussable.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4402-replication-system-error
> Issue: https://github.com/tarantool/tarantool/issues/4402

I've checked your patch set into master.

--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-11-21 17:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 14:59 [Tarantool-patches] [PATCH 0/3] Replication status errno Vladislav Shpilevoy
2019-11-05 14:59 ` [Tarantool-patches] [PATCH 1/3] access: fix invalid error type for not found user Vladislav Shpilevoy
2019-11-05 18:15   ` Konstantin Osipov
2019-11-06 14:48     ` Vladislav Shpilevoy
2019-11-05 14:59 ` [Tarantool-patches] [PATCH 2/3] error: move errno into an error object Vladislav Shpilevoy
2019-11-05 18:18   ` Konstantin Osipov
2019-11-06 14:49     ` Vladislav Shpilevoy
2019-11-05 14:59 ` [Tarantool-patches] [PATCH 3/3] replication: show errno in replication info Vladislav Shpilevoy
2019-11-05 18:18   ` Konstantin Osipov
2019-11-05 18:13 ` [Tarantool-patches] [PATCH 0/3] Replication status errno Konstantin Osipov
2019-11-21 17:38 ` Kirill Yukhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox