Hi, Kevin.
On Thu, Jul 16, 2015 at 10:13:42AM +0300, egbert. wrote:
> On Wed, Jul 15, 2015 at 08:15:55PM -0700, Kevin J. McCarthy wrote:
> > Kevin J. McCarthy wrote:
> > > Kevin J. McCarthy wrote:
> > > > I wasn't really happy having a loop counter option for GnuTLS. It turns
> > > > out the GNUTLS_E_AGAIN is translated directly from EAGAIN/EWOULDBLOCK.
> > > > Since we're not using non-blocking io, this will only be triggered by
> > > > our timeout.
> > >
> > > Sorry, I was wrong. It will return GNUTLS_E_AGAIN for situations other
> > > than the timeout we set, so my patch is incorrect. I'll have to think
> > > about this some more.
> >
> > After looking some more, I've re-convinced myself the patch approach is
> > actually okay.
> >
> > It looks like GnuTLS will return the GNUTLS_E_AGAIN for a partial
> > read only if recv returns a EAGAIN or EINTR. Otherwise it will keep
> > looping until it gets the needed amount. (see _gnutls_stream_read() and
> > _gnutls_io_read_buffered())
> >
> > The EAGAIN should only occur for a timeout on no data, and the EINTR
> > should only occur for a signal. Since the timeout is ours and we aren't
> > setting signals, there is no more reason to loop or handle these errors
> > than there is with raw_socket_read or raw_socket_write. Therefore I
> > believe patch should be okay after all.
> >
> > I am attaching a somewhat more aggressive patch that removes the looping
> > from tls_socket_read, and returns tls_socket_write to its behavior of
> > aborting on any error.
> >
> > Feedback and testing is definitely appreciated!
* I see you fixed a C90/C99 error.
+ struct timeval socket_timeout_tv = { 0, 0 };
* I’m glad you got rid of (my) socket looping counter, ‘num_tries’ or
whatever it was.
* Also I find the latest version simpler and easier to reason about (with
all looping gone). Only I’m not sure about the signals – is it the case that
if we get a signal, we will simply abort the read/write and return error?
(That seems reasonable). But a quick grep on INTR shows that someone was
worried about signals at one point.
* It’s good that you expanded this to include write sockets as well. But I
really think that the write timeout and the read timeout should be separate
config vars. For me, for example, I will have the read timeout set to
something low (5 or 10 seconds). But if I’m sending an email with a large
attachment, or on a slow connection, I want it to be something like 1 minute
or even 2.
Grts! Allen
