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

Show all issues

Why is it returns silently instead of exiting with an error message?

1. 

   Toomas Soome Dec. 7, 2018, 6:48 p.m.

   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. 

   Gergő Mihály Doma Dec. 9, 2018, 2:58 a.m.

   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. 

   Toomas Soome Dec. 9, 2018, 10:19 a.m.

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

Not for this issue.

Duplicated endian conversion macros.

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

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

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

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

Same (+ next 3 line).

"Space followed by tab" nits from the past.

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

Space followed by tab.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same.

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

Same + in the next line: convert 6 space to 1 tab.

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

Same.

C-style issues.

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

Missing braces.

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

Show all issues

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.

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

Show all issues

Missing braces.

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

Show all issues

Wrong indentation.

Man page issues.

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

Show all issues

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. 

   Toomas Soome Dec. 11, 2018, 8:44 a.m.

   noone is daring to touch those license lines. I'm certainly not going to:)

2. 

   Gergő Mihály Doma Dec. 12, 2018, 8:28 a.m.

   Ok, this should be part of this issue: Bug #1216: long lines in man pages
   
   (other related: Bug #9103: opengroup acknowledgement should be properly formatted in man pages)

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

Show all issues

Double space between "an" and "entry".

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

Show all issues

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

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

Show all issues

Missing io_mboot parameter here.

1. 

   Toomas Soome Dec. 11, 2018, 1:46 p.m.

   oops:)

Ship It!