Nir Soffer has posted comments on this change. Change subject: xmlrpc: fd leak fix ......................................................................
Patch Set 6: (4 comments) https://gerrit.ovirt.org/#/c/39506/6/lib/yajsonrpc/betterAsyncore.py File lib/yajsonrpc/betterAsyncore.py: Line 186: self._map = {} Line 187: self._is_running = False Line 188: self._wakeupEvent = AsyncoreEvent(self._map) Line 189: Line 190: def create_dispatcher(self, client, impl=None): Please use the same terms used by the dispatcher - "client" should be "sock" Line 191: return Dispatcher( Line 192: impl=impl, Line 193: sock=client, Line 194: map=self._map https://gerrit.ovirt.org/#/c/39506/6/vdsm/protocoldetector.py File vdsm/protocoldetector.py: Line 167 Line 168 Line 169 Line 170 Line 171 This indentation cause my eyes to bleed. Lets use more readable indentation: self._acceptor = self._reactor.create_dispatcher( sock, _AcceptorImpl(self._handle_accept)) Line 174: if self._sslctx is None: Line 175: self._register_protocol_detector( Line 176: self._reactor.create_dispatcher( Line 177: client Line 178: ) Are you sure this works? the first parameter is the socket? Can we format this in a more readable way? def handle_accept(self, client): if self._sslctx is None: dispatcher = self._reactor.create_dispatcher(client) self._register_protocol_detector(dispatcher) Line 179: ) Line 180: else: Line 181: self._reactor.create_dispatcher( Line 182: client, Line 183: SSLHandshakeDispatcher( Line 184: self._sslctx, Line 185: self._register_protocol_detector, Line 186: self.TIMEOUT, Line 187: ) Same indentation can be used here. Line 188: ) Line 189: Line 190: def _register_protocol_detector(self, dispatcher): Line 191: dispatcher.switch_implementation( -- To view, visit https://gerrit.ovirt.org/39506 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I705b0cf39937fe8d175305ad8ea8ad615f3ffb49 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[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
