On Tuesday 23 September 2008, Giuseppe Iuculano wrote: > attached debdiff fixes the root detection, can you review it please?
I have cloned the BR to grub-installer. Please do so yourself next time, preferably _before_ sending patches. Doing so prevents adding a load of basically unrelated info to the original report! (In this case you could also just have opened a new BR.) Note that this is most likely too late for RC1. Although the general solution may be OK, the patch needs *a lot* of improvement before it can be merged. Before it gets merged I'd like to see a dual boot with Windows tested as well. First of all: can this not just be done in a similar way to what's already been done for multipath in lines 103 and 109? If not, why? Cheers, FJP diff -Nru grub-installer-1.34/grub-installer grub-installer-1.35/grub-installer --- grub-installer-1.34/grub-installer 2008-09-21 22:25:25.000000000 +0200 +++ grub-installer-1.35/grub-installer 2008-09-23 20:39:15.000000000 +0200 @@ -135,6 +135,23 @@ tmp_drive=$(grep -v '^#' $device_map | grep "$tmp_disk *$" | \ sed 's%.*\(([hf]d[0-9][a-g0-9,]*)\).*%\1%') + # If not found, check if it is a SATA RAID partition, and set tmp_drive As this whole section is Linux-specific, I don't really like it being outside the 'case' statement. If it remains outside, it should start with a test for host_os. + satafound=0 I'd prefer to initialize to a null string instead of 0. This makes testing later simpler. + if type dmraid >/dev/null; then + if [ -z "$tmp_drive" ]; then These could be combined into a single if statement and thus reduce indentation. The 'type' statement also needs STDERR redirected. + if echo "$(basename $tmp_disk)" | grep -q "$(dmraid -sa -c | grep -v "No RAID disks")" ; then AFAIK 'dmraid -sa -c' can return multiple lines, so this is broken. The line is too long and should be wrapped. Note that in other places we use 'dmraid -s -c'. Is there a real difference? Consistency is important! + for raiddisk in $(dmraid -r -c | grep -v "No RAID disks"); do + tmp_drive=$(grep -v '^#' $device_map | grep "$raiddisk *$" | \ + sed 's%.*\(([hf]d[0-9][a-g0-9,]*)\).*%\1%') + satafound=1 + tmp_satadm=$(dmraid -sa -c | grep -v "No RAID disks") Why not set tmp_satadm earlier as you also use it in the last if? + tmp_satadev=$(basename $tmp_disk) + tmp_satadevn=$(echo $tmp_satadev | sed "s/`echo $tmp_satadm`//") The tmp_satadev variable seems rather redundant. What is that 'echo' good for? As surrounding code uses "%" for the sed expressions, it would be best to do that here too. + done + fi + fi + fi + # If not found, print an error message and exit if [ -z "$tmp_drive" ]; then echo "$1 does not have any corresponding BIOS drive." 1>&2 @@ -146,7 +163,11 @@ # GRUB's syntax case "$host_os" in linux*) - echo "$tmp_drive" | sed "s%)$%,`expr $tmp_part - 1`)%" + if [ "$satafound=1" ]; then This test is always true as it is written now and thus breaks normal installs. Also, please make the "normal" case the first option. + echo "$tmp_drive" | sed "s%)$%,`expr $tmp_satadevn - 1`)%" + else + echo "$tmp_drive" | sed "s%)$%,`expr $tmp_part - 1`)%" + fi Why not just set tmp_part instead of tmp_satadevn in the first hunk? Then this whole hunk would not be needed.
