Re: libpq SSL with non-blocking sockets

On 06/25/2011 12:14 AM, Steve Singer wrote:
> I'm not a libpq guru but I've given your patch a look over.
>

Thanks for the review!

I have since simplified the patch to assume that partial SSL writes are
disabled -- according to SSL_write(3) this is the default behaviour.
Now the SSL retry buffer only holds the data to be retried, the
remainder is moved to the new outBuffer.

> -The patch doesn't compile with non-ssl builds, the debug at the bottom
> of PQSendSome isn't in an #ifdef
>

Ack, there was another one in pqFlush. Fixed that.

> -I don't think your handling the return code properly. Consider this case.
>
> pqSendSome(some data)
> sslRetryBuf = some Data
> return 1
> pqSendSome(more data)
> it sends all of 'some data'
> returns 0
>
> I think 1 should be returned because all of 'more data' still needs to
> be sent. I think returning a 0 will break PQsetnonblocking if you call
> it when there is data in both sslRetryBuf and outputBuffer.

Hmm, I thought I thought about that. There was a check in the original
patch: "if (conn->sslRetryBytes || (conn->outCount - remaining) > 0)"
So if the SSL retry buffer was emptied it would return 1 if there was
something left in the regular output buffer. Or did I miss something
there?

> We might even want to try sending the data in outputBuffer after we've
> sent all the data sitting in sslRetryBuf.
>

IMHO that'd add unnecessary complexity to the pqSendSome. As this only
happens in non-blocking mode the caller should be well prepared to
handle the retry.

> If you close the connection with an outstanding sslRetryBuf you need to
> free it.

Fixed.

New version of the patch attached.

regards,
Martin