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