[Spice-devel] [PATCH 05/13] server/reds: RFC: improve ssl_{read, write} return values

[Marc-André Lureau]

Hi

On Thu, Feb 24, 2011 at 7:14 PM, Alon Levy <alevy at redhat.com> wrote:
> On Tue, Feb 22, 2011 at 05:08:59PM +0100, Marc-André Lureau wrote:
>> There is no known bug related to this patch, it might cause more harm
>> than good.
>>
>> The implementation of ssl_writev() looks incorrect: it never returned
>> an error (return_code is always >= 0).
>
> Actually it does - if the first call to write fails, then return_code==0,
> so return_code<=0, and so it returns n.

Ah yeah, I missed that. Then I don't know if my version makes it more clear...

>> Finally, since the SSL usage is hidden from the reader/writter, and it
>> assumes it is a fd-kind/libc of API (checking ERRNO), we should make
>> sure errno is set to a correct value in that case (EAGAIN for instance).
>>
> That (plus style changes) is what this patch adds. I have no idea if we
> use errno somewhere, no idea if this change is good or bad. It looks good
> looking at the callers (in red_channel.c red_peer_handle_outgoing for instance).


>> Note: we should be careful with the polling of SSL fd, since
>> SSL_read() or SSL_write() both can return WANT_READ or WANT_WRITE.  We
>> should poll both IN and OUT, not sure how it's handled now.
> me neither. Do you mean that after a WANT_READ we should only do a SSL_read and
> not do a SSL_write?

No I mean that if we call write() and we get EAGAIN, we might have to
poll In AND Out, depending on WANT_READ or WANT_WRITE (read() does not
imply *only* WANT_READ - same for write(). I don't think the current
code deal with that.

Anyway, since this patch doesn't really fix anything, we shouldn't
merge it. But it's good to keep as a reminder at least.

regards

-- 
Marc-André Lureau