Bug#500079: Bug#497110: Missing root option for other detected Linux OS on dmraid

[Frans Pop <elendil@planet.nl>]

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.