Dan Kenigsberg has posted comments on this change. Change subject: context manager for dhclient in functional tests ......................................................................
Patch Set 2: Code-Review-1 (6 comments) Thanks for avoiding this process leak - but please see my comments. http://gerrit.ovirt.org/#/c/34366/2/tests/functional/dhcp.py File tests/functional/dhcp.py: Line 95: if rc: # == 2 Line 96: logging.debug(''.join(err)) Line 97: raise DhcpError('dhclient failed to obtain a lease: %d', rc) Line 98: Line 99: with open(self._pid_file) as pid_fd: nit: strictly speaking, pid_fd is a file, not a file descriptor. Line 100: self.pid = int(pid_fd.read()) Line 101: Line 102: def kill(self): Line 103: if self.pid: Line 99: with open(self._pid_file) as pid_fd: Line 100: self.pid = int(pid_fd.read()) Line 101: Line 102: def kill(self): Line 103: if self.pid: kill() should not be called before the process has been started. We should raise in such a condition, not ignore that fact. So there's no need for this check. Line 104: try: Line 105: os.kill(self.pid, SIGKILL) Line 106: except OSError: Line 107: pass # No such process Line 103: if self.pid: Line 104: try: Line 105: os.kill(self.pid, SIGKILL) Line 106: except OSError: Line 107: pass # No such process There are many other flavors of OSError that we would not like to ignore. Please swallow only if OSError.errno == ESRCH. Line 108: Line 109: Line 110: @contextmanager Line 111: def dhclient(interface, tmpDir, dateFormat): Line 110: @contextmanager Line 111: def dhclient(interface, tmpDir, dateFormat): Line 112: dhclient_runner = _DhclientRunner(interface, tmpDir, dateFormat) Line 113: try: Line 114: dhclient_runner.run() please take run() out of the try-block, as there's nothing to kill unless run() finished successfully. Line 115: yield dhclient_runner Line 116: finally: Line 117: dhclient_runner.kill() Line 118: http://gerrit.ovirt.org/#/c/34366/2/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1991: with dnsmasqDhcp(server): Line 1992: Line 1993: with namedTemporaryDir(dir='/var/lib/dhclient') as dir: Line 1994: with dhcp.dhclient(client, dir, dateFormat) as \ Line 1995: _dh_client: local variable do not require the _ prefix. Line 1996: dhcp4 = getDhclientIfaces([_dh_client.lease_file]) Line 1997: Line 1998: self.assertIn(client, dhcp4, 'Test iface not found in a lease file.') Line 1999: http://gerrit.ovirt.org/#/c/34366/2/tests/tcTests.py File tests/tcTests.py: Line 353: become active when the bridge is ready, and the bridge only becomes ready Line 354: when it is attached to an active device. Line 355: """ Line 356: Line 357: # ###[ Ethernet ]### unrelated to this patch. Please rebase on master. Line 358: # dst = 00:1c:c0:d0:44:dc Line 359: # src = 00:21:5c:4d:42:75 Line 360: # type = IPv4 Line 361: # ###[ IP ]### -- To view, visit http://gerrit.ovirt.org/34366 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I458aa38415c697d3863e173444ff921d759166a2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[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
