Thread: Simplify sleeping while reading/writing from client
Looking again at the code after Andres' interrupt-handling patch series, I got confused by the fact that there are several wait-retry loops in different layers, and reading and writing works slightly differently. I propose the attached, which pulls all the wait-retry logic up to secure_read() and secure_write(). This makes the code a lot more understandable. - Heikki
Attachment
On Thu, Feb 5, 2015 at 6:45 PM, Heikki Linnakangas wrote: > Looking again at the code after Andres' interrupt-handling patch series, I > got confused by the fact that there are several wait-retry loops in > different layers, and reading and writing works slightly differently. > > I propose the attached, which pulls all the wait-retry logic up to > secure_read() and secure_write(). This makes the code a lot more > understandable. Are you sure that it is a good idea to move the check of port->noblock out of be_tls_[read|write] to an upper level? ISTM that we should set errno and n to respectively EWOULDBLOCK and -1 in be_tls_[write|read] when port->noblock and do nothing otherwise. In your patch those values are set even if the port is in block mode. -- Michael
On 02/06/2015 10:38 AM, Michael Paquier wrote: > On Thu, Feb 5, 2015 at 6:45 PM, Heikki Linnakangas wrote: >> Looking again at the code after Andres' interrupt-handling patch series, I >> got confused by the fact that there are several wait-retry loops in >> different layers, and reading and writing works slightly differently. >> >> I propose the attached, which pulls all the wait-retry logic up to >> secure_read() and secure_write(). This makes the code a lot more >> understandable. > > Are you sure that it is a good idea to move the check of port->noblock > out of be_tls_[read|write] to an upper level? ISTM that we should set > errno and n to respectively EWOULDBLOCK and -1 in be_tls_[write|read] > when port->noblock and do nothing otherwise. In your patch those > values are set even if the port is in block mode. It simplifies the code to do all the sleeping and interrupt handling code in the upper level, in secure_[read|write]. Do you see a problem with it? - Heikki
On 2015-02-05 16:45:50 +0200, Heikki Linnakangas wrote: > Looking again at the code after Andres' interrupt-handling patch series, I > got confused by the fact that there are several wait-retry loops in > different layers, and reading and writing works slightly differently. They don't really work all that differently? > I propose the attached, which pulls all the wait-retry logic up to > secure_read() and secure_write(). This makes the code a lot more > understandable. Generally a good idea. Especially if we get more ssl implementations. But I think it'll make the already broken renegotiation handling even worse. Because now we're always entering do_handshake in nonblocking mode (essentially). Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Feb 6, 2015 at 1:46 PM, Heikki Linnakangas wrote: > It simplifies the code to do all the sleeping and interrupt handling code in > the upper level, in secure_[read|write]. Do you see a problem with it? Not directly. Reading the code I got uneasy with the fact that we fact unconditionally those parameters even if port is in block mode. -- Michael
On 02/06/2015 11:50 AM, Andres Freund wrote: > On 2015-02-05 16:45:50 +0200, Heikki Linnakangas wrote: >> I propose the attached, which pulls all the wait-retry logic up to >> secure_read() and secure_write(). This makes the code a lot more >> understandable. > > Generally a good idea. Especially if we get more ssl implementations. > > But I think it'll make the already broken renegotiation handling even > worse. Because now we're always entering do_handshake in nonblocking > mode (essentially). Good point. In the other thread, we concluded that the SSL_do_handshake() call isn't really needed anyway, so attached are two patches: 1. remove the SSL_do_handshake() calls, and 2. the previous patch, to simplify the waiting logic. - Heikki