List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:December 13 2010 1:41pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)
Bug#57450
View as plain text  
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 <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450Dmitry Shulga8 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)Bug#57450Sergey Vojtovich8 Dec
    • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450Dmitry Shulga9 Dec
      • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)Bug#57450Sergey Vojtovich9 Dec
        • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450Dmitry Shulga13 Dec
          • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)Bug#57450Sergey Vojtovich13 Dec