On (18/05/13 18:55), Jakub Hrozek wrote:
On Fri, May 17, 2013 at 07:29:01PM +0200, Lukas Slebodnik wrote:
> On (17/05/13 18:50), Lukas Slebodnik wrote:
> >ehlo,
> >
> >This week Ondra fix segfault, which was caused by missing argument
> >in debug message.
> >> ("nsupdate_get_addrs_done failed: [%d]: [%s]\n",
> >> sss_strerror(ret)));
> >
> >I was working on another ticket and I spotted similar bug.
> >So I decided to enable printf format checking in debug_fn.
> >There was a huge number of warnings. Attached patch fixes the most critical.
> >--missing argument(s).
> >--format '%s', but argument is integer.
> >--wrong format string, example: '%\n'
> >
> >There are another 150 warnings, but they aren't critical.
> >for example:
> >format '%d' expects argument of type 'int', but argument 2 has
type 'size_t'
> >
Hi,
this is a really good patch that fixes many potentially bad issues.
I think we should still fix these eventually, for instance for size_t,
we should use the %z conversion.
> I forgot to wrote in the first mail:
> Remaining warnings are not critical, but they are. So I don't want to pollute
> output from compiler with warnings, therefore patch did not enable
> printf format checking in debug_fn.
>
What if we enabled the warnings only if SSSD was compiled with a certain
preprocessor directive? Then the developers would see if they introduce
any new warning, but compiling the "production" code that runs with
debug_level=0 anyway would not produce any warnings.
I think we should fix all warnings, because it is hard to find new introduced
warning among another 150 "integer format warnings"
(int -> {size_t, uint32_t, long ...})
Should I file a ticket?
About the patch - almost ack. Please squash the attached patch to fix
some debug levels and I'll ack.
Thank you. Patch squashed.
I squased another patch to improve readability.
--- a/src/providers/ldap/sdap_async_connection.c
+++ b/src/providers/ldap/sdap_async_connection.c
@@ -850,8 +850,8 @@ static void simple_bind_done(struct sdap_op *op,
talloc_zfree(nval);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
- ("Could not convert control response to an integer.
"
- "[%s]\n", strerror(ret)));
+ ("Couldn't convert control response "
+ "to an integer [%s].\n", strerror(ret)));
goto done;
}
LS