[Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Cyrill Gorcunov via Tarantool-patches]

We've been notified that if tarantool instance is running for
long time slice the logger start to print weird lines such as

 | main/-244760339/cartridge.failover.task I> Instance state changed

where fiber's ID represented as a negative number. Eventually we
discovered a few issues

 - a bunch of say_x called with wrong specificators but compiler
   simply ignored it
 - fiber IDs are wrapping and this is inconvenient
 - CFORMAT simply doesn't work

In the series we fix all these issues. Note that using simplified
form for unsigned arguments such as

	"%llu", (long long)(uint64_t)

is pretty fine becase signed/unsiged values are counterpart of
each other guaranteed by standart.

issue https://github.com/tarantool/tarantool/issues/5846
branch gorcunov/gh-5846-fid-name-3

Cyrill Gorcunov (10):
  fiber: use uint64_t for fiber IDs
  popen: fix say_x format arguments
  raft: fix say_x arguments
  box/error: fix argument for CustomError
  xlog: fix say_x format
  box/vynil: fix say_x format
  txn: fix say_x format
  limbo: fix say_x format
  wal: fix say_x format
  say: fix CFORMAT specification

 changelogs/unreleased/gh-5846-cformat.md  | 12 +++++++++
 changelogs/unreleased/gh-5846-fiber-id.md |  4 +++
 src/box/error.cc                          |  2 +-
 src/box/txn.c                             |  3 ++-
 src/box/txn_limbo.c                       |  2 +-
 src/box/vy_scheduler.c                    |  2 +-
 src/box/wal.c                             | 17 ++++++------
 src/box/xlog.c                            |  4 +--
 src/lib/core/fiber.c                      | 30 ++++++++++-----------
 src/lib/core/fiber.h                      |  8 +++---
 src/lib/core/popen.c                      |  4 +--
 src/lib/core/say.c                        |  8 +++---
 src/lib/core/say.h                        |  2 +-
 src/lib/raft/raft.c                       |  5 ++--
 src/lua/fiber.c                           | 33 ++++++++++++-----------
 15 files changed, 79 insertions(+), 57 deletions(-)
 create mode 100644 changelogs/unreleased/gh-5846-cformat.md
 create mode 100644 changelogs/unreleased/gh-5846-fiber-id.md


base-commit: 4500547d7a177c0b49bff35c922286b4f21c8719
-- 
2.30.2

[Tarantool-patches] [PATCH v3 01/10] fiber: use uint64_t for fiber IDs

[Cyrill Gorcunov via Tarantool-patches]

Historically we use uint32_t for fiber IDs. And these IDs were
wrapping in time, especially if instance is running for a long
period. Strictly speaking this is not very convenient because if
some external tool gonna track fibers by their IDs it might get
confused (or miss) by IDs wrapping.

Lets rather switch to wide integers and fixup outputs (such as
snprintf callers, Lua's fid reports and etc). This allows us to
simplify code a bit and forget about IDs wrapping at all.

Same time wrong output specificators resulted in weird informal
lines

 | main/-244760339/cartridge.failover.task I> Instance state changed

Thus changing IDs type forced us to review all printouts and fix
formatting to not confuse users.

Closes #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 changelogs/unreleased/gh-5846-cformat.md  | 12 +++++++++
 changelogs/unreleased/gh-5846-fiber-id.md |  4 +++
 src/lib/core/fiber.c                      | 30 ++++++++++-----------
 src/lib/core/fiber.h                      |  8 +++---
 src/lib/core/say.c                        |  8 +++---
 src/lua/fiber.c                           | 33 ++++++++++++-----------
 6 files changed, 57 insertions(+), 38 deletions(-)
 create mode 100644 changelogs/unreleased/gh-5846-cformat.md
 create mode 100644 changelogs/unreleased/gh-5846-fiber-id.md

diff --git a/changelogs/unreleased/gh-5846-cformat.md b/changelogs/unreleased/gh-5846-cformat.md
new file mode 100644
index 000000000..027de35d7
--- /dev/null
+++ b/changelogs/unreleased/gh-5846-cformat.md
@@ -0,0 +1,12 @@
+## bugfix/core
+ * Fixed wrong type specificator when printing fiber state
+   change which lead to negative fiber's ID logging.
+
+   For exmaple
+   ```
+   main/-244760339/cartridge.failover.task I> Instance state changed
+   ```
+   instead of proper
+   ```
+   main/4050206957/cartridge.failover.task I> Instance state changed
+   ```
diff --git a/changelogs/unreleased/gh-5846-fiber-id.md b/changelogs/unreleased/gh-5846-fiber-id.md
new file mode 100644
index 000000000..b645da849
--- /dev/null
+++ b/changelogs/unreleased/gh-5846-fiber-id.md
@@ -0,0 +1,4 @@
+## feature/core
+ * Fiber IDs are switched to monotonically increasing unsigned 8 byte
+   integers so that there won't be IDs wrapping anymore. This allows
+   to detect fiber's precedence by their IDs if needed.
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 196dffe26..216f8be5d 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -823,28 +823,28 @@ fiber_schedule_idle(ev_loop *loop, ev_idle *watcher,
 
 
 struct fiber *
-fiber_find(uint32_t fid)
+fiber_find(uint64_t fid)
 {
-	struct mh_i32ptr_t *fiber_registry = cord()->fiber_registry;
-	mh_int_t k = mh_i32ptr_find(fiber_registry, fid, NULL);
+	struct mh_i64ptr_t *fiber_registry = cord()->fiber_registry;
+	mh_int_t k = mh_i64ptr_find(fiber_registry, fid, NULL);
 
 	if (k == mh_end(fiber_registry))
 		return NULL;
-	return (struct fiber *) mh_i32ptr_node(fiber_registry, k)->val;
+	return mh_i64ptr_node(fiber_registry, k)->val;
 }
 
 static void
 register_fid(struct fiber *fiber)
 {
-	struct mh_i32ptr_node_t node = { fiber->fid, fiber };
-	mh_i32ptr_put(cord()->fiber_registry, &node, NULL, NULL);
+	struct mh_i64ptr_node_t node = { fiber->fid, fiber };
+	mh_i64ptr_put(cord()->fiber_registry, &node, NULL, NULL);
 }
 
 static void
 unregister_fid(struct fiber *fiber)
 {
-	struct mh_i32ptr_node_t node = { fiber->fid, NULL };
-	mh_i32ptr_remove(cord()->fiber_registry, &node, NULL);
+	struct mh_i64ptr_node_t node = { fiber->fid, NULL };
+	mh_i64ptr_remove(cord()->fiber_registry, &node, NULL);
 }
 
 struct fiber *
@@ -1257,13 +1257,13 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr,
 	}
 
 	fiber->f = f;
-	/* Excluding reserved range */
-	if (++cord->max_fid < FIBER_ID_MAX_RESERVED)
-		cord->max_fid = FIBER_ID_MAX_RESERVED + 1;
-	fiber->fid = cord->max_fid;
+	fiber->fid = cord->next_fid;
 	fiber_set_name(fiber, name);
 	register_fid(fiber);
 
+	cord->next_fid++;
+	assert(cord->next_fid > FIBER_ID_MAX_RESERVED);
+
 	return fiber;
 
 }
