From: Sergey Vojtovich Date: December 13 2010 1:41pm Subject: Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450 List-Archive: http://lists.mysql.com/commits/126640 Message-Id: <20101213134152.GA6570@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hi Dmitry, On Mon, Dec 13, 2010 at 11:21:11AM +0600, Dmitry Shulga wrote: > Hi Sergey! > On 09.12.2010, at 15:18, Sergey Vojtovich wrote: > > > Hi Dmitry, > > > >>> Later I realized that this is probably a regression caused by > >>> fix for BUG#41486. As far as I can see fill_buffer() returns > >>> -1 on error and 0 on eof. But with fix for BUG#41486 > >>> intern_read_line() ignores error from fill_buffer(), unless > >>> buffer->eof is set. > >>> > >>> So the main question is: what benefit gives hidding a bug in > >>> intern_read_line() instead of restoring original logics? > >> First, I didn't know about bug 41486 when I was implementing my patch. > > Ok. > > > >> Second, when I was inspecting this source code I found a bug in handling of > >> error from read syscall. I fixed this error. > > From what I can see if my_read() fails fill_buffer() returns -1 (failure) > > immediately. What is the bug here? > In previous version of fill_buffer() if call to my_read() returned -1 then fill_buffer() returned -1 immediately without setting flag eof. > If flag eof wasn't set then intern_read_line() didn't know about error and returned without an error. So an error on socket is hidden. It is a bug. > Does that mean eof == error? These conditions are very different to me. > > > >> I don't consider that I should redesign this piece of source code. If redesign is needed > >> then a separate request for redesign should be created. > > Do you mean we're in agreement that we should restore original, > > pre BUG#41486, logics? > No, I don't think so. If we arrange restoring of original behavior we'll only complicate matters. Could you elaborate how exactly we'll complicate matters? I look at the original fill_buffer() return values and see: -1 means error 0 means string truncated 0 + eof means eof >0 means ok I look at the updated fill_buffer() return values and see: -1 means temporal error -1 + eof means fatal error 0 means string truncation 0 + eof means eof >0 means ok To me original fill_buffer looks simpler. What am I doing wrong? Don't you think that realloc() failure (in fill_buffer) is fatal? What is the use case, which didn't work before, the concept of temporal error is attempting to address? Regards, Sergey -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com