From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Thu, 18 Apr 2019 21:50:22 +0300 From: Cyrill Gorcunov Subject: Re: [PATCH 1/3] core/coio_file: Use eio_sendfile_sync instead of a chunk mode Message-ID: <20190418185022.GK3040@uranus.lan> References: <20190416200858.19473-1-gorcunov@gmail.com> <20190416200858.19473-2-gorcunov@gmail.com> <20190418184155.2junklpawlvucyjh@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190418184155.2junklpawlvucyjh@esperanza> To: Vladimir Davydov Cc: tml List-ID: 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