On Fri, 2012-03-09 at 08:43 -0600, Mark Hamzy wrote:
> This is a patch to switch ppc from using Yaboot to using Grub2. Because the default
> terminal size is 80x24 and Open Firmware's default size is a little larger than that
> so repaints will mess up the display. Therefore, tell grub to configure
> 'terminfo -g 80x24 ofconsole'. We do this with a new GRUB_TERMINFO variable and
> enablement script in grub.
Looks good in general, but I have several comments below.
>
> ---
> pyanaconda/bootloader.py | 72 +++++++++++++++++++++++++++++++++++++++++++++-
> pyanaconda/platform.py | 4 +-
> 2 files changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/pyanaconda/bootloader.py b/pyanaconda/bootloader.py
> index 0e759f5..792d1e9 100644
> --- a/pyanaconda/bootloader.py
> +++ b/pyanaconda/bootloader.py
> @@ -1794,7 +1794,7 @@ class GRUB2(GRUB):
> def install(self):
> # XXX will installing to multiple drives work as expected with GRUBv2?
> for (stage1dev, stage2dev) in self.install_targets:
> - args = ["--no-floppy", self.grub_device_name(stage1dev)]
> + args = ["--no-floppy", stage1dev.path]
> if stage1dev == stage2dev:
> # This is hopefully a temporary hack. GRUB2 currently refuses
> # to install to a partition's boot block without --force.
> @@ -2002,6 +2002,76 @@ class IPSeriesYaboot(Yaboot):
> log.info("Updated PPC boot list with the command: nvram --update-config %s" % update_value)
>
>
> +class IPSeriesGRUB2(GRUB2):
> +
> + # GRUB2 sets /boot bootable and not the PReP partition. This causes the Open Firmware BIOS not
> + # to present the disk as a bootable target. If stage2_bootable is False, then the PReP partition
> + # will be marked bootable. Confusing.
> + stage2_bootable = False
> +
> + #
> + # installation
> + #
> +
> + def install(self):
> + self.updatePowerPCBootList()
> +
> + super(IPSeriesGRUB2, self).install()
> +
> + def updatePowerPCBootList(self):
Can we rename this to updatePPCBootList so that common searches for
ppc-specific stuff will find this? Maybe even updateNVRAMBootList
instead?
> +
> + log.debug("updatePowerPCBootList: self.stage1_device.path = %s" % self.stage1_device.path)
> +
> + buf = iutil.execWithCapture("nvram",
> + ["--print-config=boot-device"],
> + stderr="/dev/tty5")
> +
> + if len(buf) == 0:
> + log.error ("FAIL: nvram --print-config=boot-device")
I'd personally prefer a more generic message here like "failed to
determine nvram boot device". Remember that program.log will contain a
record of every command run, complete with arguments and output. And,
since it's already going to have "ERROR" at the beginning of the line in
the log you could certainly leave out the "FAIL:" part. Likewise for the
other log statements in this method.
> + return
Installation will continue unless you raise an exception -- is this your
intention?
> +
> + boot_list = buf.strip().split()
> + log.debug("updatePowerPCBootList: boot_list = %s" % boot_list)
> +
> + buf = iutil.execWithCapture("ofpathname",
> + [self.stage1_device.path],
> + stderr="/dev/tty5")
> +
> + if len(buf) > 0:
> + boot_disk = buf.strip()
> + log.debug("updatePowerPCBootList: boot_disk = %s" % boot_disk)
It will be easy to see what this value is by looking in program.log, so
this log statement can go.
> + else:
> + log.error("FAIL: ofpathname %s" % self.stage1_device.path)
> + return
Should this be a fatal error? Returning instead of raising an exception
here means the bootloader installation will continue instead of
aborting.
> +
> + # Place the disk containing the PReP partition first.
> + # Remove all other occurances of it.
> + boot_list = [boot_disk] + filter(lambda x: x != boot_disk, boot_list)
> +
> + log.debug("updatePowerPCBootList: updated boot_list = %s" % boot_list)
Just a few lines down this information will get entered into program.log
as the command line for nvram --update-config so you don't need to log
it here separately.
> +
> + update_value = "boot-device=%s" % " ".join(boot_list)
Does the argument need to be quoted for the case where boot_list
contains multiple devices/strings?
> +
> + rc = iutil.execWithRedirect("nvram", ["--update-config", update_value],
> + stdout="/dev/tty5", stderr="/dev/tty5")
> + if rc:
> + log.error("FAIL: nvram --update-config %s" % update_value)
> + else:
> + log.info("Updated PPC boot list with the command: nvram --update-config %s" % update_value)
This second log statement is not needed. It will already be logged in
program.log.
> +
> + #
> + # In addition to the normal grub configuration variable, add one more to set the size of the
> + # console's window to a standard 80x24
> + #
> + def write_defaults(self):
> + super(IPSeriesGRUB2, self).write_defaults()
> +
> + defaults_file = "%s%s" % (ROOT_PATH, self.defaults_file)
> + defaults = open(defaults_file, "a+")
> + defaults.write("GRUB_TERMINFO=\"%s -g %dx%d %s\"\n" % ("terminfo", 80, 24, "console"))
You might as well just do:
defaults.write("GRUB_TERMINFO=\"terminfo -g 80x24 console\"\n")
> + defaults.close()
> +
> +
> class MacYaboot(Yaboot):
> prog = "mkofboot"
> can_dual_boot = True
> diff --git a/pyanaconda/platform.py b/pyanaconda/platform.py
> index 536a849..cfadbb3 100644
> --- a/pyanaconda/platform.py
> +++ b/pyanaconda/platform.py
> @@ -231,7 +231,7 @@ class MacEFI(EFI):
>
> class PPC(Platform):
> _ppcMachine = iutil.getPPCMachine()
> - _bootloaderClass = bootloader.Yaboot
> + _bootloaderClass = bootloader.GRUB2
> _boot_stage1_device_types = ["partition"]
Is this necessary? It makes the default bootloader on ppc grub2. I
realize it may not matter if all the ppc platforms that actually get
used have their own bootloader class definition, but if grub2 can't
actually boot most ppcs maybe we should leave this as yaboot for now.
>
> @property
> @@ -239,7 +239,7 @@ class PPC(Platform):
> return self._ppcMachine
>
> class IPSeriesPPC(PPC):
> - _bootloaderClass = bootloader.IPSeriesYaboot
> + _bootloaderClass = bootloader.IPSeriesGRUB2
> _boot_stage1_format_types = ["prepboot"]
> _boot_stage1_max_end_mb = 10
> _boot_prep_description = N_("PReP Boot Partition")
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
