Re: [PATCH 1/3] core/coio_file: Use eio_sendfile_sync instead of a chunk mode

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Thu, Apr 18, 2019 at 09:41:55PM +0300, Vladimir Davydov wrote:
> 
> The patch looks fine, but we typically don't split tests from the code
> when submitting a patch, i.e. I'd squash all the three patches in one.

ok

> Also, after having added a new error injection, you have to update
> box/errinj.result. Please do.

Ah, will do, tahnks!

> Also, please check that app/fio.test.lua works fine when Tarantool is
> built in Release mode. The thing is we typically disable tests using
> error injections for Release builds, because they don't work there
> (see suite.ini:release_disabled). I assume the test case you submitted
> should pass even without the error injection, nevertheless better check
> that.

Sure, thanks!

> > diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
> > index c5b2db781..20053558e 100644
> > --- a/src/lib/core/coio_file.c
> > +++ b/src/lib/core/coio_file.c
> > @@ -571,7 +571,7 @@ static void
> >  coio_do_copyfile(eio_req *req)
> >  {
> >  	struct coio_file_task *eio = (struct coio_file_task *)req->data;
> > -
> > +	off_t pos, ret, left;
> 
> Nit: We don't have a requirement to define all variables in the
> beginning of a code block. I'd define these variables right before
> the 'for' loop where they are used.

Yes, I noticed that, though personally I prefer to declare them early
(after all compiler highly likely will sub stack space at the
 procedure's start). Actually "define right before use" has a caveat:
implicit vars shadowing which is not always warned. Thus if this is
a code style violation I will change it of course, hmm?

	Cyrill