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 <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
