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 <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
