Saggi Mizrahi has posted comments on this change.
Change subject: json-rpc: Protocol detection
......................................................................
Patch Set 11:
(11 comments)
Amazingly good work.
Minor complaints.
Also, overall use mixedCase.
I like using_underscores as much as the next guy but that is what
was decided for VDSM.
So unless you are inheriting from a class that already uses underscores use mixed case.
http://gerrit.ovirt.org/#/c/26300/11/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 758: def stripNewLines(lines):
Line 759: return [l[:-1] if l.endswith('\n') else l for l in lines]
Line 760:
Line 761:
Line 762: def watchCmd(command, stop, cwd=None, data=None, recoveryCallback=None,
Rebase error?
Line 763: nice=None, ioclass=None, execCmdLogger=logging.root,
Line 764: deathSignal=signal.SIGKILL):
Line 765: """
Line 766: Executes an external command, optionally via sudo with stop abilities.
http://gerrit.ovirt.org/#/c/26300/11/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:
Line 460: mangledMethod = req.method.replace(".", "_")
Line 461: self.log.debug("Looking for method '%s' in bridge",
Line 462: mangledMethod)
Line 463: try:
Line 464: self.log.debug(self._bridge)
Leftover from debugging?
Line 465: method = getattr(self._bridge, mangledMethod)
Line 466: except AttributeError:
Line 467: if req.isNotification():
Line 468: return
http://gerrit.ovirt.org/#/c/26300/11/tests/jsonRpcUtils.py
File tests/jsonRpcUtils.py:
Line 66:
Line 67: @contextmanager
Line 68: def constructDetector(log, ssl, jsonBridge):
Line 69: sslctx = DEAFAULT_SSL_CONTEXT if ssl else None
Line 70: port = getFreePort()
Don't use get free port.
Bind to 0 and let the OS give us a free port.
Line 71: detector = ProtoDetector("127.0.0.1", port, None, 3, sslctx)
Line 72: detector.load_xml(FakeClientIf())
Line 73: detector.load_json(jsonBridge)
Line 74: detector.init_handlers()
Line 122: def create(socket):
Line 123: return XMLClient(socket)
Line 124:
Line 125:
Line 126: class XMLClient():
We already have and XMLRPC client
Line 127: def __init__(self, socket):
Line 128: self.socket = socket
Line 129: self.transport = CustomTransport(socket)
Line 130:
http://gerrit.ovirt.org/#/c/26300/11/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:
Line 904: (self.setupNetworks, 'setupNetworks'),
Line 905: (self.ping, 'ping'),
Line 906: (self.setSafeNetworkConfig, 'setSafeNetworkConfig'),
Line 907: (self.fenceNode, 'fenceNode'),
Line 908: (self.stop, 'prepareForShutdown'),
Unrelated?
Line 909: (self.setLogLevel, 'setLogLevel'),
Line 910: (self.setMOMPolicy, 'setMOMPolicy'),
Line 911: (self.setMOMPolicyParameters,
'setMOMPolicyParameters'),
Line 912: (self.setHaMaintenanceMode, 'setHaMaintenanceMode'),
http://gerrit.ovirt.org/#/c/26300/11/vdsm/clientIF.py
File vdsm/clientIF.py:
Line 148: else:
Line 149: cls._instance = clientIF(irs, log)
Line 150: return cls._instance
Line 151:
Line 152: def _prepareDetection(self):
_prepareProtocolDetecton
Line 153: timeout = config.getint('vars',
'vds_responsiveness_timeout')
Line 154: host = config.get('addresses', 'management_ip')
Line 155: port = config.getint('addresses', 'management_port')
Line 156: sslctx = None
http://gerrit.ovirt.org/#/c/26300/11/vdsm/protoDetector.py
File vdsm/protoDetector.py:
Line 26:
Line 27: from sslutils import SSLServerSocket
Line 28:
Line 29:
Line 30: class ProtoDetector:
PtotocolDetector
Line 31: log = logging.getLogger("protoDetector.ProtoDetector")
Line 32:
Line 33: READ_ONLY = select.POLLIN | select.POLLPRI | select.POLLHUP \
Line 34: | select.POLLERR
Line 29:
Line 30: class ProtoDetector:
Line 31: log = logging.getLogger("protoDetector.ProtoDetector")
Line 32:
Line 33: READ_ONLY = select.POLLIN | select.POLLPRI | select.POLLHUP \
Use () instead of \
Line 34: | select.POLLERR
Line 35:
Line 36: def __init__(self, host, port, default_bridge, timeout=60, sslctx=None):
Line 37: self._timeout = timeout
Line 60: for fd, flag in events:
Line 61: if flag & (select.POLLIN | select.POLLPRI):
Line 62: if fd is self._read_fd:
Line 63: self._cleanup()
Line 64: elif fd is self.socket.fileno():
Put the accept logic in it's own method
Line 65: client_socket, socket_address = self.socket.accept()
Line 66: client_socket.settimeout(self._timeout)
Line 67:
Line 68: try:
Line 123: if not addr:
Line 124: raise Exception("Could not translate address
'%s:%s'"
Line 125: % (self._host, str(self._port)))
Line 126: server_socket = socket.socket(addr[0][0], addr[0][1], addr[0][2])
Line 127: server_socket.setblocking(0)
You could leave it in blocking mode for the duration of the bind.
Would make this flow simpler and we don't really care about waiting since we don't
have a listener socket up yet.
Line 128: try:
Line 129: server_socket.bind(addr[0][4])
Line 130: server_socket.listen(5)
Line 131: except socket.error, e:
Line 148: self.reactor = self.jsonBinding.createStompReactor()
Line 149: self.jsonBinding.startReactor(self.reactor)
Line 150:
Line 151: def detect(self, data):
Line 152: return data.startswith("CONNECT") or
data.startswith("SEND")
I would try and clear some prefixing spaces or "\0" chars.
trimming the data should do the trick.
Line 153:
Line 154: def handleSocket(self, client_socket, socket_address):
Line 155: self.jsonBinding.add_socket(self.reactor, client_socket,
Line 156: socket_address)
--
To view, visit
http://gerrit.ovirt.org/26300
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id739a40e2b37dcc175137ec91cd5ec166ad24a75
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes