Hi On Thu, Feb 24, 2011 at 7:14 PM, Alon Levy <[email protected]> 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 _______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
