Nir Soffer has posted comments on this change. Change subject: vdsm: Small post-jsonrpc udpates ......................................................................
Patch Set 1: Code-Review-1 (12 comments) This is a bunch of unrelated changes that should be submitted in separate unrelated patches (don't make patches depened on each other). http://gerrit.ovirt.org/#/c/28688/1//COMMIT_MSG Commit Message: Line 5: CommitDate: 2014-06-13 11:11:13 +0200 Line 6: Line 7: vdsm: Small post-jsonrpc udpates Line 8: Line 9: Docstrings updated for MultiProtocolAcceptor and ConnectedTCPServer. Separate to improving documenation patch Line 10: Command line test for SecureXMLRPCServer made to be unit test. Line 11: SSlTests updated to use new sslutils module. Line 12: Line 13: Line 6: Line 7: vdsm: Small post-jsonrpc udpates Line 8: Line 9: Docstrings updated for MultiProtocolAcceptor and ConnectedTCPServer. Line 10: Command line test for SecureXMLRPCServer made to be unit test. Nice but I think that nobody uses this class except the ssl tests. Anyway, not related to first change, submit a separate patch. Line 11: SSlTests updated to use new sslutils module. Line 12: Line 13: Line 14: Change-Id: I4aa9021f778a2cd30ef9c19db883fac999563d1f Line 7: vdsm: Small post-jsonrpc udpates Line 8: Line 9: Docstrings updated for MultiProtocolAcceptor and ConnectedTCPServer. Line 10: Command line test for SecureXMLRPCServer made to be unit test. Line 11: SSlTests updated to use new sslutils module. Again, not related, sumbit in a separate patch. Line 12: Line 13: Line 14: Change-Id: I4aa9021f778a2cd30ef9c19db883fac999563d1f http://gerrit.ovirt.org/#/c/28688/1/lib/vdsm/SecureXMLRPCServer.py File lib/vdsm/SecureXMLRPCServer.py: Line 39 Line 40 Line 41 Line 42 Line 43 Do you do this in another module now? It is very hard to review such change in gerrit. Line 57 Line 58 Line 59 Line 60 Line 61 > Removed due to interpreter complaining about "multiple values for keyword a This is wrong. You must understand why it happens and not use voodoo programming. Line 37: Line 38: from sslutils import SSLServerSocket Line 39: from .utils import IPXMLRPCRequestHandler, SimpleIPXMLRPCServer Line 40: Line 41: SecureXMLRPCRequestHandler = IPXMLRPCRequestHandler Who uses this now? Line 42: Line 43: Line 44: class SecureXMLRPCServer(SimpleIPXMLRPCServer): Line 45: def __init__(self, addr, Line 40: Line 41: SecureXMLRPCRequestHandler = IPXMLRPCRequestHandler Line 42: Line 43: Line 44: class SecureXMLRPCServer(SimpleIPXMLRPCServer): Who uses this class now? Line 45: def __init__(self, addr, Line 46: requestHandler=IPXMLRPCRequestHandler, Line 47: logRequests=True, allow_none=False, encoding=None, Line 48: bind_and_activate=True, http://gerrit.ovirt.org/#/c/28688/1/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 30: from fnmatch import fnmatch Line 31: from SimpleXMLRPCServer import \ Line 32: SimpleXMLRPCRequestHandler, \ Line 33: SimpleXMLRPCServer, \ Line 34: SimpleXMLRPCDispatcher Don't use \ for line breaking, and don't import multyple names per line. The previous imports were just fine. Line 35: from Queue import Queue Line 36: from StringIO import StringIO Line 37: from weakref import proxy Line 38: import SocketServer Line 284: def __init__(self, requestHandler=IPXMLRPCRequestHandler, Line 285: logRequests=True, allow_none=False, encoding=None, Line 286: bind_and_activate=False): Line 287: ConnectedSimpleXmlRPCServer.__init__(self, requestHandler, Line 288: logRequests, allow_none, encoding) Please use optional calling style for optional args. Line 289: Line 290: Line 291: class SimpleIPXMLRPCServer(SimpleXMLRPCServer): Line 292: Line 300: SimpleXMLRPCServer.__init__(self, addr, requestHandler=requestHandler, Line 301: logRequests=logRequests, Line 302: allow_none=allow_none, Line 303: encoding=encoding, Line 304: bind_and_activate=bind_and_activate) This is used only by SecureXMLRPCServer - right? move it there, possibly into the class itself instead of adding a new class. Line 305: Line 306: Line 307: # Threaded version of SimpleXMLRPCServer Line 308: class SimpleThreadedXMLRPCServer(SocketServer.ThreadingMixIn, http://gerrit.ovirt.org/#/c/28688/1/tests/secureXmlRpcServerTests.py File tests/secureXmlRpcServerTests.py: Line 22: import ssl Line 23: Line 24: from vdsm.SecureXMLRPCServer import \ Line 25: SecureXMLRPCServer, \ Line 26: VerifyingSafeTransport There is no reason to use mulitple lines here. Line 27: from testrunner import VdsmTestCase as TestCaseBase Line 28: import jsonRpcHelper Line 29: Line 30: Line 74: def stop(self): Line 75: self.vtransport.close() Line 76: Line 77: Line 78: class XmlRpcServerTests(TestCaseBase): Nice test, but why do we need it? I think that the SecureXMLRPCServer is not used now for anything interesting. Line 79: Line 80: def testServer(self): Line 81: server = Server() Line 82: client = Client() -- To view, visit http://gerrit.ovirt.org/28688 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4aa9021f778a2cd30ef9c19db883fac999563d1f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
