Change in vdsm[master]: jsonrpc: xmlrpc protocol detection

danken at redhat.com danken at redhat.com
Mon Jun 9 21:47:57 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: jsonrpc: xmlrpc protocol detection
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.ovirt.org/#/c/28507/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 230:             self.wfile.write(response)
Line 231: 
Line 232: 
Line 233: class ConnectedTCPServer(SocketServer.TCPServer, object):
Line 234: 
I could use a docstring telling what this class is good for.
Line 235:     def __init__(self, RequestHandlerClass):
Line 236:         super(ConnectedTCPServer, self).__init__(None, RequestHandlerClass,
Line 237:                                                  False)
Line 238:         self.queue = Queue()


Line 253: 
Line 254: 
Line 255: class ConnectedSimpleXmlRPCServer(ConnectedTCPServer,
Line 256:                                   SimpleXMLRPCDispatcher):
Line 257:     allow_reuse_address = True
Please add a docstring stating what this class does, and that its code was copied from Python-2.7's SimpleXMLRPCServer.SimpleXMLRPCServer. (attribution is important)

Please handle this comment before merging.
Line 258: 
Line 259:     def __init__(self, requestHandler=SimpleXMLRPCRequestHandler,
Line 260:                  logRequests=True, allow_none=False, encoding=None):
Line 261:         self.logRequests = logRequests


Line 262: 
Line 263:         SimpleXMLRPCDispatcher.__init__(self, allow_none, encoding)
Line 264:         ConnectedTCPServer.__init__(self, requestHandler)
Line 265: 
Line 266:         # [Bug #1222790] If possible, set close-on-exec flag; if a
I find a url helpful, please add it: http://bugs.python.org/issue1222790
Line 267:         # method spawns a subprocess, the subprocess shouldn't have
Line 268:         # the listening socket open.
Line 269:         if fcntl is not None and hasattr(fcntl, 'FD_CLOEXEC'):
Line 270:             flags = fcntl.fcntl(self.fileno(), fcntl.F_GETFD)


Line 265: 
Line 266:         # [Bug #1222790] If possible, set close-on-exec flag; if a
Line 267:         # method spawns a subprocess, the subprocess shouldn't have
Line 268:         # the listening socket open.
Line 269:         if fcntl is not None and hasattr(fcntl, 'FD_CLOEXEC'):
in our context, fcntl is never None.
Line 270:             flags = fcntl.fcntl(self.fileno(), fcntl.F_GETFD)
Line 271:             flags |= fcntl.FD_CLOEXEC
Line 272:             fcntl.fcntl(self.fileno(), fcntl.F_SETFD, flags)
Line 273: 


http://gerrit.ovirt.org/#/c/28507/3/vdsm/protocoldetector.py
File vdsm/protocoldetector.py:

Line 30: from vdsm.utils import traceback
Line 31: from vdsm import utils
Line 32: 
Line 33: 
Line 34: class MultiProtocolAcceptor:
a docstring would be nice.

could you add a meaningful unit test, such as reading from a file instead of  a socket?
Line 35:     log = logging.getLogger("vds.MultiProtocolAcceptor")
Line 36: 
Line 37:     READ_ONLY_MASK = (select.POLLIN | select.POLLPRI | select.POLLHUP
Line 38:                       | select.POLLERR)


http://gerrit.ovirt.org/#/c/28507/3/vdsm/rpc/BindingXMLRPC.py
File vdsm/rpc/BindingXMLRPC.py:

Line 1120: 
Line 1121: 
Line 1122: class XmlDetector():
Line 1123:     log = logging.getLogger("XmlDetector")
Line 1124:     name = "xml"
NAME and REQUIRED_SIZE are class constants, and should be named as such.
Line 1125:     required_size = 10
Line 1126: 
Line 1127:     def __init__(self, xml_binding):
Line 1128:         self.xml_binding = xml_binding


-- 
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: 3
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: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei 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