Dan Kenigsberg has posted comments on this change.
Change subject: tests: Add NetworkTest.testAddDelNetworkDhcp
......................................................................
Patch Set 3: Code-Review-1
(9 comments)
....................................................
File tests/functional/dnsmasq.py
Line 19:
Line 20: from vdsm.utils import CommandPath
Line 21: from vdsm.utils import execCmd
Line 22:
Line 23: _DNSMASQ_BINARY = CommandPath('dnsmasq', '/sbin/dnsmasq')
el6 and f19 has /usr/sbin/dnsmasq. where have you verified commit?
Line 24:
Line 25:
Line 26: class Dnsmasq():
Line 27: def __init__(self):
Line 26: class Dnsmasq():
Line 27: def __init__(self):
Line 28: self.proc = None
Line 29:
Line 30: def start(self, interface, dhcpRange):
a nicer API would expect dhcpRange as a tuple of two address.
Line 31: '''
Line 32: --dhcp-option=3 don't send gateway address which would break
routing
Line 33: -k do not daemonize
Line 34: -p 0 disable all the dnsmasq dns functionality
....................................................
File tests/functional/firewall.py
Line 28:
Line 29:
Line 30: def allowDhcp(interface):
Line 31: try:
Line 32: if (_iptablesRunning):
* it's not C, no need for these parenthesis.
* (_iptablesRunning) is a function "pointer" which is invariably True.
* I do not see the benefit of the function definition.
Line 33: execCmd([_IPTABLES_BINARY.cmd, '-I', 'INPUT', '-i',
interface,
Line 34: '-p', 'udp', '--sport', '68', '--dport', '67',
'-j',
Line 35: 'ACCEPT'])
Line 36: if (_firewalldRunning):
Line 41: execCmd([_FIREWALLD_BINARY.cmd, '--zone=work',
'--add-interface='
Line 42: + interface])
Line 43: execCmd([_FIREWALLD_BINARY.cmd, '--zone=work',
Line 44: '--add-service=dhcp'])
Line 45: except Exception as e:
what would raise an exception here? you do not even check if execCmd calls have
succeeded or failed.
Line 46: raise SkipTest('Failed to allow dhcp traffic in firewall
because of' +
Line 47: '%s' % e)
Line 48:
Line 49:
....................................................
File tests/functional/networkTests.py
Line 80: @contextmanager
Line 81: def dnsmasqDhcp():
Line 82: """ Manages the life cycle of dnsmasq as a DHCP server."""
Line 83: try:
Line 84: dhcpServer = dnsmasq.Dnsmasq()
this must sit outside of the "try" block, as if it raises an exception, the
"finally" clause would find an unintialized variable.
Line 85: yield dhcpServer
Line 86: finally:
Line 87: if dhcpServer.proc:
Line 88: dhcpServer.stop()
Line 83: try:
Line 84: dhcpServer = dnsmasq.Dnsmasq()
Line 85: yield dhcpServer
Line 86: finally:
Line 87: if dhcpServer.proc:
this "if" would not be needed if dhcpServer is started before the "try" block.
Line 88: dhcpServer.stop()
Line 89:
Line 90:
Line 91: @contextmanager
Line 104: @contextmanager
Line 105: def vethIf():
Line 106: """ Yields a tuple containing pair of veth devices."""
Line 107: try:
Line 108: (right, left) = veth.create()
confusing (right, left) here, too.
again, the assignment must sit before the "try" block.
Line 109: yield (right, left)
Line 110: finally:
Line 111: veth.remove(right)
Line 112:
Line 1531: veth.setLinkUp(left)
Line 1532:
Line 1533: firewall.allowDhcp(left)
Line 1534: with dnsmasqDhcp() as dhcpServer:
Line 1535: dhcpServer.start(left, DHCP_RANGE)
I'd put this "start" into the contextmanager. It makes little sense to have
"stop" there, and "start" here.
Line 1536:
Line 1537: status, msg = self.vdsm_net.addNetwork(NETWORK_NAME,
Line 1538: nics=[right],
Line 1539:
opts={'bridged':
Line 1551: 'dhcp'})
Line 1552: self.assertEqual(status, SUCCESS, msg)
Line 1553: self.assertNetworkDoesntExist(NETWORK_NAME)
Line 1554:
Line 1555: firewall.stopAllowingDhcp(left)
this would not be called in case of an exception.
--
To view, visit http://gerrit.ovirt.org/21113
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9dbb59f3cd420b2071eb0ec42f9816ab52151bce
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Benas <pbe...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
