I like these changes ! Comments bellow.
On Fri, Mar 13, 2009 at 09:40:39AM +0100, Martin Gracik wrote:
> Rewrote the devicelibs unittest baseclass so we can create and remove another loop devices within the tests.
>
> Rewrote the isRaidXY(raidlevel) functions in mdraid.py into one function isRaid(XY, raidlevel).
> Rewrote the get_raid_min_members and get_raid_max_spares functions
> to use the new isRaid function, and not use many if-elif-else statements.
>
> Changed the minimum raid members for raid10 to 2.
> ---
> storage/devicelibs/mdraid.py | 73 +++++++++++------------
> tests/storage/devicelibs/baseclass.py | 60 ++++++++++---------
> tests/storage/devicelibs/mdraid.py | 105 +++++++++++++++++++++++++++++++++
> 3 files changed, 172 insertions(+), 66 deletions(-)
> create mode 100644 tests/storage/devicelibs/mdraid.py
>
> diff --git a/storage/devicelibs/mdraid.py b/storage/devicelibs/mdraid.py
> index a0d2f6a..c8d8df2 100644
> --- a/storage/devicelibs/mdraid.py
> +++ b/storage/devicelibs/mdraid.py
> @@ -52,51 +52,46 @@ def getRaidLevels():
>
> raid_levels = getRaidLevels()
>
> -# FIXME: these functions should be consolidated into one function
> -def isRaid10(raidlevel):
> - """Return whether raidlevel is a valid descriptor of RAID10."""
> - return raidlevel in ("RAID10", "10", 10)
> -
> -def isRaid6(raidlevel):
> - """Return whether raidlevel is a valid descriptor of RAID6."""
> - return raidlevel in ("RAID6", "6", 6)
> -
> -def isRaid5(raidlevel):
> - """Return whether raidlevel is a valid descriptor of RAID5."""
> - return raidlevel in ("RAID5", "5", 5)
> -
> -def isRaid1(raidlevel):
> - """Return whether raidlevel is a valid descriptor of RAID1."""
> - return raidlevel in ("mirror", "RAID1", "1", 1)
> -
> -def isRaid0(raidlevel):
> - """Return whether raidlevel is a valid descriptor of RAID0."""
> - return raidlevel in ("stripe", "RAID0", "0", 0)
> +def isRaid(raid, raidlevel):
> + """Return whether raidlevel is a valid descriptor of raid"""
> + raid_descriptors = {10: ("RAID10", "10", 10),
> + 6: ("RAID6", "6", 6),
> + 5: ("RAID5", "5", 5),
> + 1: ("mirror", "RAID1", "1", 1),
> + 0: ("stripe", "RAID0", "0", 0)}
> +
> + if raid in raid_descriptors:
> + return raidlevel in raid_descriptors[raid]
> + else:
> + raise ValueError, "invalid raid in isRaid"
I would change the error message here to something like "Raid number %s
is unknown." The traceback will have the function call so there is no
need for specifying the function name.
Moreover I would specify some module vars
<snip>
RAID6=6
RAID5=5
.
.
.
</snip>
In this way the call to the function will look like:
<snip>
isRaid(mdraid.RAID6, VALUE)
</snip>
Which is better than:
<snip>
isRaid(6, VALUE)
</snip>
>
> def get_raid_min_members(raidlevel):
> """Return the minimum number of raid members required for raid level"""
> - if isRaid0(raidlevel):
> - return 2
> - elif isRaid1(raidlevel):
> - return 2
> - elif isRaid5(raidlevel):
> - return 3
> - elif isRaid6(raidlevel):
> - return 4
> - elif isRaid10(raidlevel):
> - return 4
> - else:
> - raise ValueError, "invalid raidlevel in get_raid_min_members"
> + raid_min_members = {10: 2,
> + 6: 4,
> + 5: 3,
> + 1: 2,
> + 0: 2}
> +
> + for raid, min_members in raid_min_members.items():
> + if isRaid(raid, raidlevel):
> + return min_members
> +
> + raise ValueError, "invalid raidlevel in get_raid_min_members"
>
> def get_raid_max_spares(raidlevel, nummembers):
> """Return the maximum number of raid spares for raidlevel."""
> - if isRaid0(raidlevel):
> - return 0
> - elif isRaid1(raidlevel) or isRaid5(raidlevel) or \
> - isRaid6(raidlevel) or isRaid10(raidlevel):
> - return max(0, nummembers - get_raid_min_members(raidlevel))
> - else:
> - raise ValueError, "invalid raidlevel in get_raid_max_spares"
> + raid_max_spares = {10: lambda: max(0, nummembers - get_raid_min_members(10)),
This is what I mean: it be better to call get_raid_min_members(RAID10).
> + 6: lambda: max(0, nummembers - get_raid_min_members(6)),
> + 5: lambda: max(0, nummembers - get_raid_min_members(5)),
> + 1: lambda: max(0, nummembers - get_raid_min_members(1)),
> + 0: lambda: 0}
> +
> + for raid, max_spares_func in raid_max_spares.items():
> + if isRaid(raid, raidlevel):
> + return max_spares_func()
> +
> + raise ValueError, "invalid raidlevel in get_raid_max_spares"
>
> def mdcreate(device, level, disks, spares=0):
> argv = ["--create", device, "--level", str(level)]
> diff --git a/tests/storage/devicelibs/baseclass.py b/tests/storage/devicelibs/baseclass.py
> index efc9c80..01fe6c7 100644
> --- a/tests/storage/devicelibs/baseclass.py
> +++ b/tests/storage/devicelibs/baseclass.py
> @@ -15,33 +15,39 @@ class TestDevicelibs(unittest.TestCase):
>
> def setUp(self):
> for dev, file in self._LOOP_DEVICES:
> - proc = subprocess.Popen(["dd", "if=/dev/zero", "of=%s" % file, "bs=1024", "count=102400"])
> - while True:
> - proc.communicate()
> - if proc.returncode is not None:
> - rc = proc.returncode
> - break
> - if rc:
> - raise OSError, "dd failed creating the file %s" % file
> -
> - proc = subprocess.Popen(["losetup", dev, file])
> - while True:
> - proc.communicate()
> - if proc.returncode is not None:
> - rc = proc.returncode
> - break
> - if rc:
> - raise OSError, "losetup failed setting up the loop device %s" % dev
> + self.makeLoopDev(dev, file)
>
> def tearDown(self):
> for dev, file in self._LOOP_DEVICES:
> - proc = subprocess.Popen(["losetup", "-d", dev])
> - while True:
> - proc.communicate()
> - if proc.returncode is not None:
> - rc = proc.returncode
> - break
> - if rc:
> - raise OSError, "losetup failed removing the loop device %s" % dev
> -
> - os.remove(file)
> + self.removeLoopDev(dev, file)
> +
> + def makeLoopDev(self, device_name, file_name):
> + proc = subprocess.Popen(["dd", "if=/dev/zero", "of=%s" % file_name, "bs=1024", "count=102400"])
> + while True:
> + proc.communicate()
> + if proc.returncode is not None:
> + rc = proc.returncode
> + break
> + if rc:
> + raise OSError, "dd failed creating the file %s" % file_name
> +
> + proc = subprocess.Popen(["losetup", device_name, file_name])
> + while True:
> + proc.communicate()
> + if proc.returncode is not None:
> + rc = proc.returncode
> + break
> + if rc:
> + raise OSError, "losetup failed setting up the loop device %s" % device_name
> +
> + def removeLoopDev(self, device_name, file_name):
> + proc = subprocess.Popen(["losetup", "-d", device_name])
> + while True:
> + proc.communicate()
> + if proc.returncode is not None:
> + rc = proc.returncode
> + break
> + if rc:
> + raise OSError, "losetup failed removing the loop device %s" % device_name
> +
> + os.remove(file_name)
> diff --git a/tests/storage/devicelibs/mdraid.py b/tests/storage/devicelibs/mdraid.py
> new file mode 100644
> index 0000000..d60d59e
> --- /dev/null
> +++ b/tests/storage/devicelibs/mdraid.py
> @@ -0,0 +1,105 @@
> +import baseclass
> +import unittest
> +import storage.devicelibs.mdraid as mdraid
> +
> +import time
> +
> +class TestMDRaid(baseclass.TestDevicelibs):
> +
> + def testMDRaidStuff(self):
> + ##
> + ## getRaidLevels
> + ##
> + # pass
> + self.assertEqual(mdraid.getRaidLevels(), mdraid.getRaidLevels())
> +
> + ##
> + ## getRaidMinMembers
> + ##
> + # pass
> + self.assertEqual(mdraid.getRaidMinMembers(0), 2)
Again, here you could call the mdraid.RAID0, instead of what you are
doing.
> + self.assertEqual(mdraid.getRaidMinMembers(1), 2)
> + self.assertEqual(mdraid.getRaidMinMembers(5), 3)
> + self.assertEqual(mdraid.getRaidMinMembers(6), 4)
> + self.assertEqual(mdraid.getRaidMinMembers(10), 2)
> +
> + # fail
> + # unsupported raid
> + self.assertRaises(ValueError, mdraid.getRaidMinMembers, 4)
.
.
.
> +
> + self.assertEqual(mdraid.mddestroy(self._LOOP_DEV0), None)
> + self.assertEqual(mdraid.mddestroy(self._LOOP_DEV1), None)
> +
> + # fail
> + # not a component
> + self.assertRaises(mdraid.MDRaidError, mdraid.mddestroy, "/dev/md0")
> + self.assertRaises(mdraid.MDRaidError, mdraid.mddestroy, "/not/existing/device")
> +
> +
> +if __name__ == "__main__":
> + suite = unittest.TestLoader().loadTestsFromTestCase(TestMDRaid)
> + unittest.TextTestRunner(verbosity=2).run(suite)
> +
> --
> 1.6.0.6
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
regards.
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
