[Confuse-devel] Re: Dangerous assert() usage, parsed value count and stdbool.h compatibility

[Martin Hedenfalk]

3 nov 2010 kl. 16.56 skrev Jens Rehsack:

> Hi Martin, hi Nathan,

Hello,

> because confuse-devel@ still denies mails from non-members, I continue
> to cc you.
> 
> I currently stepped into several "issues" in libConfuse - but before
> spending effort on
> fixing them, I'd like to read the maintainers/authors opinion.
> 
> 1) I've seen during some code checking, that only assert() is used to prove 
> for
>   NULL-pointers. But assert is expanded to a no-op when NDEBUG is defined,
>   which might be default for some production compiles.
>   Is it intended that in such circumstances (e.g. typo in opt-name when 
> calling
>   cfg_set_validate_func) libconfuse simply segfaults?

If -DNDEBUG is used (why would anyone want to disable error checking!?), I 
guess it's deliberate. IMHO, the behaviour of having -DNDEBUG disable assert 
was a mistake.


>   I suggest a macro 'assure(check, error-value)' which returns
> error-value if the
>   test fails (in case of -DNDEBUG). Probably abort() after displaying an error
>   could be suitable, too (even I this is definitively not a preferred way 
> ...).

My intention with aborting was to make it easier to spot errors in calling the 
library.


> 2) struct cfg_opt_t.nvalues is documented as the number of parsed values, but
>   it's used as the amount of values in struct cfg_opt_t.values.
>   I miss a call to detect whether an option was included in the
> configuration file
>   or not (e.g. to differ if one wants to suppress logging by set
> log-file = "" or if
>   simply no entry of log-file was included in the parsed config).
>   As workaround I use currently a null pointer as default value for
> strings, but
>   this doesn't work for sections.
> 
>   My suggestions would be to have a separate counter for parsed values in
>   struct cfg_opt_t.
> 
> 3) libConfuse has an incompatible boolean type compared to stdbool.h.
>   I'd like to see there some defines to the stdbool.h boolean types and 
> values,
>   when available - and the libConfuse own own ones only on compilers
>   before C99.

IIRC, recent commits made libconfuse dependent on C99. So either a simple int 
or the bool from stdbool.h seems reasonable.

> I can spent some effort to fix above issues if you want.

Nathan or Carlo ?

> Cheers,
> Jens