Change in vdsm[master]: json-rpc: Protocol detection

nsoffer at redhat.com nsoffer at redhat.com
Thu Jun 5 15:54:22 UTC 2014


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.kliczewski at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list