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
patches.tar.gz
Description: application/gzip
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
