[pkg-cryptsetup-devel] Bug#546610: Bug#546610: cryptsetup: root= cmdline call is not honoured by initrd in crypto+lvm (Was initramfs-tools: ...)

[Agustin Martin]

On Thu, Feb 16, 2012 at 05:39:12PM +0100, Agustin Martin wrote:
> On Thu, Feb 16, 2012 at 02:26:57PM +0100, Jonas Meurer wrote:
> > > NEWROOT=${cmdline_root=/dev/mapper/$cryptlvm}
> > 
> > In my tests, this didn't work as expected. For some reason, $cryptlvm
> > is ignored within the specified default value for NEWROOT.
> 
> Not sure if related to the above, but I should have used
> 
> NEWROOT=${cmdline_root:-/dev/mapper/$cryptlvm}
> 
> Not using ":" might cause portability issues, and there is no need to change
> $cmdline_root value so ":-" instead of "=" seems better.
> 
> Do not have the lilo booted box here to test and remember the details. I
> however added some debugging lines just after that definition,

Hi, Jonas, finally could test this in a lilo booted box.

First noticed that my previous lilo tests were not as good as I expected
when testing that replacement line, all lilo entries had an explicit
root= line. So, I prepared a lilo entry with no root= line at all anywhere
and started testing again in my lilo system with no cryptopts= settings.

Everything seems to work well, even old "=" setting booted properly. Tested
with initramfs built in testing, booting in three cases, no root= at all
(booted testing), root= pointing to stable (booted stable) and root=
pointing to testing (booted testing). Substitutions seem to work as
expected.

Has your test setup something special?

> > > -if [ -n "$found" ]; then -	exit 0 +if [ -n "$cmdline_cryptopts" ];
> > > then +    setup_mapping "$cmdline_cryptopts" +    exit 0 fi
> > > 
> > > # Do we have any settings from the /conf/conf.d/cryptroot file?
> > 
> > This will break systems with more than one cryptopts-parameter.
> > Currently, it should work to give several cryptopts-parameters as boot
> > parameters and that way unlock more than one cryptdisk within the
> > initramfs. your patch will give all cryptopts parameters as one
> > argument seperated by commas to setup_mapping at once, instead of
> > invoking setup_mapping for every single cryptopts-parameter. Do you
> > get my point? I guess it would be enough to change the seperator from
> > comma to space and do something like 'for cryptopt in
> > $cmdline_cryptopts; do setup_mapping "$cryptopt"; done' later.
> 
> You are right. I have tested using 
> 
> if [ -n "$cmdline_cryptopts" ]; then
>     # Call setup_mapping separately for each possible cryptopts= setting
>     for cryptopt in $cmdline_cryptopts; do
>        setup_mapping "$cryptopt"
>     done
>     exit 0
> fi

After further inspection I noticed that test only checked that above code
did not cause evident problems, but did not actually test its behavior,
all boots were done without cryptopts= settings. If something reasonable
can be put there just to test, without the need of a special setup, please
let me know.

By the way seems that when using whitespace as separator instead of comma, 
things can be simplified a bit more when concatenating cryptopts= values.

I am attaching a couple of diffs, first one just contains clean current
status of my changes, while second also contains a bunch of 'message'
lines I used for debugging boot.

Please let me know if there is something else I can try in a normal box
(None of both boxes is a sandbox).

Regards,

-- 
Agustin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: initramfs_cryptroot-script_20120221.diff
Type: text/x-diff
Size: 1417 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-cryptsetup-devel/attachments/20120221/30ae8f5c/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: initramfs_cryptroot-script_DEBUG_20120221.diff
Type: text/x-diff
Size: 1890 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-cryptsetup-devel/attachments/20120221/30ae8f5c/attachment-0001.diff>