Fujii Masao <masao.fujii@gmail.com> writes:
> On Sat, Jul 25, 2009 at 8:39 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>> Oh, another gripe: I'll bet a nickel that this doesn't work very nicely
>> under SSL. �Bytes available on the socket doesn't necessarily equate to
>> decrypted payload bytes being available. �Depending on how you're using
>> secure_poll, that might be okay, but it seems like a hazard waiting to
>> trap unwary maintainers.
> Is it only necessary to add the comment about how to use secure_poll?
> There is the assumption that secure_poll must be used with secure_write/read
> (e.g., in read case, pq_recvbuf instead of native recv should be called after
> passing pq_wait). So, it's assumed that encrypted data are resolved in those
> R/W functions and only decrypted data are located in buffer.
Well, actually, this description perfectly illustrates my basic
complaint: the patch breaks the API abstraction provided by pqcomm.c.
Callers are encouraged/forced to deal with the next layer down, and to
the extent that pqcomm.c does anything useful, callers have to allow for
that too.
As far as the read side of pq_wait goes, it should probably be more
like* return true if any bytes are in the buffer* else try to read some bytes into the buffer, without blocking* now
returntrue or false depending on whether bytes are in the buffer.
(Or perhaps instead of true/false, return the count of available bytes.)
Also, I suspect the API needs to make a distinction between "no more
bytes available yet" and EOF (channel closed, so no more bytes ever
will be available).
I'm not sure about the write side. The patch isn't really addressing
blocking-on-write, except in the offhand way of having a forWrite option
in pq_wait, but that doesn't seem too well thought out to me. Again,
the buffering pqcomm does makes a direct query of the underlying state
seem pretty suspicious.
regards, tom lane