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:
- 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.
- I noticed that you do not check the status of f_memstream = open_memstream
call. You probably should.
Thank you. An asserion was added.
- 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 amatch * 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