Dan Kenigsberg has posted comments on this change.
Change subject: Move and encapsulate preun section into vdsm-tool
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(6 inline comments)
much better, but there's still a long way to go.
....................................................
File vdsm/constants.py.in
Line 71: # Configuration file definitions
constants.py is an unloved dumpster. do not put here stuff that is needed only by one
marginal module.
....................................................
File vdsm.spec.in
Line 400: # Stop vdsm service
please keep the indentation.
Line 412: # Restore libvirt service
since you have good function names, there's no need in comments.
comments are good only when they add information.
....................................................
File vdsm-tool/unregister.py
Line 26: BY_VDSM = '# by vdsm'
there used to be version-specific code here. please maintain feature parity.
which reminds me - functional tests are required for this patches.
Line 51: def __remove_from_file(fileName, begin=None, end=None):
the semantics of the function is not very clear.
it is impolite that your default args leads the function to skip lines ending with None.
unittest most certainly needed here.
Line 72: f.writelines(newfile)
this is quite bad, as it resets the old file before writing the new one. on
crash/poweroff, you end up with an inconsistent file.
try to use the atomic move trick (write to another name, then move) but remember to
ovirt-node case, where this is more complicated (ask mburns).
--
To view, visit
http://gerrit.ovirt.org/4526
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e7b5dc969dfb51e6880b9bb209a363609f5e123
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Lei Li <lilei(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl(a)us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh(a)us.ibm.com>