On Sunday 20 July 2008, Ryan Niebur wrote:
> here are the patches
Other than the comments below and final testing this looks ready for
committing to me.
Please also test creating (and deleting) multiple RAID devices in the
same "configure RAID" session, for example RAID1 for /boot + RAID5 for
LVM + RAID 1 for swap + RAID0 for /home/media (or whatever).
Patch 1:
+ LEVEL=${1#RAID}
This statement should probably go _before_ the case statement to determine
minimum sizes. That would also "reunite" the setting of the minimum size
in the template with the code that determines it.
+ # Loop until at least one device has been selected for RAID 1
Comment is incorrect. We want only 1 device, not "at least". So it should
be: "# For RAID 1 only one device can be selected"
Patch 2:
+ f - far copies: Multiple copies have very different offsets
The explanation for the other two options end with a ".", this one does
not. I would suggest _removing_ the period for the other two options.
+ [ $(echo $RET | sed s/.//) -le $DEV_COUNT ]; then
Please add quotes around the sed regexp. Maybe not strictly needed, but
reduces the risk of unexpected errors, improves readability and
consistency with rest of code.
Patch3:
Looking at the diff in templates this is really worth doing. Should reduce
memory usage nicely.
+ for i in devcount devs sparecount sparedevs; do
+ db_subst mdcfg/raid$i LEVEL "$LEVEL"
+ done
I had to look at this a couple of times before I got what was happening.
Also note that you again split the determination of minimum size and
setting its use for templates, which makes the logic less clear.
I would suggest moving the 3 lines quoted above to before the case
statement for min. size and add a comment before them.
Keep the two lines that subst/set min size for the template together after
that case statement and also add a comment.
Cheers,
FJP
Attachment:
signature.asc
Description: This is a digitally signed message part.