[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: RFC: partman-auto-crypto



On Sun, Jun 25, 2006 at 05:05:01PM +0200, Max Vozeler wrote:
Last night I completed two test installs using the SVN version (r38313) and both worked flawlessly. One was using the "All in one partition" scheme, the other the "/home, /usr, /var, /tmp" scheme. The system was a VMware instance with one 2GB virtual SCSI disk attached. Only one thing surprised me: The /boot partition ended up with a size of ~250 MB when using the "All in one" scheme,
which seems very large. Maybe this is not specific to crypto
though, and it doesn't seem very important.

This is from the partman-auto-lvm scheme, there is already a bug filed against partman-lvm which mentions this (#342234, which is quite messy, I'll file a separate wishlist report against partman-auto-lvm).

Have you had a chance to test on a system with multiple
disks attached yet?

Yes, it makes no difference since partman-auto-crypto, like all partman-auto packages only support a single disk to work on.

1) Some comments exceed 80 characters. This makes it a bit
ugly to read on normal terminals.

I've fixed the ones that are in the code I've added, I wont change the code I haven't changed since it'll make it harder to share common parts later.

2) You can drop Standards-Version from debian/control. It does not apply to pure-udeb packages.

Done

3) Please add some information about authorship of the
code to debian/copyright. I realize this is tricky as other
partman-* packages are not very clear on copyright. It would
already help to say which code -auto-crypto is derived from
and which parts you hold copyright on.

Done

4) Some comments that originate from -auto-lvm no longer
match the code, which is a bit confusing. e.g. "This variable
will be used.. zero it" in automatically_partition/
some_device_crypto/do_option. It would help maintainability
if you could go through all comments and check whether they
still make sense.

I've removed that comment, couldn't find any other

5) Minor: There is "modprobe $foo 2>/dev/null 2>&1 || true" in
automatically_partition/.../do_option. This is not really
important, but it seems we either a) expect modprobe can fail -
in this case perhaps wrap it in log-output? Or we expect it
not to fail, which means the || true could be dropped. Also the
comment there, "Be sure modules are loaded" is not really true
if we don't check the return code of modprobe. </nitpick>

Same here as for the first note, I won't change the code which is suitable to be shared later.

There are currently two items on the todo list:

o Support loop-AES as well as dm-crypt (this mostly depends on partman-crypto getting encrypted-root-via-loop-AES support)

I'm hoping to work on this after my vacation (ie. in about two
weeks time). For now I actually think we can consider the non-
availability of loop-AES support an advantage, as we don't have
to think about how to present the choice of crypto system to
users in a non-confusing way. :-)

Yes, considering that we just changed the default in partman-crypto to be dm-crypt, perhaps we shouldn't even offer the choice between the two in partman-auto-crypto...

None of these items seem to be a reason to postpone the use
of partman-auto-crypto, and code sharing is hard to implement
before both packages are in the source tree proper.

I agree and would like to add my support for moving the partman-auto-crypto package to trunk/.

Cool, and thanks for the review.

Regards,
David



Reply to: