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
diff --git a/debian/initramfs/cryptroot-script b/debian/initramfs/cryptroot-script
index 8c40af5..c83dcc1 100644
--- a/debian/initramfs/cryptroot-script
+++ b/debian/initramfs/cryptroot-script
@@ -303,7 +303,7 @@ setup_mapping()
return 1
fi
- NEWROOT="/dev/mapper/$cryptlvm"
+ NEWROOT=${cmdline_root:-/dev/mapper/$cryptlvm}
if [ "$cryptrootdev" = "yes" ]; then
# required for lilo to find the root device
echo "ROOT=$NEWROOT" >> /conf/param.conf
@@ -336,18 +336,34 @@ setup_mapping()
#
# Do we have any kernel boot arguments?
-found=''
+cmdline_cryptopts=''
+unset cmdline_root
for opt in $(cat /proc/cmdline); do
case $opt in
cryptopts=*)
- found=yes
- setup_mapping "${opt#cryptopts=}"
+ opt="${opt#cryptopts=}"
+ if [ -n "$opt" ]; then
+ cmdline_cryptopts="$cmdline_cryptopts $opt"
+ fi
;;
+ root=*)
+ opt="${opt#root=}"
+ case $opt in
+ /*) # Absolute path given. Not lilo major/minor number.
+ cmdline_root=$opt
+ ;;
+ *) # lilo major/minor number (See #398957). Ignore
+ esac
+ ;;
esac
done
-if [ -n "$found" ]; then
- exit 0
+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
# Do we have any settings from the /conf/conf.d/cryptroot file?
diff --git a/debian/initramfs/cryptroot-script b/debian/initramfs/cryptroot-script
index 8c40af5..cd67d3b 100644
--- a/debian/initramfs/cryptroot-script
+++ b/debian/initramfs/cryptroot-script
@@ -303,7 +303,11 @@ setup_mapping()
return 1
fi
- NEWROOT="/dev/mapper/$cryptlvm"
+ NEWROOT=${cmdline_root:-/dev/mapper/$cryptlvm}
+ message "cmdline: $(cat /proc/cmdline)"
+ message "cmdline_root: $cmdline_root"
+ message "cryptlvm: $cryptlvm"
+ message "NEWROOT: $NEWROOT"
if [ "$cryptrootdev" = "yes" ]; then
# required for lilo to find the root device
echo "ROOT=$NEWROOT" >> /conf/param.conf
@@ -336,22 +340,40 @@ setup_mapping()
#
# Do we have any kernel boot arguments?
-found=''
+cmdline_cryptopts=''
+unset cmdline_root
for opt in $(cat /proc/cmdline); do
case $opt in
cryptopts=*)
- found=yes
- setup_mapping "${opt#cryptopts=}"
+ opt="${opt#cryptopts=}"
+ if [ -n "$opt" ]; then
+ cmdline_cryptopts="$cmdline_cryptopts $opt"
+ fi
;;
+ root=*)
+ opt="${opt#root=}"
+ case $opt in
+ /*) # Absolute path given. Not lilo major/minor number.
+ cmdline_root=$opt
+ ;;
+ *) # lilo major/minor number (See #398957). Ignore
+ esac
+ ;;
esac
done
-if [ -n "$found" ]; then
- exit 0
+if [ -n "$cmdline_cryptopts" ]; then
+ # Call setup_mapping separately for each possible cryptopts= setting
+ for cryptopt in $cmdline_cryptopts; do
+ message "Calling setup_mapping: \"$cryptopt\""
+ setup_mapping "$cryptopt"
+ done
+ exit 0
fi
# Do we have any settings from the /conf/conf.d/cryptroot file?
if [ -r /conf/conf.d/cryptroot ]; then
+ message "/conf/conf.d/cryptroot: $(cat /conf/conf.d/cryptroot)"
while read mapping <&3; do
setup_mapping "$mapping" 3<&-
done 3< /conf/conf.d/cryptroot
