11089 libsldap: return makes integer from pointer without a cast

Review Request #1875 — Created May 31, 2019 and submitted

tsoome
illumos-gate
11089
35da3f3...
general
In file included from ../common/ns_reads.c:40:0:
../common/ns_reads.c: In function '__ns_ldap_dn2uid':
../common/ns_internal.h:222:10: error: return makes integer from pointer without a cast [-Werror=int-conversion]
   return (retErr); \
          ^
../common/ns_reads.c:4297:3: note: in expansion of macro 'MKERROR'
   MKERROR(LOG_WARNING, *errorp, NS_LDAP_INTERNAL, strdup(errstr),
   ^~~~~~~
cc1: all warnings being treated as errors


  • 0
  • 0
  • 59
  • 0
  • 59
Description From Last Updated
gwr
  1. Looks good, thanks. Sorry for introducing that error.
    What should I check to see why my build didn't show me that?

    1. our sys/null.h does not allow it. But we can not change it yet because my userland NULL pointer fixes are still in queue to be posted for review.

  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

norm
  1. Ship It!
  2. 
      
domag02
  1. Mostly C-style issues.

    I'm late with this review, sorry.
    Should I move these into an another issue?

    1. This size cstyle change should be in separate update IMO.

    2. Ok.
  2. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  3. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of in_quote, is_value, at_least_one and auto_service variables should be boolean_t and should use values B_FALSE and B_TRUE instead of FALSE and TRUE.

    1. I think we should use stdbool.h instead, but even boolean_t is better.

    2. I also thought about that, but the boolean_t type is more widely used in libsldap so I applied the consistency rule:

      More important than the particular coding style used is consistency of coding style. Within a particular
      module, package, or project, a consistent coding style should be used throughout. This is particularly
      important when modifying an existing program; the modifications should be coded in the same style as
      the program being modified, not in the programmer’s personal style, nor necessarily in the style advocated
      by this document.

  4. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    The scope of in_quote and is_value variables can be reduced into this for loop.

  5. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Never use the boolean negation operator (!) with non-boolean expressions."
  6. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab (+ in the next line).
  7. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab (+ in the next line).
  8. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of finished variable should be boolean_t.

  9. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab.
  10. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  11. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  12. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  13. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  14. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  15. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab.
  16. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  17. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  18. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

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

  19. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  20. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  21. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "If one arm of an if-else statement contains braces, all arms should contain braces."
  22. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  23. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  24. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Arguments serverctrls and clientctrls can be marked with __unused (from <sys/ccompile.h>).

  25. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Never use the boolean negation operator (!) with non-boolean expressions."
  26. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    I think this "special" comment can also be removed.
  27. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  28. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    This comment doesn't seems to be valid.
    (cred is an argument of __ns_ldap_list())

  29. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    This can be a for loop:

        for (; uid[i] != '\0'; i++) {
            if (uid[i] == '=') {
                *userDN = strdup(uid);
                return (NS_LDAP_SUCCESS);
            }
        }
    
  30. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Another for loop:

        for (i = 0; uid[i] != '\0' && isdigit(uid[i]); i++)
            ;
    
  31. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Memory leak: filter isn't freed.

  32. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Memory leak: filter isn't freed.

  33. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    I think this "special" comment can also be removed.
  34. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  35. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    This comment doesn't seems to be valid.
    (cred is an argument of __ns_ldap_list())

  36. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Memory leak: filter isn't freed.

  37. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    I think this "special" comment can also be removed.
  38. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  39. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    This comment doesn't seems to be valid.
    (cred is an argument of __ns_ldap_list())

  40. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Memory leak: filter isn't freed.

  41. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    This "special" comment is unnecessary.
  42. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  43. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    This "special" comment is unnecessary.
  44. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  45. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of done variable should be boolean_t.

  46. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    "If the body of a for or while loop is empty, no braces are needed."
    Should be:

            for (max = 1; (send = strchr(++send, SEMITOK)) != NULL; max++)
                ;
    
  47. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  48. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  49. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab (+ 2 more times in this comment).
  50. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Wrong line continuation.
  51. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of found variable should be boolean_t.

  52. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    This should look like:

            if (found)
                break;
    
  53. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Casts should not be followed by a blank, with the exception of function calls whose return values are ignored."
  54. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Casts should not be followed by a blank, with the exception of function calls whose return values are ignored."
  55. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of escape variable should be boolean_t.

  56. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Blanks should never separate unary operators ... from their operands."
  57. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of escape variable should be boolean_t.

  58. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Blanks should never separate unary operators ... from their operands."
  59. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    "Blanks should never separate unary operators ... from their operands."
  60. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

    Type of firstdigit and escape variables should be boolean_t.

  61. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     

        req = (LDAPControl *)malloc(sizeof (LDAPControl));
    .
    .
    .
        req->ldctl_value.bv_len = 0;
        req->ldctl_value.bv_val = NULL;
    

    is the same as:

        req = (LDAPControl *)calloc(1, sizeof (LDAPControl));
    
  62. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab.
  63. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab.
  64. usr/src/lib/libsldap/common/ns_reads.c (Diff revision 1)
     
     
    Space followed-by tab.
  65. usr/src/lib/libsldap/common/ns_reads.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."

    1. this variable is not needed at all, use asprintf().

    2. That was my second thought.
  66. 
      
Loading...