[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] Migrate PPC from Yaboot to Grub2 for Anaconda



> On Fri, 2012-03-09 at 09:28:06 -0600, David Lehman wrote:
>> On Fri, 2012-03-09 at 08:43 -0600, Mark Hamzy wrote:
>Can we rename this to updatePPCBootList so that common searches for
>ppc-specific stuff will find this? Maybe even updateNVRAMBootList
>instead?

Yes.  Although, if I name it updateNVRAMBootList, will your common search
for ppc-specific stuff find it?  Will everyone think of nvram as a ppc only
search term?

>> +        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.

Okay.

>> +            return
>
>Installation will continue unless you raise an exception -- is this your
>intention?

Yes.  Failure in this function should not abort installation.  The bios
can be canged manually after an install.

>> +            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.

Okay.

>> +            return
>
>Should this be a fatal error? Returning instead of raising an exception
>here means the bootloader installation will continue instead of
>aborting.

Yes.  Failure in this function should not abort installation.  The bios
can be canged manually after an install.

>> +        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.

Okay.

>> +        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?

Yes.  Thanks for catching that.

>> +            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.

Okay.

>> +        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")

I wanted to remind future developers that the specific sub-variables could change.

>>  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.

Yes.  We are removing the need for two boot loaders and moving to only one.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]