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

[Dmitri Pal]

On 05/30/2015 03:47 PM, Lukas Slebodnik wrote:
> On (02/01/15 14:47), Dmitri Pal wrote:
> > Hello,
> >
> > Please find attached patches for the new interface to modify configuration
> > files using libini_config.
>
> Dimitri,
> I was writing additional unit tests for missing parts
> and I found a small problem with INI_VA_MOD and INI_VA_MODADD
>
> The documentation says:
> /**
> @brief Update a specific value (best effort).
> Value of the index is used to determine which one of the duplicates
> needs to be updated. Index is 0-based. If the index is out of range
> the function will do best effort and return the last instance of the key.
> For example if you have five duplicates and you are searching for the tenth
> the function will find and return the fifth instance.
>
> */
>   INI_VA_MOD        = 1,
>
> Input config:
> key0 = valuer0
> key1 = value1a
> key1 = value1b
> key1 = value1c
> key1 = value1d
> key2 = value2
> key3 = value3
>
> Expected:                               Result:
> [zero]                                    [zero]
> [one]                                     [one]
> key0 = valuer0                            key0 = valuer0
> key1 = value1a                            key1 = value1a
> key1 = value1b                            key1 = value1b
> key1 = value1c                            key1 = value1c
> key1 = newvalue <<<<<<<<<<                key1 = value1d       <<<<<<<
> key2 = value2   <<<<<<<<<<                key2 = newvalue      <<<<<<<
> key3 = value3                             key3 = value3
>
> I need the second pair of eyes to look into this issue.
> I will appreciate if you could find few minutes.
> Attached is updated patches with check-based unit for this problem.
> (ini_configmod_ut_check)

I have not looked at that yet. However I reviewed the patch where you 
fix the leak - that one makes sense.
Other patches (except the unit test which will take me a bit to digest) 
look good. But I just reviewed them visually.

The patch with unit test is challenging and will take some time for me 
to grasp.
> BTW. It's not clear to me waht is a difference
> between INI_VA_MOD and INI_VA_MODADD
> or between INI_VA_MOD_E and INI_VA_MODADD_E.
> The code is the same.

With MOD or MOD_E you expect to modify a value so you expect that the 
value exists. If the value does not exist the col_get_dup_item with 
return ENOENT and the function ini_config_add_str_value will return 
error too because you are trying to modify something that should exist 
but it does not.

If you do not care whether some value exists you can use MODADD or 
MODADD_E. In this case the ENOENT error is suppressed. See error 
checking line (this is the difference). So later we check that item 
exists. If it exists we will modify it, otherwise just add.

The difference between no _E suffix and with _E is what search we are 
conducting: an exact one or not. For more details see col_get_dup_item. 
In exact mode it will return error if you are asking for 10th duplicate 
when there are just five. In not exact mode it will do its best as 
described in you quoted text at the top of this email.

HTH
Dmitri
> LS