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