Nir Soffer has posted comments on this change. Change subject: json-rpc: Protocol detection ......................................................................
Patch Set 33: (6 comments) protocol detection and xmlrpc looks fine, expect two issues: - This patch exposes a bug we had in the non-secure xmlprc version, where http 1.1 is not used. This causes a regression in the secure xmlrpc version. Trivial fix, but requires some testing with ssl=false. - None return value from SSLSocket.recv is not handled - trivial fix. http://gerrit.ovirt.org/#/c/26300/33/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 90: HTTP_HEADER_FLOWID = "FlowID" Line 91: Line 92: threadLocal = self.cif.threadLocal Line 93: Line 94: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler This change expose a bug in the non-secure xmlrpc version. SecureXMLRPCServer.SecureXMLRPCRequestHandler used before this patch is actually utils.IPXMLRPCRequestHandler, which is modified to support HTTP/1.1 since ovirt-3.4. So the correct code here would to remove the basehandler variable and replace its usage with utils.IPXMLRPCRequestHandler. This requires testing vdsm with ssl=false in vdsm.conf. Line 95: Line 96: class RequestHandler(basehandler): Line 97: Line 98: # Timeout for the request socket Line 92: threadLocal = self.cif.threadLocal Line 93: Line 94: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler Line 95: Line 96: class RequestHandler(basehandler): basehandler -> utils.IPXMLRPCRequestHandler Line 97: Line 98: # Timeout for the request socket Line 99: timeout = 60 Line 100: log = logging.getLogger("BindingXMLRPC.RequestHandler") Line 115: Line 116: def setup(self): Line 117: threadLocal.client = self.client_address[0] Line 118: threadLocal.server = self.request.getsockname()[0] Line 119: return basehandler.setup(self) basehandler -> utils.IPXMLRPCRequestHandler Line 120: Line 121: def do_GET(self): Line 122: try: Line 123: length = self._getIntHeader(self.HEADER_SIZE, Line 258: self.end_headers() Line 259: self.wfile.write(json_response) Line 260: Line 261: def parse_request(self): Line 262: r = basehandler.parse_request(self) basehandler -> utils.IPXMLRPCRequestHandler Line 263: threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID) Line 264: return r Line 265: Line 266: def finish(self): Line 263: threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID) Line 264: return r Line 265: Line 266: def finish(self): Line 267: basehandler.finish(self) basehandler -> utils.IPXMLRPCRequestHandler Line 268: threadLocal.client = None Line 269: threadLocal.server = None Line 270: threadLocal.flowID = None Line 271: http://gerrit.ovirt.org/#/c/26300/33/vdsm/protocolDetector.py File vdsm/protocolDetector.py: Line 178: self.log.warning("Unable to read data") Line 179: self._remove_connection(client_socket) Line 180: client_socket.close() Line 181: return Line 182: Looking in the betterAsyncore.py, we should handle here the case where data is None. This happens when using M2Crypto, if the SSLSocket want to read more data from the other side before it can return data. I would add this for now: if data is None: # SSLSocket wants to read more data from the peer return Line 183: self._remove_connection(client_socket) Line 184: try: Line 185: handler = self.detect_protocol(data) Line 186: except _CannotDetectProtocol: -- 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: 33 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@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: Vinzenz Feenstra <vfeen...@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