11055 loader: zfs reader cstyle cleanup

Review Request #1852 — Created May 21, 2019 and updated

tsoome
illumos-gate
11055
4e08708...
general
11055 loader: zfs reader cstyle cleanup


  • 30
  • 0
  • 4
  • 0
  • 34
Description From Last Updated
Déjà vu... Type of found variable should be bool. domag02 domag02
Format string - argument type mismatch: type of j variable is unsigned int. domag02 domag02
"Never use the boolean negation operator (!) with non-boolean expressions." domag02 domag02
Type of is_new variable (and also the is_newer argument) should be bool. domag02 domag02
0 vs NULL. domag02 domag02
0 vs NULL. domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Optional, drop this if you think it isn't important: buf[0] = '\0'; domag02 domag02
"A blank line should always be used ... After local variable declarations." domag02 domag02
Type of first variable should be bool. domag02 domag02
is_newer should be bool. domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Is it safe to expect that only two types of ZAP can exist? See the implementation of zap_lookup() in this ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Is it safe to expect that only two types of ZAP can exist? domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Possible buffer overrun: component array can hold up to 256 bytes, but zap_rlookup can write into it at most 65535 ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Is it safe to expect that only two types of ZAP can exist? domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
Is it safe to expect that only two types of ZAP can exist? domag02 domag02
"If one arm of an if-else statement contains braces, all arms should contain braces." domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... domag02 domag02
rejohnst
  1. Ship It!
  2. 
      
domag02
  1. All of my findings: some of them unrelated to c-style.
    1. Thanks!

      Note, I am keeping this list "open", I want to keep the initial cleanup very simple, just to make the pbchk happy etc, also I need to get few functional changes integrated first:D

      I think you have spotted real bugs there too and we will need to address them with separate patches anyhow.

  2. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     
    Unnecessary blank line.
  3. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Never use the boolean negation operator (!) with non-boolean expressions."
    Should be:

            if (memcmp(name, pairname, namelen) == 0 && type == pairtype) {
    
  4. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    This if...else ladder can be rewritten as a switch...case statement, but is it worth doing?
    Example:

                switch (type) {
                case DATA_TYPE_UINT64:
                    xdr_uint64_t(&p, (uint64_t *)valuep);
                    return (0);
                    break;
                case DATA_TYPE_STRING: {
                    int len;
    
                    xdr_int(&p, &len);
                    (*(const char **)valuep) = (const char *)p;
                    return (0);
                    break;
                }
                case DATA_TYPE_NVLIST:
                case DATA_TYPE_NVLIST_ARRAY:
                    (*(const unsigned char **)valuep) =
                        (const unsigned char *)p;
                    return (0);
                    break;
                default:
                    return (EIO);
                    break;
                }
    
  5. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Déjà vu...
    Type of found variable should be bool.

  6. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

                printf(" = 0x%" PRIx64 "\n", val);
    
  7. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Format string - argument type mismatch: type of j variable is unsigned int.

  8. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Never use the boolean negation operator (!) with non-boolean expressions."

  9. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    0 vs NULL.
    This line should be:

        return (NULL);
    
  10. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Yet another "malloc + memset => calloc" issue?

  11. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Type of is_new variable (and also the is_newer argument) should be bool.

  12. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    switch instead of if...else ladder?

  13. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    0 vs NULL.

  14. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    0 vs NULL.

  15. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

    Example:

    #include <sys/tem_impl.h>
    .
    .
    .
        char line[TEM_DEFAULT_COLS];
    
  16. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

  17. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Optional, drop this if you think it isn't important:

        buf[0] = '\0';
    
  18. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "A blank line should always be used ... After local variable declarations."

  19. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Type of first variable should be bool.

  20. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    is_newer should be bool.

  21. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    How about using format constans here instead of casting?

  22. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  23. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

    Example:

    #define ZFS_SECSIZE 512
    .
    .
    .
        size = dnode->dn_datablkszsec * ZFS_SECSIZE;
    
  24. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "A blank line should always be used ... After local variable declarations."

  25. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     
    Unusual to see variable declarations almost in the middle of a function.
    1. Yes, unfortunately this is new trend, we see it more and more with openzfs...

  26. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

        size = dnode->dn_datablkszsec * ZFS_SECSIZE;
    
  27. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."
    Example:

    #define ZAP_LE_NAME_BUFSIZE 256
    .
    .
    .
            char name[ZAP_LE_NAME_BUFSIZE], *p;
    
  28. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     
    Is it necessary to preserve this commented-out tracing code?
    And if it is, how about creating a macro for this purpose?
  29. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

        size_t size = dnode->dn_datablkszsec * ZFS_SECSIZE;
    
  30. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Is it safe to expect that only two types of ZAP can exist?

    See the implementation of zap_lookup() in this file.

    Also: Lustre feature ideas - ZFS TinyZAP.

  31. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

        size = dnode->dn_datablkszsec * ZFS_SECSIZE;
    
  32. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

        size_t size = dnode->dn_datablkszsec * ZFS_SECSIZE;
    
  33. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Is it safe to expect that only two types of ZAP can exist?

  34. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

  35. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  36. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Possible buffer overrun: component array can hold up to 256 bytes, but zap_rlookup can write into it at most 65535 bytes.

  37. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

  38. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  39. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  40. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  41. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  42. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  43. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  44. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

            child_dir_zap.dn_datablkszsec * ZFS_SECSIZE);
    
  45. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Is it safe to expect that only two types of ZAP can exist?

  46. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  47. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Instead of casting, format constant can be used here.

  48. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

        size = dir.dn_datablkszsec * ZFS_SECSIZE);
    
  49. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    Is it safe to expect that only two types of ZAP can exist?

  50. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "If one arm of an if-else statement contains braces, all arms should contain braces."

  51. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

  52. usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)
     
     

    "Numerical constants should not be coded directly.
    The #define feature of the C preprocessor should
    be used to assign a meaningful name."

    Example:

    #define BOOT_MAXLINK    10
    .
    .
    .
                if (symlinks_followed > BOOT_MAXLINK) {
    
  53. 
      
tsoome
tsoome
Review request changed

Change Summary:

usr/src/boot/lib/libstand/zfs/zfsimpl.c: 1442: missing space after (void) cast
usr/src/boot/lib/libstand/zfs/zfsimpl.c: 1737: space after cast

Commit:

-9ac02fb709b9f0e6713b3f205540577e1f90b90e
+4e0870808d6fb48d32d7839d3fd280103587fb34

Diff:

Revision 3 (+157 -137)

Show changes

tsoome
  1. Note this one is integrated but I keep it visible for list of issues:)

  2. 
      
Loading...