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@gmail.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com