summaryrefslogtreecommitdiff
path: root/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal
diff options
context:
space:
mode:
Diffstat (limited to 'doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal')
-rw-r--r--doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment26
-rw-r--r--doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_2_d63d84b56ece233f795d1075aaba887a._comment18
-rw-r--r--doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_3_1405e20c8f5dc6e9cca3732e3e368d03._comment25
-rw-r--r--doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_4_20c6734d67fefeb1a8c07730d537e06b._comment8
-rw-r--r--doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_5_aee8b3d2768fb7307a6cc2e3295fd1f6._comment12
5 files changed, 89 insertions, 0 deletions
diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment
new file mode 100644
index 00000000..5982361f
--- /dev/null
+++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_1_74c6576b25f74c6e620eb015af8b0f6a._comment
@@ -0,0 +1,26 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 1"""
+ date="2017-08-31T22:40:34Z"
+ content="""
+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. :)
+
+* VolumeGroup and LogicalVolume seem like easy things to mix up.
+ Also, there's never a LogicalVolume without an associated VolumeGroup.
+ So, suggest `newtype VolumeGroup = VolumeGroup String` and
+ `data LogicalVolume = LogicalVolume String VolumeGroup` -- then
+ the user would write something like
+ `LogicalVolume "test" (VolumeGroup "vg0")`
+* Why not make `LvState` contain a `Maybe Partition.Fs` rather than
+ the string value. (This also would move the parsing of filesystem names
+ from `fsMatch` to `lvState` or perhaps to another function it uses.)
+* It seems a bit wrong for `parseSize` to 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.
+"""]]
diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_2_d63d84b56ece233f795d1075aaba887a._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_2_d63d84b56ece233f795d1075aaba887a._comment
new file mode 100644
index 00000000..546fe436
--- /dev/null
+++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_2_d63d84b56ece233f795d1075aaba887a._comment
@@ -0,0 +1,18 @@
+[[!comment format=mdwn
+ username="Nicolas.Schodet"
+ avatar="http://cdn.libravatar.org/avatar/0d7ec808ec329d04ee9a93c0da3c0089"
+ subject="comment 2"
+ date="2017-09-01T21:38:16Z"
+ content="""
+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?
+
+ File descriptor 10 (/usr/local/propellor/.lock) leaked on vgs invocation. Parent PID 31216: ./dist/build/propellor-config/p
+ File descriptor 11 (pipe:[282601]) leaked on vgs invocation. Parent PID 31216: ./dist/build/propellor-config/p
+ File descriptor 12 (pipe:[282601]) leaked on vgs invocation. Parent PID 31216: ./dist/build/propellor-config/p
+ File descriptor 13 (pipe:[282602]) leaked on vgs invocation. Parent PID 31216: ./dist/build/propellor-config/p
+ File descriptor 14 (pipe:[282602]) leaked on vgs invocation. Parent PID 31216: ./dist/build/propellor-config/p
+
+I have pushed a new version with the suggested fixes.
+"""]]
diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_3_1405e20c8f5dc6e9cca3732e3e368d03._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_3_1405e20c8f5dc6e9cca3732e3e368d03._comment
new file mode 100644
index 00000000..76c89ca6
--- /dev/null
+++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_3_1405e20c8f5dc6e9cca3732e3e368d03._comment
@@ -0,0 +1,25 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 3"""
+ date="2017-09-01T22:32:43Z"
+ content="""
+One way would be to use System.Process's `close_fds` when 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.
+"""]]
diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_4_20c6734d67fefeb1a8c07730d537e06b._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_4_20c6734d67fefeb1a8c07730d537e06b._comment
new file mode 100644
index 00000000..74a8bbe1
--- /dev/null
+++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_4_20c6734d67fefeb1a8c07730d537e06b._comment
@@ -0,0 +1,8 @@
+[[!comment format=mdwn
+ username="Nicolas.Schodet"
+ avatar="http://cdn.libravatar.org/avatar/0d7ec808ec329d04ee9a93c0da3c0089"
+ subject="comment 4"
+ date="2017-09-03T21:00:36Z"
+ content="""
+I can rebase/squash, do you see something else to improve?
+"""]]
diff --git a/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_5_aee8b3d2768fb7307a6cc2e3295fd1f6._comment b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_5_aee8b3d2768fb7307a6cc2e3295fd1f6._comment
new file mode 100644
index 00000000..6850f3b9
--- /dev/null
+++ b/doc/todo/LVM_logical_volume_creation__44___resize__44___format___38___removal/comment_5_aee8b3d2768fb7307a6cc2e3295fd1f6._comment
@@ -0,0 +1,12 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 5"""
+ date="2017-09-05T20:23:44Z"
+ content="""
+Looks good to me, merged. Thanks for your contribution!
+
+(I did make a simplification to it
+in [[!commit 0a6ad2b17419fd877789053c87b95866cfc39c46]],
+which seems ok by inspection to me, but I've not tested. Please
+let me know if I somehow got that wrong.)
+"""]]