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
