Piotr Kliczewski has posted comments on this change. Change subject: Have the protocol detector use the Reactor ......................................................................
Patch Set 11: (8 comments) https://gerrit.ovirt.org/#/c/37098/11/lib/vdsm/sslutils.py File lib/vdsm/sslutils.py: Line 60: # we buffer first consumed message Line 61: def read(self, size=4096, flag=None): Line 62: result = None Line 63: if flag == socket.MSG_PEEK: Line 64: bytes_left = size - len(self._data) > was it a bug? It seems like a bug fix. Line 65: if bytes_left > 0: Line 66: self._data += self.connection.read(bytes_left) Line 67: result = self._data Line 68: else: Line 65: if bytes_left > 0: Line 66: self._data += self.connection.read(bytes_left) Line 67: result = self._data Line 68: else: Line 69: if self._data: > please state if self._data != '' if you don't use None anymore We can get None from ssl_read_nbio (connection.read) so we need to check it here. Line 70: result = self._data Line 71: self._data = '' Line 72: else: Line 73: result = self.connection.read(size) Line 250: self.key_file = key_file Line 251: self.cert_file = cert_file Line 252: Line 253: Line 254: class SSLHandshakeWrapper(object): > it should be called SSLHandshakeDispatcher or at least state that this clas Will change the name. Line 255: log = logging.getLogger("ProtocolDetector.SSLHandshakeWrapper") Line 256: SSL_HANDSHAKE_TIMEOUT = 10 Line 257: Line 258: def __init__( Line 264: self._give_up_at = monotonic_time() + handshake_timeout Line 265: self._has_been_set_up = False Line 266: self._is_handshaking = True Line 267: self._sslctx = sslctx Line 268: self._handshake_finished_handler = handshake_finished_handler > what is handshake_finished_handler? please document a bit .. at least what Done Line 269: Line 270: def _set_up_socket(self, dispatcher): Line 271: client_socket = dispatcher.socket Line 272: client_socket = self._sslctx.wrapSocket(client_socket) Line 271: client_socket = dispatcher.socket Line 272: client_socket = self._sslctx.wrapSocket(client_socket) Line 273: # Older versions of M2Crypto ignore nbio and retry internally Line 274: # if timeout is set. Line 275: client_socket.settimeout(None) > isn't this timeout define the session timeout? don't we need to expose this I am not sure what you mean here but comment above can help. Line 276: client_socket.address = client_socket.getpeername() Line 277: try: Line 278: client_socket.setup_ssl() Line 279: client_socket.set_accept_state() Line 277: try: Line 278: client_socket.setup_ssl() Line 279: client_socket.set_accept_state() Line 280: except SSL.SSLError as e: Line 281: self.log.warning("Error setting up ssl: %s", e) > only warning? it should be error Done Line 282: dispatcher.close() Line 283: return Line 284: Line 285: dispatcher.socket = client_socket Line 304: if self._is_handshaking: Line 305: try: Line 306: self._is_handshaking = (dispatcher.socket.accept_ssl() == 0) Line 307: except Exception as e: Line 308: self.log.debug("Error during handshake: %s", e) > error Done Line 309: dispatcher.close() Line 310: Line 311: if not self._is_handshaking: https://gerrit.ovirt.org/#/c/37098/11/vdsm/protocoldetector.py File vdsm/protocoldetector.py: Line 35: def _create_socket(host, port): Line 36: addr = socket.getaddrinfo(host, port, socket.AF_INET, Line 37: socket.SOCK_STREAM) Line 38: if not addr: Line 39: raise Exception("Could not translate address '%s:%s'" > raise socket.error Done Line 40: % (host, str(port))) Line 41: Line 42: server_socket = socket.socket(addr[0][0], addr[0][1], addr[0][2]) Line 43: filecontrol.set_close_on_exec(server_socket.fileno()) -- To view, visit https://gerrit.ovirt.org/37098 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If1cc2a205cdad335e8c3af4a77ed49c8977a79ba Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches