[SSSD] [INI] Patches for the INI configuration modification

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 23 10:59:31 UTC 2015


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.
>
>   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.
>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)

> * 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tar.gz
Type: application/gzip
Size: 53766 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150623/caf7a970/attachment.bin>


More information about the sssd-devel mailing list