On Fri, Apr 08, 2016 at 01:05:54PM +0200, Jakub Hrozek wrote:
On Fri, Apr 08, 2016 at 12:15:25PM +0200, Pavel Březina wrote:
> On 03/01/2016 01:53 PM, Pavel Březina wrote:
> >On 02/25/2016 02:08 PM, Jakub Hrozek wrote:
> >>On Tue, Feb 09, 2016 at 02:07:21PM +0100, Pavel Březina wrote:
> >>>First of the responders is converted -)
> >>
> >>Sorry for the first delay in review. Before reading the code, I
> >>submitted the patches to automated tests, so far I found:
> >>
> >>Error: COMPILER_WARNING:
> >>sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c:29:
> >>included_from: Included from here.
> >>sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c: scope_hint:
> >>In function 'sudosrv_get_rules_done'
> >>sssd-1.13.90/src/util/util.h:144:9: warning: 'debug_name' may be
used
> >>uninitialized in this function [-Wmaybe-uninitialized]
> >># sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> >># ^
> >>sssd-1.13.90/src/responder/sudo/sudosrv_get_sudorules.c:225:17: note:
> >>'debug_name' was declared here
> >># const char *debug_name;
> >># ^
> >># 142| int __debug_macro_level = level; \
> >># 143| if (DEBUG_IS_SET(__debug_macro_level)) { \
> >># 144|-> sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
> >># 145| __debug_macro_level, \
> >># 146| format, ##__VA_ARGS__); \
> >>
> >>But feel free to only fix the issue in your branch for now, at least
> >>until
> >>all the automated downstream tests finish.
> >
> >I think it is false positive, well it should be anyway, but here's the
> >diff squashed into forth patch:
> >
> >diff --git a/src/responder/sudo/sudosrv_get_sudorules.c
> >b/src/responder/sudo/sudosrv_get_sudorules.c
> >index 1861e3f..04345f7 100644
> >--- a/src/responder/sudo/sudosrv_get_sudorules.c
> >+++ b/src/responder/sudo/sudosrv_get_sudorules.c
> >@@ -222,7 +222,7 @@ static errno_t sudosrv_cached_rules(TALLOC_CTX
> >*mem_ctx,
> > {
> > unsigned int flags = SYSDB_SUDO_FILTER_NONE;
> > struct sysdb_attrs **rules;
> >- const char *debug_name;
> >+ const char *debug_name = "unknown";
> > uint32_t num_rules;
> > errno_t ret;
> > const char *attrs[] = {SYSDB_OBJECTCLASS,
> >
> >Another solution would be to add default branch into the switch in this
> >function.
>
>
> Bump.
Sorry about the delay. I verified all tests pass with the patches (not
sure if I told you earlier), so we just need to ack the code..
The code looks good to me, too and neither Coverity nor our CI:
http://sssd-ci.duckdns.org/logs/job/41/07/summary.html
found any issues. I also re-ran the FreeIPA tests using this code and
didn't find any bugs.
ACK