Dan Kenigsberg has posted comments on this change.
Change subject: Added VDSM interface for the virt-alignment-scan tool from libguestfs
......................................................................
Patch Set 1: (9 inline comments)
....................................................
Commit Message
Line 16: Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
You should have used the same Change-Id of
http://gerrit.ovirt.org/2687
so we could compare this version to our comments to the former one.
Now I guess your best option is to abandon the old post, so it does not confuse us.
....................................................
File vdsm/alignmentScan.py
Line 25: def runScanArgs(args):
this should be local to the module (not that I very much like this function).
Line 31: image_path = image_path.strip()
do not strip - trailing spaces may be significant.
Line 33: raise ValueError("Image path does not exist")
I do not see a point in checking path existence here - let the tool fail and report its
error.
Line 35: retcode, out, err = runScanArgs(args)
please check the value of retcode, and raise an exception on error.
Line 42: return {'status':{ 'message':out_list,
'code':1},
please do not use the awkward external API vdsm verbs have, as this is an internal
function. out_list is not a message...
Instead, define a cleare API for this function: it could be a list of named tuples, each
one representing a different partition.
....................................................
File vdsm/alignmentScan_test.py
Line 1: #!/usr/bin/python
there's a tests sub directory for unittests, please help us filling it up!
Line 46: execCmd(cmd.split())
splitting on spaces is bad practice (space may appear withing imagepath). better start
with a list literal.
Line 50: img = "/tmp/aligncheck_bad.img"
please use tempdir.mktemp - who knows, two people may want to run this in conjunction.
--
To view, visit
http://gerrit.ovirt.org/2916
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d2c146a0fa3101317720f2e9c373e0d21b5cfdf
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić <tomic80(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>