Change in vdsm[master]: netinfo: Determine bootproto from dhclient's lease files
osvoboda at redhat.com
osvoboda at redhat.com
Fri Jan 31 18:55:37 UTC 2014
Ondřej Svoboda has posted comments on this change.
Change subject: netinfo: Determine bootproto from dhclient's lease files
......................................................................
Patch Set 14:
(7 comments)
http://gerrit.ovirt.org/#/c/23098/14/tests/functional/dhcp.py
File tests/functional/dhcp.py:
Line 64: '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT),
Line 65: '-1', interface])
Line 66:
Line 67: if rc: # == 2
Line 68: print ''.join(err)
> again, please do not print. log. or better - include the err string and rc
The error output will span multiple lines here so I will put the rc in a message and log stderr for verbose info.
Line 69: raise DhcpError('dhclient failed to obtain a lease.')
Line 70:
Line 71:
Line 72: def addNMplaceholderConnection(interface, connection):
http://gerrit.ovirt.org/#/c/23098/14/tests/functional/networkTests.py
File tests/functional/networkTests.py:
Line 1705:
Line 1706: dhcp.runDhclient(client, leaseFile, pidFile)
Line 1707: dhcp4 = getDhclientIfaces([leaseFile])
Line 1708:
Line 1709: # dhclient would already have exited with a return code 2 otherwise.
> the comment's necessity is not clear enough. there are all kinds of possibl
OK, the assertion below is enough and it covers any corner left.
Line 1710: self.assertIn(client, dhcp4, 'Test iface not found in a lease file.')
Line 1711:
Line 1712: @RequireDummyMod
Line 1713: @ValidateRunningAsRoot
http://gerrit.ovirt.org/#/c/23098/14/tests/netinfoTests.py
File tests/netinfoTests.py:
Line 293: finally:
Line 294: rmtree(tempDir)
Line 295:
Line 296: def testGetDhclientIfaces(self):
Line 297: leases = (
> that's a CONSTANT.
Done
Line 298: 'lease {\n'
Line 299: ' interface "valid";\n'
Line 300: ' expire 3 2037/01/29 18:25:46;\n'
Line 301: '}\n',
Line 301: '}\n',
Line 302: 'lease {\n'
Line 303: ' interface "expired";\n'
Line 304: ' expire 3 2014/01/29 18:25:46;\n'
Line 305: '}\n',
> drop the trailing coma and it becomes a string constant - no need to use jo
Done
Line 306: )
Line 307:
Line 308: dhcp4 = set()
Line 309: with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 304: ' expire 3 2014/01/29 18:25:46;\n'
Line 305: '}\n',
Line 306: )
Line 307:
Line 308: dhcp4 = set()
> why is this assignment needed?
It is not.
Line 309: with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 310: leaseFile = tmpDir + '/test.lease'
Line 311: with open(leaseFile, 'w') as f:
Line 312: f.write(''.join(leases))
Line 305: '}\n',
Line 306: )
Line 307:
Line 308: dhcp4 = set()
Line 309: with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
> please use a tmp directory (by dropping the "dir" arg) - this one requires
Found this one too :-)
Line 310: leaseFile = tmpDir + '/test.lease'
Line 311: with open(leaseFile, 'w') as f:
Line 312: f.write(''.join(leases))
Line 313:
Line 306: )
Line 307:
Line 308: dhcp4 = set()
Line 309: with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 310: leaseFile = tmpDir + '/test.lease'
> puritans would ask you to use os.path.join()
Done
Line 311: with open(leaseFile, 'w') as f:
Line 312: f.write(''.join(leases))
Line 313:
Line 314: dhcp4 = getDhclientIfaces([leaseFile])
--
To view, visit http://gerrit.ovirt.org/23098
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5fbc48a0adf5f40120a72ec2c4cc2fc80b7226b8
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list