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