Nir Soffer has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 26: (6 comments) Few important issues should be fixed. http://gerrit.ovirt.org/#/c/26300/26/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 204: Line 205: server = utils.SimpleThreadedXMLRPCServer( Line 206: requestHandler=RequestHandler, Line 207: logRequests=True) Line 208: utils.closeOnExec(server.socket.fileno()) This line is unneeded now, as the server should not have any listening socket. Line 209: Line 210: return server Line 211: Line 212: def _registerFunctions(self): Line 1060: try: Line 1061: method, rest = data.split(" ", 1) Line 1062: except ValueError: Line 1063: return False Line 1064: return method == "POST" and rest.startswith("/RPC2") As is, this will break storage image uploading through http, and requires also http://gerrit.ovirt.org/27839. Since each patch *must* not break anything, this cannot be merged as is. You must pass here both "PUT /" and "POST /RPC2". The next patch can remove the "PUT /" part from this detector. Line 1065: Line 1066: def handleSocket(self, client_socket, socket_address): Line 1067: self.xml_binding.add_socket(client_socket, socket_address) Line 1068: self.log.debug("xml over http detected from %s", socket_address) http://gerrit.ovirt.org/#/c/26300/26/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 107: client_socket.close() Line 108: Line 109: self._poller.unregister(self._socket) Line 110: self._poller.unregister(self._read_fd) Line 111: self._socket.close() It would be nice to close also _read_fd and _write_fd. Line 112: Line 113: def _cleanup_pending_connections(self): Line 114: for _, (accepted, client_socket) in self._pending_connections.items(): Line 115: if time.time() - accepted > self.CLEANUP_INTERVAL: Line 185: host, port = client_socket.getpeername() Line 186: self.log.debug("Detected protocol %s from %s:%d", Line 187: handler.name, host, port) Line 188: handler.handleSocket(client_socket, Line 189: client_socket.getsockname()) getsockname()?! use (host, port) Line 190: Line 191: def _create_socket(self, host, port): Line 192: addr = socket.getaddrinfo(host, port, socket.AF_INET, Line 193: socket.SOCK_STREAM) Line 193: socket.SOCK_STREAM) Line 194: if not addr: Line 195: raise Exception("Could not translate address '%s:%s'" Line 196: % (self._host, str(self._port))) Line 197: server_socket = socket.socket(addr[0][0], addr[0][1], addr[0][2]) You should use utils.closeOnExec(server_socket.fileno()) to ensure that child processes do not inherit this file descriptor. Line 198: try: Line 199: server_socket.bind(addr[0][4]) Line 200: server_socket.listen(5) Line 201: except socket.error as e: Line 198: try: Line 199: server_socket.bind(addr[0][4]) Line 200: server_socket.listen(5) Line 201: except socket.error as e: Line 202: if e.errno != errno.EINPROGRESS: The socket is created in blocking mode, so you cannot get this exception. You can safely remove the exception handler and let it blow if we get an exception here, as we cannot handle them. Line 203: raise Line 204: Line 205: if self._sslctx: Line 206: server_socket = SSLServerSocket(raw=server_socket, -- 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: 26 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.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
