[SSSD] [INI] Patches for the INI configuration modification
Jakub Hrozek
jhrozek at redhat.com
Tue Jun 23 11:29:57 UTC 2015
On Tue, Jun 23, 2015 at 12:59:31PM +0200, Lukas Slebodnik wrote:
> On (23/06/15 11:25), Jakub Hrozek wrote:
> >On Tue, Jun 23, 2015 at 10:36:30AM +0200, Lukas Slebodnik wrote:
> >> Patches 1-16 are Dimitri's patches
> >> and other patches are my bug fixes + unit test.
> >> It's quite big but it covers all functionality of function
> >> ini_config_add_str_value which is reused by other public functions.
> >
> >I'll only review Lukas' fixes:
> > * ini_config_ut: enable verbose mode with env variable - ACK
> >
> > * collection: Add new function col_remove_item_with_cb - the changes
> > are fine, but do you want to also change the TRACE_FLOW_STRING from
> > Exit/Exit to Enter/Exit in col_remove_item_with_cb()?
> Nice catch.
> Fixed.
ACK
> >
> > Also col_remove_item() has no tracing, but I guess that's OK since
> > it's just a wrapper.
> >
> > * INI: Fix memory leak with INI_VA_CLEAN - ACK, value_destroy() would
> > now be called
> >
> > * COLLECTION: Return the last duplicate for big index - sorry, I'm too
> > detached from the code to understand the change, can you add a comment
> > what does this block do?
> col_get_dup_item should return last duplicate for very big index (out of bound)
> and not different property.
>
> Comment was added.
Thank you, ACK
> >Style-wise, why did you use Yoda-condition?
> >
> Updated.
>
>
> > * INI: Fix adding string with INI_VA_MODADD_E and big index - this
> > seems a bit strange to me, wouldn't it be better to
> > INI_VA_MODADD/INI_VA_MODADD_E cases? In particular, using exact case
> > always to 1 even for INI_VA_MODADD seems a bit odd?
> >
> I split INI_VA_MODADD INI_VA_MODADD_E.
> But code INI_VA_MODADD_E is almost the same as in previous patch.
> Because We need to distinguish between non-existing property (ENOENT)
> and very big index + "exact index flag" (ENOENT)
I think that's fine, because the code is now easier to read even if you
don't know the code too deep. It's just clearer.
ACK.
>
> > * INI: Add check based test ini_configmod_ut_check
> > Seems to cover the code well, runs fine and has no leaks:
> > ==18028== total heap usage: 20,176 allocs, 20,176 frees, 4,786,296
> > bytes allocated
>
> Updated version is attached.
>
> LS
ACK to all your patches.
More information about the sssd-devel
mailing list