Change in vdsm[master]: Move and encapsulate preun section into vdsm-tool

danken at redhat.com danken at redhat.com
Wed Jun 20 02:04:02 UTC 2012


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 at linux.vnet.ibm.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Lei Li <lilei at linux.vnet.ibm.com>
Gerrit-Reviewer: Ryan Harper <ryanh at us.ibm.com>


More information about the vdsm-patches mailing list