Firstly, at this point I am thoroughly convinced that something like
this patch is necessary, and I think the tone being taken with the
original submitter has been out of order; driving people away from
making improvements to d-i is not what we want to do, considering our
limited developer base! Yes, we should make sure that patches are
necessary, since all maintenance has a cost, but we should also
demonstrate a little more respect for the judgement of our fellow
developers and remember that there is a substantial difference in tone
between "I'm trying to understand why this is needed" and "huge NACK",
even if we are stressed out trying to produce something releaseable.
With that out of the way:
On Sun, Aug 10, 2008 at 03:29:16AM +0200, Robert Millan wrote:
> On Sat, Aug 09, 2008 at 10:17:34PM +0200, Frans Pop wrote:
> > 1) The use-case is insufficiently clear
> >
> > The reason _why_ a separate partition is required at all has (at least for
> > me) not been sufficiently explained. So far my impression is that all
> > this is just one huge ugly hack needed when GPT and some otherwise
> > undefined "BIOS-based bootloader" are combined, but I have no idea yet
> > for which type of systems that would be.
> > I may be completely wrong, but it's up to the submitter to provide
> > sufficient information. Instead we only get hackish patches without the
> > context.
>
> The whole purpose of this is solving a problem (whose technical details I
> have explained in #488111), that will affect all users who install Debian on
> standard PCs in the near future, when disks get bigger.
In short, we need somewhere to put GRUB 2's core.img on GPT that isn't
just sticking it in a filesystem, since the latter means GRUB's boot
sector needs to be able to find the filesystem in order to load bits of
itself and there isn't room in the boot sector to implement reliable
mechanisms for that (e.g. UUIDs). This is particularly problematic on
multi-disk setups.
http://grub.enbug.org/BIOS_Boot_Partition has a good, longer,
explanation.
> > - ensure it is created *only* in that use-case
>
> My last patch does that; except for GRUB, but then again there's no way to
> tell at the partitioning stage if user will want to use LILO.
Yes, and we certainly don't want to bail out at the boot loader
installation stage because the partitioning was wrong ...
In any case, GRUB is overwhelmingly the common case for d-i particularly
now that GRUB 2 can cope with /boot on LVM, so I don't think this minor,
non-installation-blocking issue should get in our way.
> > If the flag and this partition _are_ related then partman should ensure
> > that the BIOS boot partition flag can only be set on a BIOS boot
> > partition and not on any random other partition.
>
> There's no "flag". This is just the way Parted exports it to the upper
> layer. For example, if you set the boot "flag" in a GPT partition, you get
> an EFI partition type.
>
> > If the BIOS boot partition needs to be a separate partition because its
> > usage means any file system data could get overwritten, then partman
> > should ensure that such a partition cannot be used for a file system. The
> > current flag does not do that.
>
> I'll fix that.
The correct approach here, I think, is to use a partman method instead.
That ensures that a partition can contain a filesystem or a BIOS boot
area, but not both. This also makes preseeding a lot easier - we don't
need a special-cased $bios_boot{ } internal specifier any more, but can
just set the method in the natural way.
See my first attached patch. In particular I'd like translator comments
on the slight textual changes; Christian?
In future, we perhaps also ought to add a check.d script to
grub-installer to have it verify that a bios_grub partition exists when
on GPT. Not having it is no worse than the present situation, though.
> > My current feeling is that if it is demonstrated after that that a
> > separate bootloader partition really is needed, it should either
> > - be added based on the detection of a separate subarch (and thus
> > *separate* recipes), or
>
> It's not a subarch, as explained.
Indeed, a subarch would be entirely wrong here. "i386 with DOS partition
table" is not necessarily a different subarchitecture from "i386 with
GPT partition table"; this is most easily demonstrated by the fact that
the very same system will use DOS or GPT by default depending on whether
the disk is in excess of 2TB, or indeed that the very same system might
be given one or the other partition table type on a pre-existing disk.
I agree with Frans' earlier comment that there are good reasons to
continue to use the default recipes for i386 and amd64.
> > - be added at runtime on top of a normal, clean recipe based on very
> > specific and targeted tests (similar to the way a /boot partition is
> > added automatically when LVM is used).
>
> I did this already (Jérémy requested), except on buildtime.
>
> Shall I move it to runtime?
Given that this is much more specific to GPT than to the x86
architecture, and since it's vanishingly rare to use GPT on any other
architecture, I really don't think we need to make this x86-specific in
the recipes at all. Making sure that it's GPT-specific should be
adequate. Your most recent patch isn't bad for this, but the
implementation is a bit *too* specific to GPT. I propose an optional
$iflabel{ gpt } internal specifier in recipes instead. See my second
attached patch.
(For now, this is hard to make work for automatic LVM partitioning;
there's a comment in the source explaining why. I don't think we should
allow the perfect to be the enemy of the good, though.)
Cheers,
--
Colin Watson [cjwat...@debian.org]
Index: choose_method/_numbers
===================================================================
--- choose_method/_numbers (revision 0)
+++ choose_method/_numbers (revision 0)
@@ -0,0 +1 @@
+45 biosgrub
Index: choose_method/biosgrub/do_option
===================================================================
--- choose_method/biosgrub/do_option (revision 0)
+++ choose_method/biosgrub/do_option (revision 0)
@@ -0,0 +1,14 @@
+#!/bin/sh
+
+dev=$2
+id=$3
+
+mkdir -p $dev/$id
+
+echo biosgrub >$dev/$id/method
+>$dev/$id/bootable
+
+rm -f $dev/$id/use_filesystem
+rm -f $dev/$id/filesystem
+
+exit 0
Property changes on: choose_method/biosgrub/do_option
___________________________________________________________________
Added: svn:executable
+ *
Index: choose_method/biosgrub/choices
===================================================================
--- choose_method/biosgrub/choices (revision 0)
+++ choose_method/biosgrub/choices (revision 0)
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+. /lib/partman/lib/base.sh
+
+dev=$1
+id=$2
+
+cd "$dev"
+
+valid_biosgrub=no
+open_dialog VALID_FLAGS $id
+while { read_line flag; [ "$flag" ]; }; do
+ if [ "$flag" = bios_grub ]; then
+ valid_biosgrub=yes
+ fi
+done
+close_dialog
+
+[ "$valid_biosgrub" = yes ] || exit 0
+
+db_metaget partman/method_long/biosgrub description
+
+printf "biosgrub\t${RET}\n"
Property changes on: choose_method/biosgrub/choices
___________________________________________________________________
Added: svn:executable
+ *
Index: debian/partman-partitioning.templates
===================================================================
--- debian/partman-partitioning.templates (revision 60490)
+++ debian/partman-partitioning.templates (working copy)
@@ -216,13 +216,6 @@
# :sl2:
_Description: Name:
-Template: partman-partitioning/text/biosgrub
-Type: text
-# :sl3:
-# Setting to reserve a small part of the disk for use by BIOS-based bootloaders
-# such as GRUB.
-_Description: Reserve BIOS boot area:
-
Template: partman-partitioning/text/bootable
Type: text
# :sl2:
@@ -263,3 +256,17 @@
# :sl2:
_Description: Create a new empty partition table on this device
+Template: partman/method_long/biosgrub
+Type: text
+# :sl5:
+# Setting to reserve a small part of the disk for use by BIOS-based bootloaders
+# such as GRUB.
+_Description: Reserved BIOS boot area
+
+Template: partman/method_short/biosgrub
+Type: text
+# :sl5:
+# short variant of 'Reserved BIOS boot area'
+# Up to 10 character positions
+_Description: biosgrub
+
Index: debian/di-numbers
===================================================================
--- debian/di-numbers (revision 60491)
+++ debian/di-numbers (working copy)
@@ -2,3 +2,5 @@
active_partition lib/partman
free_space lib/partman
init.d lib/partman
+update.d lib/partman
+choose_method lib/partman
Index: debian/changelog
===================================================================
--- debian/changelog (revision 60491)
+++ debian/changelog (working copy)
@@ -1,6 +1,9 @@
partman-partitioning (71) UNRELEASED; urgency=low
* Upgrade to debhelper v7.
+ * Convert BIOS GRUB boot area support to a method, thereby making it
+ impossible to accidentally put a filesystem on such a partition as well,
+ and making it simpler to preseed. See #491376.
-- Colin Watson <cjwat...@debian.org> Fri, 28 Aug 2009 10:56:22 +0100
Index: active_partition/_numbers
===================================================================
--- active_partition/_numbers (revision 60493)
+++ active_partition/_numbers (working copy)
@@ -1,7 +1,6 @@
10 change_name
65 toggle_bootable
66 change_flags
-67 toggle_biosgrub
80 resize
83 copy
87 delete
Index: active_partition/toggle_biosgrub/do_option
===================================================================
--- active_partition/toggle_biosgrub/do_option (revision 60490)
+++ active_partition/toggle_biosgrub/do_option (working copy)
@@ -1,59 +0,0 @@
-#!/bin/sh
-
-. /lib/partman/lib/base.sh
-
-task=$1
-dev=$2
-id=$3
-
-cd $dev
-
-if [ "$task" = biosgrub ]; then
- open_dialog PARTITION_INFO $id
- read_line x1 x2 x3 type x5 x6 x7
- close_dialog
-fi
-
-new_flags=''
-open_dialog GET_FLAGS $id
-while { read_line flag; [ "$flag" ]; }; do
- if [ "$flag" != bios_grub ]; then
- if [ "$new_flags" ]; then
- new_flags="$new_flags
-$flag"
- else
- new_flags="$flag"
- fi
- fi
-done
-close_dialog
-
-if [ $task = biosgrub ]; then
- new_flags="$new_flags
-bios_grub"
-fi
-
-open_dialog SET_FLAGS $id
-write_line "$new_flags"
-write_line NO_MORE
-close_dialog
-
-partitions=''
-numparts=1
-open_dialog PARTITIONS
-while { read_line num id size type fs path name; [ "$id" ]; }; do
- [ "$fs" != free ] || continue
- partitions="$partitions $id"
- numparts=$(($numparts + 1))
-done
-close_dialog
-
-db_progress START 0 $numparts partman/text/please_wait
-db_progress INFO partman-partitioning/new_state
-
-for id in $partitions; do
- update_partition $dev $id
- db_progress STEP 1
-done
-
-db_progress STOP
Index: active_partition/toggle_biosgrub/choices
===================================================================
--- active_partition/toggle_biosgrub/choices (revision 60490)
+++ active_partition/toggle_biosgrub/choices (working copy)
@@ -1,39 +0,0 @@
-#!/bin/sh
-
-. /lib/partman/lib/base.sh
-
-dev=$1
-id=$2
-
-cd $dev
-
-valid_biosgrub=no
-open_dialog VALID_FLAGS $id
-while { read_line flag; [ "$flag" ]; }; do
- if [ "$flag" = bios_grub ]; then
- valid_biosgrub=yes
- fi
-done
-close_dialog
-
-[ $valid_biosgrub = yes ] || exit 0
-
-biosgrub=no
-open_dialog GET_FLAGS $id
-while { read_line flag; [ "$flag" ]; }; do
- if [ "$flag" = bios_grub ]; then
- biosgrub=yes
- fi
-done
-close_dialog
-
-db_metaget partman-partitioning/text/biosgrub description
-description="$RET"
-
-if [ $biosgrub = yes ]; then
- db_metaget partman-partitioning/text/on description
- printf "nobiosgrub\t%s\${!TAB}%s\n" "$description" "${RET}"
-else
- db_metaget partman-partitioning/text/off description
- printf "biosgrub\t%s\${!TAB}%s\n" "$description" "${RET}"
-fi
Index: update.d/biosgrub_sync_flag
===================================================================
--- update.d/biosgrub_sync_flag (revision 0)
+++ update.d/biosgrub_sync_flag (revision 0)
@@ -0,0 +1,51 @@
+#! /bin/sh
+
+# Remove the bios_grub flag for partitions whose method is not biosgrub, and
+# set it for partitions whose method is biosgrub.
+
+. /lib/partman/lib/base.sh
+
+dev=$1
+num=$2
+id=$3
+size=$4
+type=$5
+fs=$6
+path=$7
+name=$8
+
+cd $dev
+
+if [ "$fs" = free ]; then
+ exit 0
+fi
+
+method=
+if [ -f "$id/method" ]; then
+ method="$(cat "$id/method")"
+fi
+
+has_biosgrub=no
+flags=''
+open_dialog GET_FLAGS "$id"
+while { read_line flag; [ "$flag" ]; }; do
+ if [ "$flag" != bios_grub ]; then
+ flags="${flags:+$flags$NL}$flag"
+ else
+ has_biosgrub=yes
+ fi
+done
+close_dialog
+
+if [ "$method" = biosgrub ] && [ "$has_biosgrub" = no ]; then
+ open_dialog SET_FLAGS "$id"
+ write_line "$flags"
+ write_line bios_grub
+ write_line NO_MORE
+ close_dialog
+elif [ "$method" != biosgrub ] && [ "$has_biosgrub" = yes ]; then
+ open_dialog SET_FLAGS "$id"
+ write_line "$flags"
+ write_line NO_MORE
+ close_dialog
+fi
Property changes on: update.d/biosgrub_sync_flag
___________________________________________________________________
Added: svn:executable
+ *
Index: update.d/biosgrub_visuals
===================================================================
--- update.d/biosgrub_visuals (revision 0)
+++ update.d/biosgrub_visuals (revision 0)
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+. /usr/share/debconf/confmodule
+
+dev=$1
+num=$2
+id=$3
+size=$4
+type=$5
+fs=$6
+path=$7
+name=$8
+
+cd $dev
+
+[ -f $id/method ] || exit 0
+method=$(cat $id/method)
+
+case "$method" in
+ biosgrub)
+ db_metaget partman/method_short/biosgrub description || RET=''
+ printf "${RET:-biosgrub}" >$id/visual_filesystem
+ >$id/visual_mountpoint
+ ;;
+esac
Property changes on: update.d/biosgrub_visuals
___________________________________________________________________
Added: svn:executable
+ *
Index: update.d/_numbers
===================================================================
--- update.d/_numbers (revision 0)
+++ update.d/_numbers (revision 0)
@@ -0,0 +1,2 @@
+21 biosgrub_sync_flag
+60 biosgrub_visuals
Index: init.d/_numbers
===================================================================
--- init.d/_numbers (revision 60490)
+++ init.d/_numbers (working copy)
@@ -1 +1,2 @@
01 unsupported
+50 biosgrub
Index: init.d/biosgrub
===================================================================
--- init.d/biosgrub (revision 0)
+++ init.d/biosgrub (revision 0)
@@ -0,0 +1,34 @@
+#! /bin/sh
+
+# Set method "biosgrub" for all partitions that have the bios_grub flag set.
+
+. /lib/partman/lib/base.sh
+
+for dev in $DEVICES/*; do
+ [ -d "$dev" ] || continue
+ cd "$dev"
+ partitions=
+ open_dialog PARTITIONS
+ while { read_line num id size type fs path name; [ "$id" ]; }; do
+ if [ "$fs" != free ]; then
+ partitions="$partitions $id"
+ fi
+ done
+ close_dialog
+
+ for id in $partitions; do
+ biosgrub=no
+ open_dialog GET_FLAGS $id
+ while { read_line flag; [ "$flag" ]; }; do
+ if [ "$flag" = bios_grub ]; then
+ biosgrub=yes
+ # cannot break here
+ fi
+ done
+ close_dialog
+ if [ "$biosgrub" = yes ]; then
+ mkdir -p $id
+ echo biosgrub >$id/method
+ fi
+ done
+done
Property changes on: init.d/biosgrub
___________________________________________________________________
Added: svn:executable
+ *
Index: debian/changelog
===================================================================
--- debian/changelog (revision 60463)
+++ debian/changelog (working copy)
@@ -1,6 +1,8 @@
partman-auto (89) UNRELEASED; urgency=low
* Upgrade to debhelper v7.
+ * Add a biosgrub partition to default recipes, used only if the disk label
+ is GPT (closes: #491376). Requires partman-partitioning 71.
-- Colin Watson <cjwat...@debian.org> Thu, 27 Aug 2009 00:39:31 +0100
Index: recipes/home
===================================================================
--- recipes/home (revision 60462)
+++ recipes/home (working copy)
@@ -1,5 +1,9 @@
partman-auto/text/home_scheme ::
+1 1 1 free
+ $iflabel{ gpt }
+ method{ biosgrub } .
+
128 512 256 ext2
$defaultignore{ }
$primary{ }
Index: recipes/multi
===================================================================
--- recipes/multi (revision 60462)
+++ recipes/multi (working copy)
@@ -1,5 +1,9 @@
partman-auto/text/multi_scheme ::
+1 1 1 free
+ $iflabel{ gpt }
+ method{ biosgrub } .
+
128 512 256 ext2
$defaultignore{ }
$primary{ }
Index: recipes/atomic
===================================================================
--- recipes/atomic (revision 60462)
+++ recipes/atomic (working copy)
@@ -1,5 +1,9 @@
partman-auto/text/atomic_scheme ::
+1 1 1 free
+ $iflabel{ gpt }
+ method{ biosgrub } .
+
128 512 256 ext2
$defaultignore{ }
$primary{ }
Index: lib/recipes.sh
===================================================================
--- lib/recipes.sh (revision 60462)
+++ lib/recipes.sh (working copy)
@@ -27,7 +27,7 @@
unnamed=0
decode_recipe () {
- local ignore ram line word min factor max fs -
+ local ignore ram line word min factor max fs iflabel label -
ignore="${2:+${2}ignore}"
unnamed=$(($unnamed + 1))
ram=$(grep ^Mem: /proc/meminfo | { read x y z; echo $y; }) # in bytes
@@ -111,7 +111,25 @@
if [ "$ignore" ] && [ "$(echo $line | grep "$ignore")" ]; then
:
else
- scheme="${scheme:+$scheme$NL}$line"
+ # Exclude partitions that are only for a different
+ # disk label. The $PWD check avoids problems when
+ # running from partman-auto-lvm, where we aren't in
+ # a subdirectory of $DEVICES while decoding the
+ # recipe; but we do need to perform this check early
+ # so that size calculations work. As a result, for
+ # now, $iflabel will not work when doing automatic
+ # LVM partitioning.
+ iflabel="$(echo $line | sed -n 's/.*\$iflabel{ \([^}]*\) }.*/\1/p')"
+ if [ "$iflabel" ] && [ "${PWD#$DEVICES/}" != "$PWD" ]; then
+ open_dialog GET_LABEL_TYPE
+ read_line label
+ close_dialog
+ if [ "$iflabel" = "$label" ]; then
+ scheme="${scheme:+$scheme$NL}$line"
+ fi
+ else
+ scheme="${scheme:+$scheme$NL}$line"
+ fi
fi
line=''
;;
