[asterisk-dev] [Code Review] Make ast_carefulwrite() ... be more careful

[Russell Bryant]

> On 2008-12-18 15:21:54, Mark Michelson wrote:
> > So here's a basic architectural question for you. Why is it that we write first and then poll the file descriptor only if we need to write more data? Wouldn't it make more sense and in fact be more "careful" to poll on the file descriptor and then write when the descriptor is ready?

Good point, sir.  :-)  I guess I just had it like that because that's what it did before.  What you suggest makes more sense.  Done.


> On 2008-12-18 15:21:54, Mark Michelson wrote:
> > /branches/1.4/main/utils.c, lines 951-979
> > <http://reviewboard.digium.com/r/99/diff/1/?file=1955#file1955line951>
> >
> >     This is strictly my opinion, but instead of having do {} while (1), why don't you instead do something like:
> >     
> >     while((res = poll(&pfd, 1, timeoutms)) <= 0))) {
> >         /* Error checking */
> >     }

Good suggestion.  That makes it more readable.  Done.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/99/#review251
-----------------------------------------------------------


On 2008-12-18 14:52:14, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/99/
> -----------------------------------------------------------
> 
> (Updated 2008-12-18 14:52:14)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch was written to address some cases where users of the Asterisk Manager Interface could observe partial writes of messages.  In Asterisk 1.4, the function ast_carefulwrite() is used to write data to the socket.  While the function was sort of careful, it was not careful enough.  This patch addresses the following issues:
> 
> 1) It did not previously handle an error of EINTR from write().
> 2) It did not handle EINTR or EGAIN errors from poll().
> 
> 
> This addresses bug 13546.
>     http://bugs.digium.com/view.php?id=13546
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/main/utils.c 165762 
> 
> Diff: http://reviewboard.digium.com/r/99/diff
> 
> 
> Testing
> -------
> 
> I connected to the AMI, made sure I could still log in, and observed normal looking responses and events.
> 
> 
> Thanks,
> 
> Russell
> 
>