Yaniv Bronhaim has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 16: (9 comments) http://gerrit.ovirt.org/#/c/26300/16/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 259: # Warning: this is for debugging purposes only! Never set this to True in Line 260: # production code, as will be sending out sensitive information (exception Line 261: # and stack trace details) when exceptions are raised inside Line 262: # SimpleXMLRPCRequestHandler.do_POST Line 263: _send_traceback_header = False not sure i would keep it at all in the code, maybe use the debug-plugin if possible Line 264: Line 265: def __init__(self, requestHandler=SimpleXMLRPCRequestHandler, Line 266: logRequests=True, allow_none=False, encoding=None): Line 267: self.logRequests = logRequests http://gerrit.ovirt.org/#/c/26300/16/tests/jsonRpcUtils.py File tests/jsonRpcUtils.py: Line 1: import httplib is this util used only for tests? if no why is it under tests, and if yes why don't you call it jsonRpcTestHelper Line 2: import logging Line 3: import os Line 4: import socket Line 5: import threading http://gerrit.ovirt.org/#/c/26300/16/vdsm.spec.in File vdsm.spec.in: Line 1409: %files yajsonrpc Line 1410: %dir %{python_sitelib}/yajsonrpc Line 1411: %{python_sitelib}/yajsonrpc/betterAsyncore.py* Line 1412: %{python_sitelib}/yajsonrpc/stomp.py* Line 1413: %{python_sitelib}/yajsonrpc/stompReactor.py* why don't you make it one package ? under jsonrpc you have yajsonrpc/__init__.py , you almost there :) so its either move the init file to yajsonrpc package or make from both one rpm which from my prospective sounds righter Line 1414: Line 1415: %files python-zombiereaper Line 1416: %{python_sitelib}/zombiereaper/__init__.py* Line 1417: http://gerrit.ovirt.org/#/c/26300/16/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 84: def load_json(self, bridge): Line 85: from BindingJsonRpc import BindingJsonRpc Line 86: Line 87: self._jsonBinding = BindingJsonRpc(bridge) Line 88: self._jsonBinding.start() I assume (and i might wrong) you don't want to run if one protocol failed to initialize, so why do you start the server if BindingJsonRpc returns None? report the warn message here if self._jsonBinding or _xmlBinding is None . btw, in that case don't we prefer to Panic ? or keep vdsm not initialized ? Line 89: Line 90: def load_xml(self, cif): Line 91: from BindingXMLRPC import BindingXMLRPC Line 92: Line 113: os.write(self._write_fd, '1') Line 114: Line 115: def _cleanup(self): Line 116: os.read(self._read_fd, 1) Line 117: write short comment how that works ^.. short, so that reader won't need to search how the protocol works if you already done that Line 118: def _accept_conn(self): Line 119: client_socket, socket_address = self.socket.accept() Line 120: client_socket.settimeout(self._timeout) Line 121: return (client_socket, socket_address) Line 170: if self.reactor: Line 171: self.reactor.stop() Line 172: Line 173: Line 174: class XmlDetector(): you could make the Detectors private.. Line 175: log = logging.getLogger("protocolDetector.XmlDetector") Line 176: Line 177: def __init__(self, xmlBinding): Line 178: self.xmlBinding = xmlBinding Line 175: log = logging.getLogger("protocolDetector.XmlDetector") Line 176: Line 177: def __init__(self, xmlBinding): Line 178: self.xmlBinding = xmlBinding Line 179: don't you want to warn here also if self._xmlBinding is None? should it be self_xmlBinding and self._jsonBinding? Line 180: def detect(self, data): Line 181: return data.startswith("POST") and "/RPC2" in data Line 182: Line 183: def handleSocket(self, client_socket, socket_address): http://gerrit.ovirt.org/#/c/26300/16/vdsm/sslutils.py File vdsm/sslutils.py: Line 1: # sslUtils.py . and why isn't is under lib/vdsm/ as Utils.py are ? Line 2: # Copyright 2014 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 69: return socket._fileobject(self, mode, bufsize) Line 70: else: Line 71: return self.connection.makefile(mode, bufsize) Line 72: Line 73: # # M2C do not supports MSG_PEEK flag and ? is it a todo? Line 74: # def read(self, size=4096, flag=None): Line 75: # result = None Line 76: # if flag == 2: Line 77: # result = self.connection.peek(size) -- 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: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
