[Tarantool-patches] [PATCH] xlog: fix arguments processing in commit 8e429f4b7

[Tarantool-patches] [PATCH] xlog: fix arguments processing in commit 8e429f4b7

[Cyrill Gorcunov via Tarantool-patches]

The order of evaluation of arguments passed to a function
is not a part of language standart but compiler specifics.
Thus make sure we pass proper "errno" to xdir_say_gc.

Note that this is not critical issue: in case of hit
we simply get wrong syserror message when file not
present on a storage device.

Reported-by: Aleksandr Lyapunov <alyapunov@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
Guys, I didn't create a branch for this fix since it is not
critical and there is no issue for it. Even if the problem
occurs it won't affect the workflow so it is rather a cosmetic
fix.

 src/box/xlog.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 974f460be..462245d27 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -684,10 +684,13 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, unsigned flags)
 	       vclock_sum(vclock) < signature) {
 		const char *filename =
 			xdir_format_filename(dir, vclock_sum(vclock), NONE);
-		if (flags & XDIR_GC_ASYNC)
+		if (flags & XDIR_GC_ASYNC) {
 			eio_unlink(filename, 0, xdir_complete_gc, NULL);
-		else
-			xdir_say_gc(unlink(filename), errno, filename);
+		} else {
+			int rc = unlink(filename);
+			int _errno = rc != 0 ? errno : 0;
+			xdir_say_gc(rc, _errno, filename);
+		}
 		vclockset_remove(&dir->index, vclock);
 		free(vclock);
 
-- 
2.29.2

Re: [Tarantool-patches] [PATCH] xlog: fix arguments processing in commit 8e429f4b7

[Aleksandr Lyapunov via Tarantool-patches]

Hi! thanks for the patch!

Actually we found the error an fixed it in #5823.
You can see identical commit 733081f7042974f8fee9431e46cd45d2f1601438 .
But again thanks for your cooperation!

On 04.03.2021 17:25, Cyrill Gorcunov wrote:
> The order of evaluation of arguments passed to a function
> is not a part of language standart but compiler specifics.
> Thus make sure we pass proper "errno" to xdir_say_gc.
>
> Note that this is not critical issue: in case of hit
> we simply get wrong syserror message when file not
> present on a storage device.
>
> Reported-by: Aleksandr Lyapunov <alyapunov@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
> Guys, I didn't create a branch for this fix since it is not
> critical and there is no issue for it. Even if the problem
> occurs it won't affect the workflow so it is rather a cosmetic
> fix.
>
>   src/box/xlog.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/box/xlog.c b/src/box/xlog.c
> index 974f460be..462245d27 100644
> --- a/src/box/xlog.c
> +++ b/src/box/xlog.c
> @@ -684,10 +684,13 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, unsigned flags)
>   	       vclock_sum(vclock) < signature) {
>   		const char *filename =
>   			xdir_format_filename(dir, vclock_sum(vclock), NONE);
> -		if (flags & XDIR_GC_ASYNC)
> +		if (flags & XDIR_GC_ASYNC) {
>   			eio_unlink(filename, 0, xdir_complete_gc, NULL);
> -		else
> -			xdir_say_gc(unlink(filename), errno, filename);
> +		} else {
> +			int rc = unlink(filename);
> +			int _errno = rc != 0 ? errno : 0;
> +			xdir_say_gc(rc, _errno, filename);
> +		}
>   		vclockset_remove(&dir->index, vclock);
>   		free(vclock);
>   

Re: [Tarantool-patches] [PATCH] xlog: fix arguments processing in commit 8e429f4b7

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Mar 05, 2021 at 11:16:01AM +0300, Aleksandr Lyapunov wrote:
> Hi! thanks for the patch!
> 
> Actually we found the error an fixed it in #5823.
> You can see identical commit 733081f7042974f8fee9431e46cd45d2f1601438 .
> But again thanks for your cooperation!

Aha! I see, thanks! You know the patch you merged in actually still buggy
---
+               } else {
+                       int rc = unlink(filename);
+                       xdir_say_gc(rc, errno, filename);
+
}

The compiler has all rights to fetch @errno _before_ evaluating_ unlink,
there is no dependency. We might need a barrier. My patch did

+			int rc = unlink(filename);
+			int _errno = rc != 0 ? errno : 0;
+			xdir_say_gc(rc, _errno, filename);

here is _errno depends on unlink with a purpose.

Re: [Tarantool-patches] [PATCH] xlog: fix arguments processing in commit 8e429f4b7

[Cyrill Gorcunov via Tarantool-patches]

On Fri, Mar 05, 2021 at 11:26:13AM +0300, Cyrill Gorcunov wrote:
> On Fri, Mar 05, 2021 at 11:16:01AM +0300, Aleksandr Lyapunov wrote:
> > Hi! thanks for the patch!
> > 
> > Actually we found the error an fixed it in #5823.
> > You can see identical commit 733081f7042974f8fee9431e46cd45d2f1601438 .
> > But again thanks for your cooperation!

After conversation indeed the commit above should fix the problem. Thanks!