Re: Bug#49948: liloconfig makes bad lilo.conf when from scratch
[Long email; sorry...]
pmaydell@chiark.greenend.org.uk wrote:
>Joost Kooij wrote:
>>I've done a patch for lilo.postinst that addresses the problem.
>>It also stops liloconfig from being rerun on regular upgrades, so the
>>maintainer may not wish to incorporate all the changes.
>
>I don't personally like this patch much
(Oh, and one of the things liloconfig theoretically does is check
your lilo.conf for obsolete options, so we should definitely run it on
upgrades.)
>I'll try to come up with a patch to liloconfig that fits my personal
>conception of the Right Thing :->
Well, here it is. It's quite a significant change, however (the
diff is nearly 100 lines longer than the original file!) so it obviously
needs rather more testing than I've given it.
>From the changelog at the top:
# - added error checking of various system calls
# - added $DEBUG switch and pulled lilo.conf and fstab filenames
# out into config variables.
# - turned on Perl's -w switch and use strict subs/refs
# - now does examination of current situation up front, separated
# from the logic of what we do in various situations.
# - added check for special marker in /etc/fstab that indicates that we
# are configuring the base filesystem and shouldn't actually do anything.
In particular, the first of these is a significant bug that should
be fixed in the script in any case. The 2nd, 3rd and 4th are just
cleanups to make it more comprehensible and easier to test and debug.
[I have attempted not to gratuitously change things to my personal
Perl style, though :->].
NB: I assume use strict and -w work in the cut down perl-base
(seems plausible since strict.pm is in the .deb) -- is
there any documentation on what you can and can't do with that?
On a related note, there are probably some perl-5-isms in there;
is that Allowed, or must we cater for ancient perl4?
The fifth changelog entry is the bug we were trying to fix :->
I've opted for checking /etc/fstab for a magic line that reads
"# UNCONFIGURED FSTAB FOR BASE SYSTEM".
If the base-system configuration can arrange for fstab to contain a line
like that when liloconfig is run, it will exit successfully without trying
to run lilo or anything similarly drastic. Other comments in fstab
will be ignored, so feel free to add other lines stating that that
comment is magic and should not be removed or altered.
I've also come across what looks like a bug in the control flow of
the script. If we're configuring a fresh system, we say:
You must do three things to make the Linux system boot from the
hard disk. Install a partition boot record, install a master boot
record, and set the partition active. You'll be asked to perform
each of these tasks. You may skip any or all of them, and perform
them manually later on.
However, if the user says 'yes' to the first 'install a partition
boot record' question, we then never go on to ask the other questions:
we always exit immediately after running LILO... Either the text
we print or the actions we do must be wrong, here.
I haven't tried to fix this; I've just put a FIXME tag into the source.
[I have raised this as a separate bug report against lilo.]
The patch to liloconfig is appended.
Peter Maydell
===begin===
--- liloconfig.dist Wed Nov 24 22:18:06 1999
+++ liloconfig Sun Nov 28 00:07:46 1999
@@ -1,4 +1,4 @@
-#!/usr/bin/perl
+#!/usr/bin/perl -w
#
# /usr/sbin/liloconfig configure lilo automatically
#
@@ -10,19 +10,86 @@
# - updated the template to produce a more helpfull (commentwise)
# resulting lilo.conf
#
+# Updated on 1999/11/24 -- Peter Maydell <pmaydell@chiark.greenend.org.uk>
+# - added error checking of various system calls
+# - added $DEBUG switch and pulled lilo.conf and fstab filenames
+# out into config variables.
+# - turned on Perl's -w switch and use strict subs/refs
+# - now does examination of current situation up front, separated
+# from the logic of what we do in various situations.
+# - added check for special marker in /etc/fstab that indicates that we
+# are configuring the base filesystem and shouldn't actually do anything.
+
# This script is from Bruce's debian.postinst and need to be more
# intelligent. It should be possible to install lilo in the MBR, too.
#
+use strict 'subs';
+use strict 'refs';
+# use strict 'vars' falls over on all the global variables :->
+
+# Set this to 1 to disable all commands that do things to the
+# hard disk (ie actually running lilo). Note that we still write
+# to $LILOCONF, so you should also tweak that to get a 'safe' test
+# environment.
+#$DEBUG=1;
+
+# Various files we access
+$LILOCONF='/etc/lilo.conf';
+$FSTAB='/etc/fstab';
+
$|=1;
-if ( ! (-f "/etc/fstab") ) {
- print "Huh, bad karma: file /etc/fstab is missing!\n";
- exit(1);
-}
+# Print a banner now, to give the user something to look at while
+# we ferret around in the fstab...
print "LILO, the LInux LOader, sets up your system to boot Linux directly\n";
print "from your hard disk, without the need for a boot floppy.\n";
+# First we analyse the setup and set variables appropriately
+$fstab_broken = 1; # is there a valid /etc/fstab? Assume not and prove otherwise.
+$liloconf_exists = 0; # is there a preexisting lilo.conf with a non-commented out line?
+$liloconf_incompatible = 0; # does lilo.conf use options not valid for this version of lilo?
+$configuring_base = 0; # are we configuring the 'base' filesystem (special case)
+# We also set $device, $disk, $partition (assuming fstab_broken == 0)
+
+if (-f $FSTAB) {
+ # Parse fstab for the root partition...
+ open(FSTAB, "<$FSTAB") or die "liloconfig: couldn't open $FSTAB: $!\n";
+ while (<FSTAB>) {
+ # Check for a magic string which indicates that we are configuring
+ # the base filesystem and not a real machine...
+ $configuring_base = 1 if /^# UNCONFIGURED FSTAB FOR BASE SYSTEM/;
+ next if /^#/; # ignore comment lines
+ ($device,$filesystem) = split(/[ \t]+/);
+ # Stop if we found the root device...
+ if ($filesystem eq '/') {
+ $fstab_broken = 0;
+ last;
+ }
+ }
+ close(FSTAB) or die "liloconfig: couldn't close $FSTAB: $!\n";
+}
+
+if (!$fstab_broken) {
+ # Valid device/filesystem pair, parse them
+ $disk = $device;
+ $disk =~ s/[0-9]+$//; # turn '/dev/hda4' into '/dev/hda'...
+ $partition = $device;
+ $partition =~ s/$disk$//; # ...and '4'
+}
+
+# Check for an existing lilo.conf with some non-comment lines in it...
+system("grep -qsv '^#' $LILOCONF");
+# Exit status is 0 iff lilo.conf exists and contains at least one non-comment line.
+if ($? == 0) {
+ $liloconf_exists = 1;
+ $liloconf_incompatible = &compatibility_check();
+}
+
+##########################################################
+# Boilerplate arrays used to produce an initial lilo.conf
+##########################################################
+
@header = (
"# Generated by liloconfig\n",
"\n",
@@ -83,6 +150,10 @@
"\n"
);
+####################
+# Utility functions
+####################
+
sub asky {
print @_,"? [Yes] ";
$answer=<STDIN>;
@@ -95,40 +166,93 @@
return ( $answer =~ /^[yY].*/ );
}
-system('grep -qv "^#" /etc/lilo.conf');
-if ( $? == 0 ) {
+sub compatibility_check {
+ # Check a lilo.conf for options which are not compatible
+ # with the current version of lilo, and return 1 if any
+ # incompatible usages are found.
+ # This currently just checks for use of the any_* loaders.
+ system("egrep '^[^#]*any_' $LILOCONF");
+ return 1 if ($? == 0);
+ return 0;
+}
+
+sub safe_system {
+ # Works like system(), but just echoes the command that would
+ # be run if $DEBUG is 1.
+ if ($DEBUG) {
+ print "[Would have run: ", join(' ', @_), "]\n";
+ $? = 0;
+ } else {
+ system(@_);
+ }
+}
+
+#####################################
+# Actual work is done below here...
+#####################################
+
+# Debian's 'base' filesystem is a special case -- it's prebuilt
+# by installing and configuring packages into a subdirectory,
+# which is then made into a tarball, which is then used to
+# make the initial filesystem for a fresh Debian install.
+# Thus we can't actually run LILO now, because we know nothing
+# of the disk layout. That will be done as part of the install
+# process.
+if ($configuring_base) {
+ print <<EOT;
+
+Hmm. I think you're configuring the base filesystem, and I'm therefore
+simply going to exit successfully without trying to actually configure
+LILO properly. If you're not doing that, this is an important bug
+against Debian's lilo package, and should be reported as such...
+EOT
+ exit(0);
+}
+
+if ($fstab_broken) {
+ print <<EOT;
+
+WARNING!
+Your $FSTAB configuration file appears to either not contain an entry
+for the root filesystem, or it's missing completely! This generally
+means that your system is very badly broken. Configuration of LILO
+will be aborted; you should try to repair the situation and then
+run /usr/sbin/liloconfig again to retry the configuration process.
+EOT
+ exit(1);
+}
+
+if ($liloconf_exists) {
# Trust and use the existing lilo.conf.
# FIX: If the current lilo.conf installs a master boot record, ask
# to edit it to a partition boot record and install the master boot
# record to chain to that.
print "\n";
- print "You already have a LILO configuration in the file /etc/lilo.conf\n";
+ print "You already have a LILO configuration in the file $LILOCONF\n\n";
+ print "Checking your $LILOCONF for incompatible options...\n\n";
+ if ($liloconf_incompatible) {
+ print "WARNING: You have an old incompatible Configuration Line\n";
+ print " Read the File /usr/doc/lilo/INCOMPAT.gz and rerun\n";
+ print " /sbin/lilo to write the changes to your boot sectors\n\n";
+ exit(1);
+ }
+
if ( &asky("Install a boot block using your current LILO configuration") ) {
- print "\nchecking your /etc/lilo.conf for incompatible options...\n\n";
- system('egrep "^[^#]*any_" /etc/lilo.conf');
- if ( $? == 0 ) {
- print "WARNING: You have an old incompatible Configuration Line\n";
- print " Read the File /usr/doc/lilo/INCOMPAT.gz and restart\n";
- print " /sbin/lilo to write the changes to your boot sectors\n\n";
- exit(1);
- } else {
- print "WARNING: Even if lilo runs successfully, see /usr/doc/lilo/INCOMPAT.gz\n";
- print " for changes in the usage of the /etc/lilo.conf file.\n";
- print " If needed: edit /etc/lilo.conf and rerun '/sbin/lilo -v'\n\n";
- }
- print "Running lilo\n";
- system("/sbin/lilo -v");
+ print "WARNING: Even if lilo runs successfully, see /usr/doc/lilo/INCOMPAT.gz\n";
+ print " for changes in the usage of the $LILOCONF file.\n";
+ print " If needed: edit $LILOCONF and rerun '/sbin/lilo -v'\n\n";
+ print "Running lilo...\n";
+ &safe_system("/sbin/lilo -v");
if ( $? == 0 ) {
exit(0);
}
- print "\nERROR: correct /etc/lilo.conf manually and rerun /sbin/lilo\n\n";
+ print "\nERROR: correct $LILOCONF manually and rerun /sbin/lilo\n\n";
exit(1);
- }
- else {
+ } else {
print "\n";
if ( &askn("Wipe out your old LILO configuration and make a new one") ) {
- print "Saving configuration in /etc/lilo.conf.OLD\n";
- rename("/etc/lilo.conf", "/etc/lilo.conf.OLD");
+ print "Saving configuration in $LILOCONF.OLD\n";
+ rename($LILOCONF, "$LILOCONF.OLD") or die "liloconfig: couldn't save old $LILOCONF as $LILOCONF.OLD: $!\n";
}
else {
print "No changes made.\n";
@@ -137,25 +261,8 @@
}
}
-# Create a lilo.conf if one doesn't exist.
-open(FSTAB, "</etc/fstab");
-while ( $line=<FSTAB> ) {
- if ( $line =~ m/^\#/ ) {
- next;
- }
- ($device,$filesystem)=split(/[ \t]+/, $line);
- if ( $filesystem =~ m:^/$: ) {
- last;
- }
-}
-if ( ! $filesystem =~ m:^/$: ) {
- exit(0);
-}
-
-$disk = $device;
-$disk =~ s/[0-9]+$//;
-$partition = $device;
-$partition =~ s/$disk//;
+# ASSERT: that we get here only if there is no lilo.conf or the user
+# asked us to wipe out the old one...
print "\n";
print "You must do three things to make the Linux system boot from the\n";
@@ -167,11 +274,18 @@
if ( &asky("Install a partition boot record to boot Linux from ", $device)) {
print "Creating small lilo.conf and running lilo.\n";
umask(022);
- open(CONF, ">/etc/lilo.conf");
- chown(0, 0, "/etc/lilo.conf");
+ open(CONF, ">$LILOCONF") or die "Couldn't open $LILOCONF for writing: $!\n";
+ if (!chown(0, 0, "$LILOCONF")) {
+ die "Couldn't make $LILOCONF owned by root.root: $!\n" unless $DEBUG;
+ print "Oops, couldn't make $LILOCONF owned by root.root. Since you\n";
+ print "have set the DEBUG flag, I'm going to assume this is because\n";
+ print "you're running liloconfig as a normal user, and continue anyway.\n";
+ }
print CONF @header, "boot=".$device, "\n", @rootheader, "root=".$device, "\n", @boilerplate;
- close(CONF);
- system("/sbin/lilo") || exit(0);
+ close(CONF) or die "Couldn't close $LILOCONF: $!\n";
+ # FIXME : surely we want to continue on to the next stage
+ # (install MBR) in some cases? Currently we don't do that...
+ safe_system("/sbin/lilo") || exit(0);
print "\nERROR: correct /etc/lilo.conf manually and rerun /sbin/lilo\n\n";
exit(1);
}
@@ -183,7 +297,11 @@
if ( &askn("Install a master boot record on ", $disk) ) {
print "Installing MBR on $disk\n";
- system("dd if=/boot/mbr.b of=$disk bs=446 count=1");
+ safe_system("dd if=/boot/mbr.b of=$disk bs=446 count=1");
+ if ($? != 0) {
+ print "ERROR: dd failed! Your system may not be bootable.\n";
+ exit(1);
+ }
}
print "\n";
print "The master boot record will boot the active partition.\n";
@@ -195,7 +313,11 @@
if ( &asky("Make ", $device, " the active partition") ) {
print "Activating Partition $partition on disk $disk.\n";
- system("/sbin/activate $disk $partition");
+ safe_system("/sbin/activate $disk $partition");
+ if ($? != 0) {
+ print "ERROR: activate failed! Your system may not be bootable.\n";
+ exit(1);
+ }
}
else {
print "\n";
===endit===
Reply to: