6086 add install bootblock option for bootadm

Review Request #79 — Created July 26, 2015 and submitted

tsoome
illumos-gate
6086
78, 80
85, 89, 90
b5cd118...
general
6086 add install bootblock option for bootadm
  1. no output from defaults. this is because the grub capfiles check does not provide any verbose messages - perhaps should add some?

root@testoi:~# bootadm install-bootloader -v
root@testoi:~#

  1. this is normal installgrub output with version check disabled. the default locations are used for stage files. note: this is whole disk setup, so libbe is enforcing MBR update. note: -F in installgrub is in fact useless as our grub does not use embedded versioning, but included anyhow for sake of completeness.

root@testoi:~# bootadm install-bootloader -vf
Command: "/sbin/installgrub -F -m -f //boot/grub/stage1 //boot/grub/stage2 /dev/rdsk/c3t3d0s0"
Output:
stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
stage1 written to partition 0 sector 0 (abs 256)
stage1 written to master boot sector

  1. install to pool "tank" using current root as source for stage files
    root@testoi:/# bootadm install-bootloader -v -P tank -R /
    Command: "/sbin/installgrub -m -f //boot/grub/stage1 //boot/grub/stage2 /dev/rdsk/c3t5d0s0"
    Output:
    stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
    stage1 written to partition 0 sector 0 (abs 256)
    stage1 written to master boot sector

  2. mount previous BE to /a and use it as altroot
    root@testoi:/# beadm mount openindiana-1 /a
    Mounted successfully on: '/a'
    root@testoi:/# bootadm install-bootloader -v -P tank -R /a
    Command: "/sbin/installgrub -m -f /a/boot/grub/stage1 /a/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
    Output:
    stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
    stage1 written to partition 0 sector 0 (abs 256)
    stage1 written to master boot sector

  3. specify only pool
    root@testoi:~# bootadm install-bootloader -v -P tank
    Command: "/sbin/installgrub -m -f /tmp/.be.ZfaGnc/boot/grub/stage1 /tmp/.be.ZfaGnc/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
    Output:
    stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
    stage1 written to partition 0 sector 0 (abs 256)
    stage1 written to master boot sector

  4. ok, those tests show tempnames for mountpoints, is it actually correbt mountpoint? one way to make sure would to run for example with truss, but for this test I incremented version in BE capability file
    root@testoi:~# beadm mount tank-1 /a
    Mounted successfully on: '/a'
    root@testoi:~# vi /a/boot/grub/capability
    root@testoi:~# grep VERSION /a/boot/grub/capability
    VERSION=26
    root@testoi:~# grep VERSION /tank/boot/grub/capability
    VERSION=25
    root@testoi:~# beadm umount /a
    Unmounted successfully
    root@testoi:~# bootadm install-bootloader -v -P tank
    Command: "/sbin/installgrub -m -f /tmp/.be.kjaqqc/boot/grub/stage1 /tmp/.be.kjaqqc/boot/grub/stage2 /dev/rdsk/c3t5d0s0"
    Output:
    stage2 written to partition 0, 289 sectors starting at 1024 (abs 1280)
    stage1 written to partition 0 sector 0 (abs 256)
    stage1 written to master boot sector
    root@testoi:~# grep VERSION /tank/boot/grub/capability
    VERSION=26

  • 0
  • 0
  • 12
  • 0
  • 12
Description From Last Updated
tsoome
tsoome
xenol
  1. Ship It!
  2. 
      
jeffpc
  1. 
      
    1. also, after this update, dmake was clean, dmake lint was clean and git-pbchk did not complain.

  2. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
     
     

    cstyle... != NULL

    1. yep, and not only on this line, there were few more instances.

  3. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
     
     

    Remove superfluous {}. (Or add them to the else - just be consistent)

    Actually, these few lines could be written more clearly. E.g.,

    if (ret)
            ret = BAM_ERROR;
    else
            ret = be_installboot(nvl) ? BAM_ERROR : 0;
    
    1. good suggestion, updated.

  4. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 3)
     
     

    Why not: return (f());

    You could even get rid of ret completely, and make the if: if (check_subcmd_and_options(...) == BAM_ERROR)

    1. updated, yes its more clean this way.

  5. usr/src/man/man1m/bootadm.1m (Diff revision 3)
     
     

    This seems unrelated to what's in the bug report.

    1. fixed. I'll open separate bug for those 2 options.

  6. usr/src/man/man1m/bootadm.1m (Diff revision 3)
     
     

    This seems unrelated to what's in the bug report.

  7. 
      
tsoome
jeffpc
  1. Ship It!
  2. 
      
rm
  1. 
      
    1. Just a general note on all this, while it generally looks good, I think the manual page can be cleaned up quite a lot to actually reflect what's going on and tightened up. At least, as someone who nominally administers some systems, I was confused by a lot of what it was referring to.

  2. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    What is defaults in this context? How is this similar to beadm activate? Is running this command actually going to cause my current boot environment to change? If so, why is that?

    In general, this could use a bit more information discussing when you would want to run this.

  3. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    This is the first time in the manual page that boot blocks are mentioned. They need to be defined and explained in a way that an administrator can relate it to the act of installing the boot loader.

    I'd instead initially just describe this in the already defined and generally known terms, that of installing the boot loader.

    Afterwards, I'd go into the specific details of what this is going to do on each platform. What parts of what disks are actually going to be touched. Is it the MBR, something related to the EFI/GPT, etc.

  4. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    Isn't bootblock really a SPARCism at the moment? Regardless, feels like it should be couched in terms of what the administrators know and if we're going to use bootblock, it should be discussed in detail and explained in the DESCRIPTION.

  5. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    Presumably there aren't 'bootlock's. I think this would be better by describing what always happens on x86 and SPARC up in the initial discussion of the command and then only indicating the side effects that -M has here, rather than hiding a lot of the x86 semantics of this in the -M option.

  6. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    I'd change the wording and fix up the grammar here. I'd write this as:

    'In an install-bootloader operation, the bootloader is installed on the devices of the specified ZFS pool. If the -P option is not specified, then the current ZFS system boot pool is used instead.'

    This is also the kind of information (the default) that I'd put up in the initial discussion of the command.

  7. usr/src/man/man1m/bootadm.1m (Diff revision 4)
     
     

    I'd again not refer to boot block files here, but I'd instead phrase this as:

    'In an install-bootloader operation, the boot loader is still installed on the specified pool; however, the bootloader itself will come from the alternate root.

  8. 
      
tsoome
tsoome
tsoome
tsoome
igork
  1. Ship It!
  2. 
      
jeffpc
  1. 
      
  2. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 8)
     
     

    Minor nit: these can be const

  3. 
      
tsoome
jeffpc
  1. Ship It!
  2. 
      
xenol
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...