Nir Soffer has posted comments on this change. Change subject: jsonrpc: xmlrpc protocol detection ......................................................................
Patch Set 5: Code-Review-1 (9 comments) Few minor issues should be fixed. http://gerrit.ovirt.org/#/c/28507/5/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 233: class ConnectedTCPServer(SocketServer.TCPServer, object): Line 234: Line 235: def __init__(self, RequestHandlerClass): Line 236: super(ConnectedTCPServer, self).__init__(None, RequestHandlerClass, Line 237: False) False?! The method signature is: def __init__(self, server_address, RequestHandlerClass, bind_and_activate=True): Never call an optional argument in positional argument style. It is valid to add a new parameter in __init__ before bind_and_activate - and your code will break. Also it is more clear as bind_and_activate=False. Line 238: self.queue = Queue() Line 239: Line 240: def add(self, connected_socket, socket_address): Line 241: self.queue.put((connected_socket, socket_address)) Line 257: """ Line 258: Code below was part of SimpleXMLRPCServer.SimpleXMLRPCServer. Line 259: We need this code here in order to construct our ConnectedTCPServer Line 260: which let us provide connected sockets during runtime. Line 261: """ This docstring is useless. It may be needed on the ConnectedTCPServer class. Line 262: allow_reuse_address = True Line 263: Line 264: def __init__(self, requestHandler=SimpleXMLRPCRequestHandler, Line 265: logRequests=True, allow_none=False, encoding=None): Line 258: Code below was part of SimpleXMLRPCServer.SimpleXMLRPCServer. Line 259: We need this code here in order to construct our ConnectedTCPServer Line 260: which let us provide connected sockets during runtime. Line 261: """ Line 262: allow_reuse_address = True Not needed now as we don't have a listening socket. Line 263: Line 264: def __init__(self, requestHandler=SimpleXMLRPCRequestHandler, Line 265: logRequests=True, allow_none=False, encoding=None): Line 266: self.logRequests = logRequests Line 260: which let us provide connected sockets during runtime. Line 261: """ Line 262: allow_reuse_address = True Line 263: Line 264: def __init__(self, requestHandler=SimpleXMLRPCRequestHandler, Why do we need this default value? we never want ot use this request handler class. Line 265: logRequests=True, allow_none=False, encoding=None): Line 266: self.logRequests = logRequests Line 267: Line 268: SimpleXMLRPCDispatcher.__init__(self, allow_none, encoding) Line 272: #-on-exec flag; if a method spawns a subprocess, the subprocess Line 273: # shouldn't have the listening socket open. Line 274: flags = fcntl.fcntl(self.fileno(), fcntl.F_GETFD) Line 275: flags |= fcntl.FD_CLOEXEC Line 276: fcntl.fcntl(self.fileno(), fcntl.F_SETFD, flags) Not needed as we don't have listening socket on this server, and even if we needed it, we should use our utils.closeOnExec function. Line 277: Line 278: Line 279: class IPXMLRPCServer(ConnectedSimpleXmlRPCServer): Line 280: Line 282: daemon_threads = True Line 283: Line 284: def __init__(self, requestHandler=IPXMLRPCRequestHandler, Line 285: logRequests=True, allow_none=False, encoding=None, Line 286: bind_and_activate=True): bind_and_activate=True !? We don't need a listening socket for this server -- protocol detector is listening for connections. Line 287: ConnectedSimpleXmlRPCServer.__init__(self, requestHandler, Line 288: logRequests, allow_none, encoding) Line 289: Line 290: Line 290: Line 291: # Threaded version of SimpleXMLRPCServer Line 292: class SimpleThreadedXMLRPCServer(SocketServer.ThreadingMixIn, Line 293: IPXMLRPCServer): Line 294: allow_reuse_address = True Not needed now, we don't have a listening socket. Line 295: Line 296: Line 297: def _parseMemInfo(lines): Line 298: """ http://gerrit.ovirt.org/#/c/28507/5/vdsm/protocoldetector.py File vdsm/protocoldetector.py: Line 180: client_socket.close() Line 181: return Line 182: Line 183: if data is None: Line 184: # None is returned when we need to read more data (EAGIAN) No, None is returned when ssl socket need to read more data from the other side. EAGAIN is the error it should have raised if it was implemented properly. Line 185: return Line 186: Line 187: self._remove_connection(client_socket) Line 188: try: http://gerrit.ovirt.org/#/c/28507/5/vdsm/rpc/BindingXMLRPC.py File vdsm/rpc/BindingXMLRPC.py: Line 1121: Line 1122: class XmlDetector(): Line 1123: log = logging.getLogger("XmlDetector") Line 1124: NAME = "xml" Line 1125: REQUIRED_SIZE = 10 This is now 6 # len("POST /") Line 1126: Line 1127: def __init__(self, xml_binding): Line 1128: self.xml_binding = xml_binding Line 1129: -- To view, visit http://gerrit.ovirt.org/28507 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I72819cc0ab5bde0150456263ab7dd1a5e99abfeb Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[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
