On (14/06/15 11:47), Dmitri Pal wrote:
On 05/30/2015 03:49 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)
>
>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.
>
>LS
Hi Lukas,
I took some time and reviewed the test module and the code.
Here are my findings:
1. In the test module system() call can return 0 but still fail. Please see
how to check errors for the system() call in other ut modules in "ini".
I
compare in memory configurations with memcmp. The call system
(with utility diff) is used just for printing diff to stdout in case of
different results.
2. I noticed that you do not check the status of f_memstream =
open_memstream
call. You probably should.
Thank you.
An asserion was added.
3. The main issue that you are asking about above looks like a bug to
me.
Under no conditions the key2 should be touched if you are adding or modifying
key1. I looked at the code.
The core of the issue is in the collection.c in col_parent_traverse_handler
function
Here is the section of code from the function where it handles the processing
of the list
if (to_find->interrupt) {
/* As soon as we found first non matching one but there was a
match
* return the parent of the last found item.
*/
if (((!match) || (current->next == NULL)) &&
(to_find->index != 0) && (to_find->found)) {
*stop = 1;
if (to_find->index == -2)
*((struct collection_item **)traverse_data) =
to_find->parent;
else
if (to_find->exact) {
/* This means that we need to match the exact
* index but we did not */
to_find->parent = NULL;
to_find->found = 0;
}
else
*((struct collection_item **)traverse_data) =
to_find->parent->next; <----- WRONG for the use case we test. I think it
should be just parent and not parent->next
I tried many times but any change to col_parent_traverse_handler
caused failed collection unit test. So I decided to slightly modify
function col_get_dup_item.
However I suspect that some other test case will start to fail.
This would mean that an if statement is missing within this last else above.
Give it a try and let me know the results.
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.
LS