From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Fri, 5 Jul 2019 16:09:53 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH] fio: introduce utime function
Message-ID: <20190705130953.ht7jnn5waufdv4i4@esperanza>
References: <20190705090529.95338-1-olegrok@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190705090529.95338-1-olegrok@tarantool.org>
To: Oleg Babin <olegrok@tarantool.org>
Cc: tarantool-patches@freelists.org, Oleg Babin <babinoleg@mail.ru>
List-ID: <tarantool-patches.dev.tarantool.org>

On Fri, Jul 05, 2019 at 12:05:29PM +0300, Oleg Babin wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Closes #4323
> 
> Issue: https://github.com/tarantool/tarantool/issues/4323

Conventionally, we don't include links to tickets into commit messages -
we put them after '---' so that they are omitted by git-am.

> 
> @TarantoolBot document
> Title: fio.utime
> 
> fio.utime (filepath [, atime [, mtime]])
> Set access and modification times of a file. The first argument is the filename, the second argument (atime) is the access time, and the third argument (mtime) is the modification time. Both times are provided in seconds. If the modification time is omitted, the access time provided is used; if both times are omitted, the current time is used.

Please align the text so that its width is < 72 characters - it's a part
of the commit message, after all.

> times are provided in seconds

... since the epoch, I assume. Please mention it in the doc bot request.

> diff --git a/test/app/fio.result b/test/app/fio.result
> index 8819e39fa..ef21faffc 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -4,6 +4,9 @@ fio = require 'fio'
>  ffi = require 'ffi'
>  ---
>  ...
> +fiber = require 'fiber'
> +---
> +...
>  buffer = require 'buffer'
>  ---
>  ...
> @@ -1045,6 +1048,93 @@ fh:close()
>  ---
>  - true
>  ...
> +-- test utime
> +fh = fio.open('newfile', {'O_RDWR','O_CREAT'})
> +---
> +...
> +eps = 0.01
> +---
> +...
> +fio.utime('newfile', 0, 0)
> +---
> +- true
> +...
> +fh:stat().atime == 0
> +---
> +- true
> +...
> +fh:stat().mtime == 0
> +---
> +- true
> +...
> +fio.utime('newfile', 1, 2)
> +---
> +- true
> +...
> +fh:stat().atime == 1
> +---
> +- false
> +...
> +fh:stat().mtime == 2
> +---
> +- false
> +...
> +fio.utime('newfile', 3)
> +---
> +- true
> +...
> +fh:stat().atime == 3
> +---
> +- true
> +...
> +fh:stat().mtime == 3
> +---
> +- true
> +...
> +current_time = fiber.time()
> +---
> +...
> +fio.utime('newfile')
> +---
> +- true
> +...
> +fh:stat().atime - current_time < eps
> +---
> +- true
> +...
> +fh:stat().mtime - current_time < eps
> +---
> +- true
> +...

Under high CPU load (e.g. highly parallel test run), this may fail.
For instance, when I ran your test in a loop while having 50 CPU hogs
(while :; do :; done) in the background, I got a test failure:

} app/fio.test.lua                                                [ fail ]
} 
} Test failed! Result content mismatch:
} --- app/fio.result      Fri Jul  5 15:58:36 2019
} +++ app/fio.reject      Fri Jul  5 15:59:22 2019
} @@ -1100,11 +1100,11 @@
}  ...
}  fh:stat().atime - current_time < eps
}  ---
} -- true
} +- false
}  ...
}  fh:stat().mtime - current_time < eps
}  ---
} -- true
} +- false
}  ...
}  fio.utime(nil)
}  ---

Please rework the test to make sure it doesn't happen.

> +fio.utime(nil)
> +---
> +- error: 'builtin/fio.lua:480: Usage: fio.utime(filepath[, atime[, mtime]])'
> +...

Please make sure there are no source code line numbers in the result
file - otherwise we'd have to update it every time we patch fio.lua.

Take a look at "push filter" test run command. It's already used in
fio.test.lua so I guess you want to move it so that it covers your
test as well.

Other than that, the patch looks fine to me.