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

Lukas Slebodnik lslebodn at redhat.com
Tue Jun 23 08:36:30 UTC 2015


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


More information about the sssd-devel mailing list