[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: (ITS#3839) pedantic and format troubles

h.b.furuseth@usit.uio.no wrote:
>  Full_Name: Hallvard B Furuseth Version: HEAD OS: URL: Submission
>  from: (NULL) ( Submitted by: hallvard
>  gcc -pedantic -Wformat -Wno-format-extra-args isn't nice to the
>  config stuff:
>  ===
>  ConfigTable.arg_item is set to (void*)integer or (void*)function when
>  arg_type & (ARG_MAGIC | ARG_OFFSET).  This is not portable - function
>  and void pointers may have different sizes, or pointers may be
>  normalized in some way after being assigned integers, etc.

I feel this concern is unwarranted for a number of reasons:
  1) re int vs pointer - any supposed normalization must be reversible. 
We are not converting integers to pointers or vice versa; that is, we do 
not store an integer and later attempt to use it as a pointer. As such, 
if the value we read from this variable is not the same as the value we 
stored, the compiler is broken.
  2) there is a large body of code that depends on the 
interchangeability of function pointers and object pointers, thanks to 
APIs like dlsym() and friends. 
The fact that this stuff still works tells me no problem exists. When 
these things stop working, then there's a legitimate issue to fix.

>  This should either be replaced with 3 fields (where only one is set
>  per table entry), or the current field could be set to a pointer to a
>  variable containg the current value.

Feel free to replace this with a union if you wish. I don't see the need 
but also there's no harm if you make this change.

>  Um.  And I guess it's about time to fix the same problem with
>  ldap_pvt_thread_pool_<setkey/getkey>().  They receive function
>  pointers sometimes.  I'll create some static variables to hold these
>  function pointers, and pass them pointers to those variables instead.

Don't bother to preserve the function pointers. The only point in those 
calls is to pass a unique value to distinguish one key from another. It 
is of no interest whether the function pointer is passed *intact* so 
long as it is passed *consistently* in each invocation, so that the same 
value is propagated. The passed pointers are never dereferenced. If you 
don't like using function pointers here, just pass some other unique 
value instead.

>  ===
>  ConfigArgs.lineno is an unsigned long, but a number of routines that
>  can report line numbers (like parse_acl) take an int. So do a lot of
>  printf formats.  I started to fix the formats that are wrong, but
>  maybe it's just as well to use int lineno.

I agree, an int is probably sufficient.

>  A ligher change is
>  to make the char* file/mode args to ldif_open() LDAP_CONST, since
>  they are passed const char*'s from elsewhere.


>  ===
>  Two plain bugs in bconfig.c: Message "%s: (optional) %s overlay
>  \"%s\" configuration failed" gets the char c->argv[1][1] instead of
>  c->argv[1]+1 or something.
>  Message "<%s> cannot be deleted" gets no arg to the %s. Looks like it
>  should be deleted, since it's repeated below.


  -- Howard Chu
  Chief Architect, Symas Corp.       Director, Highland Sun
  http://www.symas.com               http://highlandsun.com/hyc
  Symas: Premier OpenSource Development and Support