Change in vdsm[master]: Have the protocol detector use the Reactor
ybronhei at redhat.com
ybronhei at redhat.com
Sun Mar 15 08:21:22 UTC 2015
Yaniv Bronhaim 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?
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
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 class is being used only as "dispatcher" (inheritance could help but we avoid it anywhere)
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 argument this function should except
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 option?
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
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
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
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 <smizrahi at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list