Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

osvoboda at redhat.com osvoboda at redhat.com
Fri Feb 7 15:57:25 UTC 2014


Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/24192/4/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1702: 
Line 1703:                     with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 1704:                         confFile = os.path.join(tmpDir, 'test.conf')
Line 1705:                         leaseFile = os.path.join(tmpDir, 'test.lease')
Line 1706:                         pidFile = os.path.join(tmpDir, 'test.pid')
> In a follow-up patch, please generate pidFile within dhcp.runDhclient() - w
dhcp.runDhclient() is tightly bound to its only caller. If pidFile did not matter, I would make it an optional argument. But it is important to avoid a possible collision with another dhclient. If it was running, our own might refuse to run. Isolating any files dhclient uses is a good idea and working with them in one place is good for consistency.

Not passing the file name would require passing the path anyway. The minimum would be to pass the working path and the path to the lease file. Or refactoring the whole: move creating a tmpdir to dhcp.runDhclient and making it return a path to a lease file. But how would I clean up?
Line 1707: 
Line 1708:                         with open(confFile, 'w') as f:
Line 1709:                             f.write('db-time-format {0};'.format(dateFormat))
Line 1710: 


http://gerrit.ovirt.org/#/c/24192/4/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 301:             '  interface "valid";\n'
Line 302:             '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
Line 303:             '}}\n'
Line 304:             'lease {{\n'                # in an actual lease file,
Line 305:             '  interface "valid2";\n'   # human-readable date follows:
> but to simulate an actual lease file, this comment should be inside the str
Would it be acceptable to include these fixed comments in the string and make a comment outside explaining that they are mere examples?

An explanation is needed in any case, even when I would put real-looking locale-aware dates in comments, this time to say they aren't actually used...

I commented on the previous patch that the comments are not meant for a machine anyway...

Quoting man 5 dhclient.conf:

"The # symbol supplies a comment that describes what actual time this is as according to the system's configured timezone, at the time the value was written.  It is provided only for human inspection, the epoch time is the only recommended value for machine inspection."

"# %a %b %d %H:%M:%S %Y" is used as a format string in dhclient's source code.
Line 306:             '  expire epoch {1:.0f};\n'
Line 307:             #  expire epoch 2117041460; # Sat Jan 31 20:04:20 2037
Line 308:             '}}\n'
Line 309:             'lease {{\n'


-- 
To view, visit http://gerrit.ovirt.org/24192
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0
Gerrit-PatchSet: 4
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: 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