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