10182 dd: print scaled stats

Review Request #1344 — Created Jan. 6, 2019 and submitted

tsoome
illumos-gate
10182
00a54ba...
general
10182 dd: print scaled stats


  • 0
  • 0
  • 26
  • 6
  • 32
Description From Last Updated
citrus
  1. Ship It!
  2. 
      
xenol
  1. 
      
  2. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     

    LGTM, only small nitpicks to be consistent with the style:

    • put a space before obytes
    • remove space between sizeof and the following bracket
    1. This would be against cstyle rules:
      C style:
      dd.c: 1796: missing space between keyword and paren
      dd.c: 1796: space after cast

      The sizeof() is particulary nasty because thats only difference with cstyle in FreeBSD:)

  3. 
      
xenol
  1. Ship It!
  2. 
      
marcel
  1. Ship It!
  2. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    You added missing spaces around & in other cases, so please add it here too.
  3. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    You added missing spaces around & in other cases, so please add it here too.
  4. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    This looks like an indentation issue.
  5. usr/src/cmd/dd/dd.c (Diff revision 1)
     
     
    Is indentation okay here?
  6. 
      
tsoome
domag02
  1. 
      
  2. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
    Missing blanks around the operator.
  3. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  4. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  5. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  6. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  7. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  8. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  9. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  10. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
    Same.
    .
    .
    .
  11. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  12. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  13. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  14. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  15. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     
  16. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     

    if (obc) {
    
  17. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     

    } else {
    
  18. 
      
domag02
  1. 
      
  2. usr/src/cmd/dd/dd.c (Diff revision 2)
     
     

    No need to cast NULL to unsigned char *.

  3. 
      
tsoome
domag02
  1. 
      
    1. Those are all fair remarks, but are going out of scope of this change. To be honest, we should also post cstyle fixes separate from the original bug to keep changes separate. I think it is time for you to take next step from reviewing things - file the issue, create patch and post it for review. If you need any help, just ask, I'll be very glad to help out:)

    2. Thanks, but I don't have a working illumos installation now, so I can't make the necessary build before sending patches to RTI.
  2. usr/src/cmd/dd/dd.c (Diff revisions 1 - 2)
     
     
    Please, remove the blank before the opening square bracket.
  3. usr/src/cmd/dd/dd.c (Diff revisions 1 - 2)
     
     
  4. 
      
domag02
  1. Magic numbers.
  2. usr/src/cmd/dd/dd.c (Diff revisions 2 - 3)
     
     

    ... dup(STDIN_FILENO);
    
  3. usr/src/cmd/dd/dd.c (Diff revisions 2 - 3)
     
     

    ... dup(STDOUT_FILENO);
    
  4. 
      
domag02
  1. 
      
  2. usr/src/cmd/dd/dd.c (Diff revision 3)
     
     

    ..., obytes

  3. 
      
domag02
  1. 
      
    1. Not for this issue.
      Fix argument handling.

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

    Pointless to use BIG here (as a limiting number), if the returned value is casted to unsigned int.

    #include <limits.h>
    .
    .
    .
    ... = (unsigned)number(UINT_MAX);
    
  3. usr/src/cmd/dd/dd.c (Diff revision 3)
     
     
  4. usr/src/cmd/dd/dd.c (Diff revision 3)
     
     
  5. usr/src/cmd/dd/dd.c (Diff revision 3)
     
     
  6. usr/src/cmd/dd/dd.c (Diff revision 3)
     
     

    Almost the same, but now with INT_MAX.

  7. 
      
tsoome
domag02
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...