I have made a new property to handle logical volume with propellor.
I am not confident my haskell code is good looking as this is my first real life haskell code, can you please have a look?
You can pull the lvm branch at http://git.ni.fr.eu.org/nicolas/propellor.git
Thanks!
That's a pretty nice job for your first haskell code! And an impressive module.
Most of my review comments have to do with improving types.. Which is always a nice way to improve already good code.
newtype VolumeGroup = VolumeGroup Stringanddata LogicalVolume = LogicalVolume String VolumeGroup-- then the user would write something likeLogicalVolume "test" (VolumeGroup "vg0")LvStatecontain aMaybe Partition.Fsrather than the string value. (This also would move the parsing of filesystem names fromfsMatchtolvStateor perhaps to another function it uses.)parseSizeto include the rounding to the next extent, which is not really related to parsing. Would be better to split those two things into separate functions.I feel that this module is fairly close to mergeable.
Thanks for your comments.
I also have a problem when running vgs/lvs, they complain about leaked file descriptors. Is it something I can fix?
I have pushed a new version with the suggested fixes.
One way would be to use System.Process's
close_fdswhen executing vgs/lvs. BTW, I've seen such complaints from lvm before, in some situations not involving propellor.I've made a commit that makes the propellor lock FD be close-on-exec, which is generally a good idea for lock FDs anyway. (To prevent some long-running daemon process that does not close such FDs keeping the lock held.)
My guess is that the other 4 FDs, which are apparently pairs of FDs at both sides of a pipe, come from System.Console.Concurrent.Internal.bgProcess, which sets up just such a pipe. Quite possibly when vgs/lvs are run, it's via that function.
Generally leaking non-lock-related FDs to child processes is not a big problem, as long as the child process doesn't write to random FDs (which would be pretty bad, but what would ever do that?) ... So I don't know if I want to try to chase down every FD used all through propellor to set them close-on-exec.
Looks good to me, merged. Thanks for your contribution!
(I did make a simplification to it in 0a6ad2b17419fd877789053c87b95866cfc39c46, which seems ok by inspection to me, but I've not tested. Please let me know if I somehow got that wrong.)
Just tested it, it still works.
Thanks!