10048 fdisk: use /boot/pmbr if possible

Review Request #1314 — Created Dec. 6, 2018 and submitted

tsoome
illumos-gate
10048
1315
1c07524...
general
10048 fdisk: use /boot/pmbr if possible


  • 0
  • 0
  • 7
  • 1
  • 8
Description From Last Updated
domag02
  1. 
      
  2. usr/src/cmd/fdisk/fdisk.c (Diff revision 1)
     
     
    Why is it returns silently instead of exiting with an error message?
    1. Because this is not fault from fdisk point of view. We do not install bootloader with fdisk, we could even just throw away this read at all, except it is "useful" to fill up the space outside the partitions.

      For boot program install, the only way since 2005 is installgrub and in illumos for last 2 years - installboot.

    2. That's absolutely fine, but I would like to recommend an another solution:

      If the user explicitly specified a master boot file in the command line then
      it would be better to not return silently, if that file cannot be opened.

      Example (snippet):

      .
      .
      .
      #define DEFAULT_MASTER_BOOT_FILE    "/boot/pmbr"
      .
      .
      .
      
          /*
           * If the master boot file hasn't been specified, try to use our
           * default.
           */
          if (io_mboot == NULL) {
              io_mboot = DEFAULT_MASTER_BOOT_FILE;
      
              if ((mDev = open(io_mboot, O_RDONLY, 0666)) == -1) {
                  /*
                   * This is not fault from fdisk point of view.
                   * We do not install bootloader with fdisk,
                   * so just throw away this error and return.
                   */
                  return;
              }
          } else {
              errno = 0;
              if ((mDev = open(io_mboot, O_RDONLY, 0666)) == -1) {
                  int errno_sav = errno;
      
                  (void) fprintf(stderr,
                      "fdisk: Cannot open master boot file '%s' : %s.\n",
                      io_mboot, strerror(errno_sav));
                  exit(1);
              }
          }
      
      ...
      
    3. I did add this one, this time. Next time, just write it down and post it for the review and RTI. Not joking at all. Those are exactly those small updates which do change the world.

      we do not need to save errno there btw, because strerror() is called before fprintf() and the value of the errno will not change after open().

  3. 
      
domag02
  1. Not for this issue.
    Duplicated endian conversion macros.

    1. Nice find, please open the issue and post the review, lets get them fixed:)

  2. usr/src/cmd/fdisk/fdisk.c (Diff revision 1)
     
     

    These endian conversion macros aren't needed, instead use LE_* macros from <sys/byteorder.h> (wich is already included).

  3. usr/src/cmd/fdisk/fdisk.c (Diff revision 1)
     
     

    Replace lel with LE_32 (+ in the next line).

  4. usr/src/cmd/fdisk/fdisk.c (Diff revision 1)
     
     
    Same (+ next 3 line).
  5. 
      
tsoome
domag02
  1. "Space followed by tab" nits from the past.
  2. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     

    Space followed by tab.

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

    Same.

  4. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  5. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  6. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  7. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  8. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  9. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  10. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  11. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  12. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same + in the next line: convert 6 space to 1 tab.
  13. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Same.
  14. 
      
domag02
  1. C-style issues.
  2. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Missing braces.
  3. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     

    Missing braces.
    From 'C Style and Coding Standards for SunOS' :

    In either case, if one arm of an if-else statement contains braces, all arms should contain braces.

  4. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Missing braces.
  5. usr/src/cmd/fdisk/fdisk.c (Diff revision 2)
     
     
    Wrong indentation.
  6. 
      
tsoome
domag02
  1. Man page issues.
  2. usr/src/man/man1m/fdisk.1m (Diff revision 3)
     
     
    The lines of licensing information are too long (it seems like this is true for other man pages too).
    Exception: "usr/src/man/man1has/ex.1has" contains a better formatted CDDL information.
    
    Is it permitted to replace this with that can be found in "/usr/src/prototypes/prototype.man", or it should be preserved as-is (for ex.: with the same dead url)?
    1. noone is daring to touch those license lines. I'm certainly not going to:)

  3. usr/src/man/man1m/fdisk.1m (Diff revision 3)
     
     
    Double space between "an" and "entry".
  4. 
      
tsoome
citrus
  1. 
      
  2. usr/src/cmd/fdisk/fdisk.c (Diff revision 4)
     
     

    This is not a fault from fdisk's* point of view.

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

    Missing io_mboot parameter here.

  4. 
      
tsoome
citrus
  1. Ship It!
  2. 
      
tsoome
tsoome
tsoome
Review request changed

Status: Closed (submitted)

Loading...