Change in vdsm[master]: Added VDSM interface for the virt-alignment-scan tool from l...

ewoud at kohlvanwijngaarden.nl ewoud at kohlvanwijngaarden.nl
Wed Mar 28 12:13:42 UTC 2012


Ewoud Kohl van Wijngaarden has posted comments on this change.

Change subject: Added VDSM interface for the virt-alignment-scan tool from libguestfs
......................................................................


Patch Set 4: (7 inline comments)

....................................................
File tests/alignmentScan_test.py
Line 41: class ScanImageBadPathTestCase(unittest.TestCase):
You can merge these all ScanImage test classes in a single test class. Name it something like ScanImageTestCase and the functions test_bad_path, test_bad_aligned and test_ok_aligned.

Line 45: def mkimage(imagepath):
You could add the sfdisk exec to this function as well since you always use mkimage and sfdisk together.

Line 46:     cmd = "/bin/dd if=/dev/zero".split() + ["of=%s" % imagepath] + "bs=4K count=1K".split()
What is against:

 cmd = ["/bin/dd", "if=/dev/zero", "of=%s" % imagepath, "bs=4K", "count=1K"]

Line 51:         img = tempfile.NamedTemporaryFile()
I personally prefer the with variant:

 with tempfile.NamedTemporaryFile() as img:
     # rest of my code

Line 56:         cmd.append(img.name)
I prefer writing it out full. So --unit instead of -u as well as no split().

 cmd = ["/sbin/sfdisk", "--unit", "S", "--force", img.name]

....................................................
File vdsm/alignmentScan.py
Line 29: def runScanArgs(args):
How about *args so you can call runScanArgs('--help') or runScanArgs('--add', image_path)

Line 51:     ScanOutput = namedtuple('ScanOutput', ['partition_name', 'partition_start_bytes', 'partition_alignment', 'alignment_scan_result'])
I think you can define this outside the function definition

--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saša Tomić <tomic80 at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud at kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saša Tomić <tomic80 at gmail.com>


More information about the vdsm-patches mailing list