@@ -1449,7 +1449,7 @@ cord_create(struct cord *cord, const char *name)
 	rlist_create(&cord->alive);
 	rlist_create(&cord->ready);
 	rlist_create(&cord->dead);
-	cord->fiber_registry = mh_i32ptr_new();
+	cord->fiber_registry = mh_i64ptr_new();
 
 	/* sched fiber is not present in alive/ready/dead list. */
 	cord->sched.fid = FIBER_ID_SCHED;
@@ -1461,7 +1461,7 @@ cord_create(struct cord *cord, const char *name)
 	cord->fiber = &cord->sched;
 	cord->sched.flags |= FIBER_IS_RUNNING;
 
-	cord->max_fid = FIBER_ID_MAX_RESERVED;
+	cord->next_fid = FIBER_ID_MAX_RESERVED + 1;
 	/*
 	 * No need to start this event since it's only used for
 	 * ev_feed_event(). Saves a few cycles on every
@@ -1502,7 +1502,7 @@ cord_destroy(struct cord *cord)
 	/* Only clean up if initialized. */
 	if (cord->fiber_registry) {
 		fiber_destroy_all(cord);
-		mh_i32ptr_delete(cord->fiber_registry);
+		mh_i64ptr_delete(cord->fiber_registry);
 	}
 	region_destroy(&cord->sched.gc);
 	diag_destroy(&cord->sched.diag);
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 586bc9433..8f4e14796 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -550,7 +550,7 @@ struct fiber {
 	/** Number of context switches. */
 	int csw;
 	/** Fiber id. */
-	uint32_t fid;
+	uint64_t fid;
 	/** Fiber flags */
 	uint32_t flags;
 #if ENABLE_FIBER_TOP
@@ -661,7 +661,7 @@ struct cord {
 	 * Every new fiber gets a new monotonic id. Ids 0 - 100 are
 	 * reserved.
 	 */
-	uint32_t max_fid;
+	uint64_t next_fid;
 #if ENABLE_FIBER_TOP
 	struct clock_stat clock_stat;
 	struct cpu_stat cpu_stat;
@@ -669,7 +669,7 @@ struct cord {
 	pthread_t id;
 	const struct cord_on_exit *on_exit;
 	/** A helper hash to map id -> fiber. */
-	struct mh_i32ptr_t *fiber_registry;
+	struct mh_i64ptr_t *fiber_registry;
 	/** All fibers */
 	struct rlist alive;
 	/** Fibers, ready for execution */
@@ -828,7 +828,7 @@ void
 fiber_call(struct fiber *callee);
 
 struct fiber *
-fiber_find(uint32_t fid);
+fiber_find(uint64_t fid);
 
 void
 fiber_schedule_cb(ev_loop * /* loop */, ev_watcher *watcher, int revents);
diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index cbd10e107..5307767b5 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -792,8 +792,9 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
 	if (cord) {
 		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
 		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
-			SNPRINT(total, snprintf, buf, len, "/%i/%s",
-				fiber()->fid, fiber_name(fiber()));
+			SNPRINT(total, snprintf, buf, len, "/%lld/%s",
+				(long long)fiber()->fid,
+				fiber_name(fiber()));
 		}
 	}
 
@@ -918,7 +919,8 @@ say_format_json(struct log *log, char *buf, int len, int level, const char *file
 		SNPRINT(total, snprintf, buf, len, "\"");
 		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
 			SNPRINT(total, snprintf, buf, len,
-				", \"fiber_id\": %i, ", fiber()->fid);
+				", \"fiber_id\": %llu, ",
+				(long long)fiber()->fid);
 			SNPRINT(total, snprintf, buf, len,
 				"\"fiber_name\": \"");
 			SNPRINT(total, json_escape, buf, len,
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 02ec3d158..c792bf385 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -113,7 +113,7 @@ lbox_create_weak_table(struct lua_State *L, const char *name)
  * Push a userdata for the given fiber onto Lua stack.
  */
 static void
-lbox_pushfiber(struct lua_State *L, int fid)
+lbox_pushfiber(struct lua_State *L, uint64_t fid)
 {
 	/*
 	 * Use 'memoize'  pattern and keep a single userdata for
@@ -131,22 +131,22 @@ lbox_pushfiber(struct lua_State *L, int fid)
 		lbox_create_weak_table(L, "memoize");
 	}
 	/* Find out whether the fiber is  already in the memoize table. */
-	lua_pushinteger(L, fid);
+	luaL_pushuint64(L, fid);
 	lua_gettable(L, -2);
 	if (lua_isnil(L, -1)) {
 		/* no userdata for fiber created so far */
 		/* pop the nil */
 		lua_pop(L, 1);
 		/* push the key back */
-		lua_pushinteger(L, fid);
+		luaL_pushuint64(L, fid);
 		/* create a new userdata */
-		int *ptr = (int *) lua_newuserdata(L, sizeof(int));
+		uint64_t *ptr = lua_newuserdata(L, sizeof(*ptr));
 		*ptr = fid;
 		luaL_getmetatable(L, fiberlib_name);
 		lua_setmetatable(L, -2);
 		/* memoize it */
 		lua_settable(L, -3);
-		lua_pushinteger(L, fid);
+		luaL_pushuint64(L, fid);
 		/* get it back */
 		lua_gettable(L, -2);
 	}
@@ -155,11 +155,11 @@ lbox_pushfiber(struct lua_State *L, int fid)
 static struct fiber *
 lbox_checkfiber(struct lua_State *L, int index)
 {
-	uint32_t fid;
+	uint64_t fid;
 	if (lua_type(L, index) == LUA_TNUMBER) {
-		fid = lua_tonumber(L, index);
+		fid = luaL_touint64(L, index);
 	} else {
-		fid = *(uint32_t *) luaL_checkudata(L, index, fiberlib_name);
+		fid = *(uint64_t *) luaL_checkudata(L, index, fiberlib_name);
 	}
 	struct fiber *f = fiber_find(fid);
 	if (f == NULL)
@@ -170,12 +170,12 @@ lbox_checkfiber(struct lua_State *L, int index)
 static int
 lbox_fiber_id(struct lua_State *L)
 {
-	uint32_t fid;
+	uint64_t fid;
 	if (lua_gettop(L)  == 0)
 		fid = fiber()->fid;
 	else
-		fid = *(uint32_t *) luaL_checkudata(L, 1, fiberlib_name);
-	lua_pushinteger(L, fid);
+		fid = *(uint64_t *) luaL_checkudata(L, 1, fiberlib_name);
+	luaL_pushuint64(L, fid);
 	return 1;
 }
 
@@ -275,7 +275,7 @@ lbox_fiber_statof(struct fiber *f, void *cb_ctx, bool backtrace)
 	lua_settable(L, -3);
 
 	lua_pushstring(L, "fid");
-	lua_pushnumber(L, f->fid);
+	luaL_pushuint64(L, f->fid);
 	lua_settable(L, -3);
 
 	lua_pushstring(L, "csw");
@@ -539,7 +539,7 @@ lbox_fiber_status(struct lua_State *L)
 {
 	struct fiber *f;
 	if (lua_gettop(L)) {
-		uint32_t fid = *(uint32_t *)
+		uint64_t fid = *(uint64_t *)
 			luaL_checkudata(L, 1, fiberlib_name);
 		f = fiber_find(fid);
 	} else {
@@ -704,7 +704,7 @@ lbox_fiber_find(struct lua_State *L)
 {
 	if (lua_gettop(L) != 1)
 		luaL_error(L, "fiber.find(id): bad arguments");
-	int fid = lua_tonumber(L, -1);
+	uint64_t fid = luaL_touint64(L, -1);
 	struct fiber *f = fiber_find(fid);
 	if (f)
 		lbox_pushfiber(L, f->fid);
@@ -736,7 +736,7 @@ lbox_fiber_serialize(struct lua_State *L)
 {
 	struct fiber *f = lbox_checkfiber(L, 1);
 	lua_createtable(L, 0, 1);
-	lua_pushinteger(L, f->fid);
+	luaL_pushuint64(L, f->fid);
 	lua_setfield(L, -2, "id");
 	lua_pushstring(L, fiber_name(f));
 	lua_setfield(L, -2, "name");
@@ -750,7 +750,8 @@ lbox_fiber_tostring(struct lua_State *L)
 {
 	char buf[20];
 	struct fiber *f = lbox_checkfiber(L, 1);
-	snprintf(buf, sizeof(buf), "fiber: %d", f->fid);
+	snprintf(buf, sizeof(buf), "fiber: %llu",
+		 (long long)f->fid);
 	lua_pushstring(L, buf);
 	return 1;
 }
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3 01/10] fiber: use uint64_t for fiber IDs

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

See 5 comments below.

> diff --git a/changelogs/unreleased/gh-5846-cformat.md b/changelogs/unreleased/gh-5846-cformat.md
> new file mode 100644
> index 000000000..027de35d7
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5846-cformat.md
> @@ -0,0 +1,12 @@
> +## bugfix/core
> + * Fixed wrong type specificator when printing fiber state
> +   change which lead to negative fiber's ID logging.
> +
> +   For exmaple

1. exmaple -> example.

> +   ```
> +   main/-244760339/cartridge.failover.task I> Instance state changed
> +   ```
> +   instead of proper
> +   ```
> +   main/4050206957/cartridge.failover.task I> Instance state changed
> +   ```

2. You need to give a link to the issue here in a form 'gh-####'.

> diff --git a/changelogs/unreleased/gh-5846-fiber-id.md b/changelogs/unreleased/gh-5846-fiber-id.md
> new file mode 100644
> index 000000000..b645da849
> --- /dev/null
> +++ b/changelogs/unreleased/gh-5846-fiber-id.md
> @@ -0,0 +1,4 @@
> +## feature/core
> + * Fiber IDs are switched to monotonically increasing unsigned 8 byte
> +   integers so that there won't be IDs wrapping anymore. This allows
> +   to detect fiber's precedence by their IDs if needed.

3. This is a bugfix, not a feature. Please, keep it in the bugfix changelog
file.

> diff --git a/src/lib/core/say.c b/src/lib/core/say.c
> index cbd10e107..5307767b5 100644
> --- a/src/lib/core/say.c
> +++ b/src/lib/core/say.c
> @@ -792,8 +792,9 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
>  	if (cord) {
>  		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
>  		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
> -			SNPRINT(total, snprintf, buf, len, "/%i/%s",
> -				fiber()->fid, fiber_name(fiber()));
> +			SNPRINT(total, snprintf, buf, len, "/%lld/%s",

4. This is llu, not lld, is it? Because FID is uint64_t. I see you
even use llu in the other places for FID.

Lets be consistent. Either you use int64_t fid and lld in the
formats, or you use uint64_t and llu in the formats.

> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 02ec3d158..c792bf385 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -155,11 +155,11 @@ lbox_pushfiber(struct lua_State *L, int fid)
>  static struct fiber *
>  lbox_checkfiber(struct lua_State *L, int index)
>  {
> -	uint32_t fid;
> +	uint64_t fid;
>  	if (lua_type(L, index) == LUA_TNUMBER) {
> -		fid = lua_tonumber(L, index);
> +		fid = luaL_touint64(L, index);
>  	} else {
> -		fid = *(uint32_t *) luaL_checkudata(L, index, fiberlib_name);
> +		fid = *(uint64_t *) luaL_checkudata(L, index, fiberlib_name);

5. Please, drop the whitespace after the type cast alongside. The same in
the block below.

>  	}
>  	struct fiber *f = fiber_find(fid);
>  	if (f == NULL)
> @@ -170,12 +170,12 @@ lbox_checkfiber(struct lua_State *L, int index)
>  static int
>  lbox_fiber_id(struct lua_State *L)
>  {
> -	uint32_t fid;
> +	uint64_t fid;
>  	if (lua_gettop(L)  == 0)
>  		fid = fiber()->fid;
>  	else
> -		fid = *(uint32_t *) luaL_checkudata(L, 1, fiberlib_name);
> -	lua_pushinteger(L, fid);
> +		fid = *(uint64_t *) luaL_checkudata(L, 1, fiberlib_name);
> +	luaL_pushuint64(L, fid);
>  	return 1;
>  }

Re: [Tarantool-patches] [PATCH v3 01/10] fiber: use uint64_t for fiber IDs

[Cyrill Gorcunov via Tarantool-patches]

On Mon, May 10, 2021 at 08:40:19PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 5 comments below.

Thanks! A fixup on top.
---
 changelogs/unreleased/gh-5846-cformat.md  | 6 +++---
 changelogs/unreleased/gh-5846-fiber-id.md | 4 ++--
 src/lib/core/say.c                        | 2 +-
 src/lua/fiber.c                           | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/changelogs/unreleased/gh-5846-cformat.md b/changelogs/unreleased/gh-5846-cformat.md
index 027de35d7..6ebbde126 100644
--- a/changelogs/unreleased/gh-5846-cformat.md
+++ b/changelogs/unreleased/gh-5846-cformat.md
@@ -1,8 +1,8 @@
 ## bugfix/core
- * Fixed wrong type specificator when printing fiber state
-   change which lead to negative fiber's ID logging.
+ * Fixed wrong type specification when printing fiber state
+   change which lead to negative fiber's ID logging (gh-5846).
 
-   For exmaple
+   For example
    ```
    main/-244760339/cartridge.failover.task I> Instance state changed
    ```
diff --git a/changelogs/unreleased/gh-5846-fiber-id.md b/changelogs/unreleased/gh-5846-fiber-id.md
index b645da849..aa8af536f 100644
--- a/changelogs/unreleased/gh-5846-fiber-id.md
+++ b/changelogs/unreleased/gh-5846-fiber-id.md
@@ -1,4 +1,4 @@
-## feature/core
+## bugfix/core
  * Fiber IDs are switched to monotonically increasing unsigned 8 byte
    integers so that there won't be IDs wrapping anymore. This allows
-   to detect fiber's precedence by their IDs if needed.
+   to detect fiber's precedence by their IDs if needed (gh-5846).
diff --git a/src/lib/core/say.c b/src/lib/core/say.c
index 5307767b5..e0738de85 100644
--- a/src/lib/core/say.c
+++ b/src/lib/core/say.c
@@ -792,7 +792,7 @@ say_format_plain_tail(char *buf, int len, int level, const char *filename,
 	if (cord) {
 		SNPRINT(total, snprintf, buf, len, " %s", cord->name);
 		if (fiber() && fiber()->fid != FIBER_ID_SCHED) {
-			SNPRINT(total, snprintf, buf, len, "/%lld/%s",
+			SNPRINT(total, snprintf, buf, len, "/%llu/%s",
 				(long long)fiber()->fid,
 				fiber_name(fiber()));
 		}
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index c792bf385..eb6bcc612 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -159,7 +159,7 @@ lbox_checkfiber(struct lua_State *L, int index)
 	if (lua_type(L, index) == LUA_TNUMBER) {
 		fid = luaL_touint64(L, index);
 	} else {
-		fid = *(uint64_t *) luaL_checkudata(L, index, fiberlib_name);
+		fid = *(uint64_t *)luaL_checkudata(L, index, fiberlib_name);
 	}
 	struct fiber *f = fiber_find(fid);
 	if (f == NULL)
@@ -174,7 +174,7 @@ lbox_fiber_id(struct lua_State *L)
 	if (lua_gettop(L)  == 0)
 		fid = fiber()->fid;
 	else
-		fid = *(uint64_t *) luaL_checkudata(L, 1, fiberlib_name);
+		fid = *(uint64_t *)luaL_checkudata(L, 1, fiberlib_name);
 	luaL_pushuint64(L, fid);
 	return 1;
 }
-- 
2.31.1

[Tarantool-patches] [PATCH v3 02/10] popen: fix say_x format arguments

[Cyrill Gorcunov via Tarantool-patches]

 - drop redundant %p
 - use %zd for size_t

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/popen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/popen.c b/src/lib/core/popen.c
index 3b5fd1062..99a3d3cbd 100644
--- a/src/lib/core/popen.c
+++ b/src/lib/core/popen.c
@@ -240,7 +240,7 @@ handle_new(struct popen_opts *opts)
 static inline void
 handle_free(struct popen_handle *handle)
 {
-	say_debug("popen: handle %p free %p", handle);
+	say_debug("popen: handle free %p", handle);
 	free(handle);
 }
 
@@ -533,7 +533,7 @@ popen_shutdown(struct popen_handle *handle, unsigned int flags)
 		if (handle->ios[idx].fd < 0)
 			continue;
 
-		say_debug("popen: %d: shutdown idx [%s:%d] fd %s",
+		say_debug("popen: %d: shutdown idx [%s:%zd] fd %d",
 			  handle->pid, stdX_str(idx), idx,
 			  handle->ios[idx].fd);
 		coio_close_io(loop(), &handle->ios[idx]);
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3 02/10] popen: fix say_x format arguments

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for working on this!

On 04.05.2021 17:58, Cyrill Gorcunov via Tarantool-patches wrote:
>  - drop redundant %p
>  - use %zd for size_t
> 
> Part-of #5846

The issue was closed in the previous commit, newer commits can't be
a part of it. Besides, if you open the ticket and read the description -
it is not related to this commit and all the next ones. The ticket was
only about fiber IDs, not about all the formats.

You might need to either omit this link, or make it 'Follow-up #5846'.

Re: [Tarantool-patches] [PATCH v3 02/10] popen: fix say_x format arguments

[Cyrill Gorcunov via Tarantool-patches]

On Mon, May 10, 2021 at 08:40:23PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> On 04.05.2021 17:58, Cyrill Gorcunov via Tarantool-patches wrote:
> >  - drop redundant %p
> >  - use %zd for size_t
> > 
> > Part-of #5846
> 
> The issue was closed in the previous commit, newer commits can't be
> a part of it. Besides, if you open the ticket and read the description -
> it is not related to this commit and all the next ones. The ticket was
> only about fiber IDs, not about all the formats.
> 
> You might need to either omit this link, or make it 'Follow-up #5846'.

Lets use Follow-up then

[Tarantool-patches] [PATCH v3 03/10] raft: fix say_x arguments

[Cyrill Gorcunov via Tarantool-patches]

Use explicit "%llu" for uint64_t type.

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/raft/raft.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index 46a30149f..247c7a71a 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -322,7 +322,8 @@ raft_process_msg(struct raft *raft, const struct raft_msg *req, uint32_t source)
 	/* Outdated request. */
 	if (req->term < raft->volatile_term) {
 		say_info("RAFT: the message is ignored due to outdated term - "
-			 "current term is %u", raft->volatile_term);
+			 "current term is %llu",
+			 (long long)raft->volatile_term);
 		return 0;
 	}
 
@@ -653,7 +654,7 @@ raft_sm_become_candidate(struct raft *raft)
 static void
 raft_sm_schedule_new_term(struct raft *raft, uint64_t new_term)
 {
-	say_info("RAFT: bump term to %llu, follow", new_term);
+	say_info("RAFT: bump term to %llu, follow", (long long)new_term);
 	assert(new_term > raft->volatile_term);
 	assert(raft->volatile_term >= raft->term);
 	raft->volatile_term = new_term;
-- 
2.30.2

[Tarantool-patches] [PATCH v3 04/10] box/error: fix argument for CustomError

[Cyrill Gorcunov via Tarantool-patches]

Drop redundant "%s" argument.

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/error.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/error.cc b/src/box/error.cc
index f3b4ffe86..225b377b7 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -345,7 +345,7 @@ CustomError::CustomError(const char *file, unsigned int line,
 void
 CustomError::log() const
 {
-	say_file_line(S_ERROR, file, line, errmsg, "%s",
+	say_file_line(S_ERROR, file, line, errmsg,
 		      "Custom type %s", m_custom_type);
 }
 
-- 
2.30.2

[Tarantool-patches] [PATCH v3 05/10] xlog: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

 - The string length specificator expects length as
   an integer value
 - Use matching quoting for key print out

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index d9e1117e1..7abbb90c5 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -314,8 +314,8 @@ xlog_meta_parse(struct xlog_meta *meta, const char **data,
 			/*
 			 * Unknown key
 			 */
-			say_warn("Unknown meta item: `%.*s'", key_end - key,
-				 key);
+			say_warn("Unknown meta item: '%.*s'",
+				 (int)(key_end - key), key);
 		}
 	}
 	*data = end + 1; /* skip the last trailing \n of \n\n sequence */
-- 
2.30.2

[Tarantool-patches] [PATCH v3 06/10] box/vynil: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

Drop redundant "%s" from format.

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/vy_scheduler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 7d8324a52..917b75f93 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1714,7 +1714,7 @@ vy_task_compaction_new(struct vy_scheduler *scheduler, struct vy_worker *worker,
 	vy_task_delete(task);
 err_task:
 	diag_log();
-	say_error("%s: could not start compacting range %s: %s",
+	say_error("%s: could not start compacting range %s",
 		  vy_lsm_name(lsm), vy_range_str(range));
 	return -1;
 }
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3 06/10] box/vynil: fix say_x format

[Vladislav Shpilevoy via Tarantool-patches]

I appreciate the work you did here!

In the commit message: vynil -> vinyl.

Re: [Tarantool-patches] [PATCH v3 06/10] box/vynil: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

On Mon, May 10, 2021 at 08:40:27PM +0200, Vladislav Shpilevoy wrote:
> I appreciate the work you did here!
> 
> In the commit message: vynil -> vinyl.

Updated, thanks!

[Tarantool-patches] [PATCH v3 07/10] txn: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

Use explicit conversion for 64 bit number.

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 6d861338f..1d42c9113 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -551,7 +551,8 @@ txn_on_journal_write(struct journal_entry *entry)
 		int n_rows = txn->n_new_rows + txn->n_applier_rows;
 		say_warn_ratelimited("too long WAL write: %d rows at LSN %lld: "
 				     "%.3f sec", n_rows,
-				     txn->signature - n_rows + 1, delta);
+				     (long long)(txn->signature - n_rows + 1),
+				     delta);
 	}
 	if (txn_has_flag(txn, TXN_HAS_TRIGGERS))
 		txn_run_wal_write_triggers(txn);
-- 
2.30.2

[Tarantool-patches] [PATCH v3 08/10] limbo: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

Use explicit cast for 64 bit number.

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn_limbo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 87b02456b..f287369a2 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -347,7 +347,7 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, uint16_t type, int64_t lsn,
 		 * Or retry automatically with some period.
 		 */
 		panic("Could not write a synchro request to WAL: "
-		      "lsn = %lld, type = %s\n", lsn,
+		      "lsn = %lld, type = %s\n", (long long)lsn,
 		      iproto_type_name(type));
 	}
 }
-- 
2.30.2

[Tarantool-patches] [PATCH v3 09/10] wal: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

 - vclock_get returns int64_t
 - vclock_sum returns int64_t

Part-of #5846

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/wal.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/src/box/wal.c b/src/box/wal.c
index 6468df884..fff530a38 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -1009,13 +1009,14 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
 			int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);
 			if (diff <= vclock_get(vclock_diff,
 					       (*row)->replica_id)) {
+				int64_t confirmed_lsn =
+					vclock_get(base, (*row)->replica_id) +
+					vclock_get(vclock_diff, (*row)->replica_id);
 				say_crit("Attempt to write a broken LSN to WAL:"
-					 " replica id: %d, confirmed lsn: %d,"
-					 " new lsn %d", (*row)->replica_id,
-					 vclock_get(base, (*row)->replica_id) +
-					 vclock_get(vclock_diff,
-						    (*row)->replica_id),
-						    (*row)->lsn);
+					 " replica id: %d, confirmed lsn: %lld,"
+					 " new lsn %lld", (*row)->replica_id,
+					 (long long)confirmed_lsn,
+					 (long long)(*row)->lsn);
 				assert(false);
 			} else {
 				vclock_follow(vclock_diff, (*row)->replica_id, diff);
@@ -1254,9 +1255,9 @@ wal_write_async(struct journal *journal, struct journal_entry *entry)
 		 * commit a transaction which has seen changes
 		 * that will be rolled back.
 		 */
-		say_error("Aborting transaction %llu during "
+		say_error("Aborting transaction %lld during "
 			  "cascading rollback",
-			  vclock_sum(&writer->vclock));
+			  (long long)vclock_sum(&writer->vclock));
 		goto fail;
 	}
 
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3 09/10] wal: fix say_x format

[Vladislav Shpilevoy via Tarantool-patches]

Good job on the patch!

> diff --git a/src/box/wal.c b/src/box/wal.c
> index 6468df884..fff530a38 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -1009,13 +1009,14 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>  			int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);
>  			if (diff <= vclock_get(vclock_diff,
>  					       (*row)->replica_id)) {
> +				int64_t confirmed_lsn =
> +					vclock_get(base, (*row)->replica_id) +
> +					vclock_get(vclock_diff, (*row)->replica_id);
>  				say_crit("Attempt to write a broken LSN to WAL:"
> -					 " replica id: %d, confirmed lsn: %d,"
> -					 " new lsn %d", (*row)->replica_id,
> -					 vclock_get(base, (*row)->replica_id) +
> -					 vclock_get(vclock_diff,
> -						    (*row)->replica_id),
> -						    (*row)->lsn);
> +					 " replica id: %d, confirmed lsn: %lld,"

Replica ID should be %u, it is unsigned.

> +					 " new lsn %lld", (*row)->replica_id,
> +					 (long long)confirmed_lsn,
> +					 (long long)(*row)->lsn);

Re: [Tarantool-patches] [PATCH v3 09/10] wal: fix say_x format

[Cyrill Gorcunov via Tarantool-patches]

On Mon, May 10, 2021 at 08:40:32PM +0200, Vladislav Shpilevoy wrote:
> Replica ID should be %u, it is unsigned.
> 

Force pushed
---
 src/box/wal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/wal.c b/src/box/wal.c
index fff530a38..63ff5584a 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -1013,7 +1013,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
 					vclock_get(base, (*row)->replica_id) +
 					vclock_get(vclock_diff, (*row)->replica_id);
 				say_crit("Attempt to write a broken LSN to WAL:"
-					 " replica id: %d, confirmed lsn: %lld,"
+					 " replica id: %u, confirmed lsn: %lld,"
 					 " new lsn %lld", (*row)->replica_id,
 					 (long long)confirmed_lsn,
 					 (long long)(*row)->lsn);
-- 
2.31.1

[Tarantool-patches] [PATCH v3 10/10] say: fix CFORMAT specification

[Cyrill Gorcunov via Tarantool-patches]

The position of first argument to check from is 6, not 0.
Looks like our tests with CFORMAT was simply not working
since commit 7d12d66e67a1ce843a2712980f4d3ba04bc0d4f2.

Lets turn them all back.

Part-of #5846

Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/say.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib/core/say.h b/src/lib/core/say.h
index fe8251e4c..e1fec8c60 100644
--- a/src/lib/core/say.h
+++ b/src/lib/core/say.h
@@ -287,7 +287,7 @@ typedef void (*sayfunc_t)(int, const char *, int, const char *,
 		    const char *, ...);
 
 /** Internal function used to implement say() macros */
-CFORMAT(printf, 5, 0) extern sayfunc_t _say;
+CFORMAT(printf, 5, 6) extern sayfunc_t _say;
 
 /**
  * Format and print a message to Tarantool log file.
-- 
2.30.2

Re: [Tarantool-patches] [PATCH v3 10/10] say: fix CFORMAT specification

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for the patch!

On 04.05.2021 17:58, Cyrill Gorcunov wrote:
> The position of first argument to check from is 6, not 0.
> Looks like our tests with CFORMAT was simply not working
> since commit 7d12d66e67a1ce843a2712980f4d3ba04bc0d4f2.

Please, in addition to the hash add the commit title in
parentheses.
https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

> Lets turn them all back.
> 
> Part-of #5846

Re: [Tarantool-patches] [PATCH v3 10/10] say: fix CFORMAT specification

[Cyrill Gorcunov via Tarantool-patches]

On Mon, May 10, 2021 at 08:40:34PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 04.05.2021 17:58, Cyrill Gorcunov wrote:
> > The position of first argument to check from is 6, not 0.
> > Looks like our tests with CFORMAT was simply not working
> > since commit 7d12d66e67a1ce843a2712980f4d3ba04bc0d4f2.
> 
> Please, in addition to the hash add the commit title in
> parentheses.
> https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message

Updated, thanks! I force pushed all updates.

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Good job on the fixes, really!

Although function lbox_fiber_statof() still uses lua_pushinteger():

====================
static int
lbox_fiber_statof(struct fiber *f, void *cb_ctx, bool backtrace)
{
	struct lua_State *L = (struct lua_State *) cb_ctx;

	lua_pushinteger(L, f->fid); <<<<<<<<
	lua_newtable(L);
====================

And cord_on_yield() still uses %d for fid:

====================
		lua_pushfstring(L, "fiber %d is switched while running the"
				" compiled code (it's likely a function with"
				" a yield underneath called via LuaJIT FFI)",
				fiber()->fid);
=========
		lua_pushfstring(L, "fiber %d is switched while running GC"
				" finalizer (i.e. __gc metamethod)",
				fiber()->fid);
====================

Please, fix these places too.

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Cyrill Gorcunov via Tarantool-patches]

On Tue, May 11, 2021 at 10:13:01PM +0200, Vladislav Shpilevoy wrote:
> Hi! Good job on the fixes, really!
> 
> Although function lbox_fiber_statof() still uses lua_pushinteger():
> 

Thanks a huge for catching it, Vlad. Here is an update. Squashed
to patch 1 "fiber: use uint64_t for fiber IDs" and force pushed.
---
 src/lua/fiber.c | 2 +-
 src/lua/utils.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index eb6bcc612..0d28fce8c 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -267,7 +267,7 @@ lbox_fiber_statof(struct fiber *f, void *cb_ctx, bool backtrace)
 {
 	struct lua_State *L = (struct lua_State *) cb_ctx;
 
-	lua_pushinteger(L, f->fid);
+	luaL_pushuint64(L, f->fid);
 	lua_newtable(L);
 
 	lua_pushliteral(L, "name");
diff --git a/src/lua/utils.c b/src/lua/utils.c
index b5a6ca5b7..0fbe700fc 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1348,10 +1348,10 @@ void cord_on_yield(void)
 		 */
 		struct lua_State *L = fiber()->storage.lua.stack;
 		assert(L != NULL);
-		lua_pushfstring(L, "fiber %d is switched while running the"
+		lua_pushfstring(L, "fiber %llu is switched while running the"
 				" compiled code (it's likely a function with"
 				" a yield underneath called via LuaJIT FFI)",
-				fiber()->fid);
+				(long long)fiber()->fid);
 		if (g->panic)
 			g->panic(L);
 		exit(EXIT_FAILURE);
-- 
2.31.1

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Cyrill Gorcunov via Tarantool-patches]

On Wed, May 12, 2021 at 12:15:39AM +0300, Cyrill Gorcunov wrote:
> On Tue, May 11, 2021 at 10:13:01PM +0200, Vladislav Shpilevoy wrote:
> > Hi! Good job on the fixes, really!
> > 
> > Although function lbox_fiber_statof() still uses lua_pushinteger():
> > 
> 
> Thanks a huge for catching it, Vlad. Here is an update. Squashed
> to patch 1 "fiber: use uint64_t for fiber IDs" and force pushed.

And one more addon (squashed) because 20 chars won't fit 8 byte
unsiged together with "fiber: " prefix. Also I'm a bit worried of

static int
lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
{
	struct lua_State *L = (struct lua_State *) cb_ctx;

-->	lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);

why do we use float formate here at all?!

---
 src/lua/fiber.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 0d28fce8c..83c259c45 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -748,7 +748,7 @@ lbox_fiber_serialize(struct lua_State *L)
 static int
 lbox_fiber_tostring(struct lua_State *L)
 {
-	char buf[20];
+	char buf[32];
 	struct fiber *f = lbox_checkfiber(L, 1);
 	snprintf(buf, sizeof(buf), "fiber: %llu",
 		 (long long)f->fid);
-- 
2.31.1

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Vladislav Shpilevoy via Tarantool-patches]

On 11.05.2021 23:24, Cyrill Gorcunov wrote:
> On Wed, May 12, 2021 at 12:15:39AM +0300, Cyrill Gorcunov wrote:
>> On Tue, May 11, 2021 at 10:13:01PM +0200, Vladislav Shpilevoy wrote:
>>> Hi! Good job on the fixes, really!
>>>
>>> Although function lbox_fiber_statof() still uses lua_pushinteger():
>>>
>>
>> Thanks a huge for catching it, Vlad. Here is an update. Squashed
>> to patch 1 "fiber: use uint64_t for fiber IDs" and force pushed.
> 
> And one more addon (squashed) because 20 chars won't fit 8 byte
> unsiged together with "fiber: " prefix. Also I'm a bit worried of
> 
> static int
> lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
> {
> 	struct lua_State *L = (struct lua_State *) cb_ctx;
> 
> -->	lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
> 
> why do we use float formate here at all?!

Because lua_pushfstring() does not support %llu.

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Cyrill Gorcunov via Tarantool-patches]

On Wed, May 12, 2021 at 08:41:42PM +0200, Vladislav Shpilevoy wrote:
> > 
> > static int
> > lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
> > {
> > 	struct lua_State *L = (struct lua_State *) cb_ctx;
> > 
> > -->	lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
> > 
> > why do we use float formate here at all?!
> 
> Because lua_pushfstring() does not support %llu.

What about the idea below? float is 4 byte len and won't cover the
8 byte integer so we will have a rounding error.
---
[cyrill@grain tarantool.git] git diff
diff --git a/src/lua/fiber.c b/src/lua/fiber.c
index 02ec3d158..753b9aa16 100644
--- a/src/lua/fiber.c
+++ b/src/lua/fiber.c
@@ -337,7 +337,10 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
 {
        struct lua_State *L = (struct lua_State *) cb_ctx;
 
-       lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
+       char sbuf[FIBER_NAME_MAX + 32];
+       snprintf(sbuf, sizeof(sbuf), "%llu/%s",
+                (long long)f->fid, f->name);
+       lua_pushstring(L, sbuf);
 
        lua_newtable(L);
 

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Vladislav Shpilevoy via Tarantool-patches]

On 13.05.2021 10:17, Cyrill Gorcunov wrote:
> On Wed, May 12, 2021 at 08:41:42PM +0200, Vladislav Shpilevoy wrote:
>>>
>>> static int
>>> lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
>>> {
>>> 	struct lua_State *L = (struct lua_State *) cb_ctx;
>>>
>>> -->	lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
>>>
>>> why do we use float formate here at all?!
>>
>> Because lua_pushfstring() does not support %llu.
> 
> What about the idea below? float is 4 byte len and won't cover the
> 8 byte integer so we will have a rounding error.

Looks good. But also this is strange how did it work before? lua_Number
is double, if I am not mistaken. Which means it should have %lf format,
and it should be fine for all the reachable fiber IDs.

Anyway, the solution below looks good too.

> ---
> [cyrill@grain tarantool.git] git diff
> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
> index 02ec3d158..753b9aa16 100644
> --- a/src/lua/fiber.c
> +++ b/src/lua/fiber.c
> @@ -337,7 +337,10 @@ lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
>  {
>         struct lua_State *L = (struct lua_State *) cb_ctx;
>  
> -       lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
> +       char sbuf[FIBER_NAME_MAX + 32];
> +       snprintf(sbuf, sizeof(sbuf), "%llu/%s",
> +                (long long)f->fid, f->name);
> +       lua_pushstring(L, sbuf);
>  
>         lua_newtable(L);
>  
> 

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Cyrill Gorcunov via Tarantool-patches]

On Thu, May 13, 2021 at 01:17:15PM +0200, Vladislav Shpilevoy wrote:
> On 13.05.2021 10:17, Cyrill Gorcunov wrote:
> > On Wed, May 12, 2021 at 08:41:42PM +0200, Vladislav Shpilevoy wrote:
> >>>
> >>> static int
> >>> lbox_fiber_top_entry(struct fiber *f, void *cb_ctx)
> >>> {
> >>> 	struct lua_State *L = (struct lua_State *) cb_ctx;
> >>>
> >>> -->	lua_pushfstring(L, "%f/%s", (lua_Number)f->fid, f->name);
> >>>
> >>> why do we use float formate here at all?!
> >>
> >> Because lua_pushfstring() does not support %llu.
> > 
> > What about the idea below? float is 4 byte len and won't cover the
> > 8 byte integer so we will have a rounding error.
> 
> Looks good. But also this is strange how did it work before? lua_Number

I think it was simply a bit screwed but nobody noticed it yet.

> is double, if I am not mistaken. Which means it should have %lf format,
> and it should be fine for all the reachable fiber IDs.
> 
> Anyway, the solution below looks good too.

I squashed it into patch 1 and force pushed. Can I assume that I've got
your Ack?

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Vladislav Shpilevoy via Tarantool-patches]

LGTM!

Re: [Tarantool-patches] [PATCH v3 00/10] fix say_x format and rework fibers

[Kirill Yukhin via Tarantool-patches]

Hello,

On 04 май 18:58, Cyrill Gorcunov wrote:
> We've been notified that if tarantool instance is running for
> long time slice the logger start to print weird lines such as
> 
>  | main/-244760339/cartridge.failover.task I> Instance state changed
> 
> where fiber's ID represented as a negative number. Eventually we
> discovered a few issues
> 
>  - a bunch of say_x called with wrong specificators but compiler
>    simply ignored it
>  - fiber IDs are wrapping and this is inconvenient
>  - CFORMAT simply doesn't work
> 
> In the series we fix all these issues. Note that using simplified
> form for unsigned arguments such as
> 
> 	"%llu", (long long)(uint64_t)
> 
> is pretty fine becase signed/unsiged values are counterpart of
> each other guaranteed by standart.
> 
> issue https://github.com/tarantool/tarantool/issues/5846
> branch gorcunov/gh-5846-fid-name-3

I've checked your patch set into master.

--
Regards, Kirill Yukhin