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

Dmitri Pal dpal at redhat.com
Sun Jun 14 15:47:17 UTC 2015


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".
2. I noticed that you do not check the status of f_memstream = 
open_memstream call. You probably should.
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

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.




-- 
Thank you,
Dmitri Pal

Director of Engineering for IdM portfolio
Red Hat, Inc.




More information about the sssd-devel mailing list