Change in vdsm[master]: context manager for dhclient in functional tests
danken at redhat.com
danken at redhat.com
Wed Oct 22 15:27:12 UTC 2014
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 <ibarkan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan 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