[SSSD] [PATCHES] Ding libs patches for merge related functionality

Ondrej Kos okos at redhat.com
Wed Dec 12 09:12:38 UTC 2012


On 07/12/12 00:20, Dmitri Pal wrote:
> On 12/06/2012 08:19 AM, Ondrej Kos wrote:
>> On 05/12/12 17:43, Dmitri Pal wrote:
>>> On 12/05/2012 09:35 AM, Dmitri Pal wrote:
>>>> On 12/05/2012 08:28 AM, Ondrej Kos wrote:
>>>>> On 05/12/12 02:17, Dmitri Pal wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please find attached the list of the 10 patches for ding-libs.
>>>>>> These patches implement merge capabilities of the ding-libs and
>>>>>> refine
>>>>>> parser to consistently deal with different parse modes.
>>>>>>
>>>>>>
>>>>>> Patches 1-5 are self contained patches that can be applied
>>>>>> independently
>>>>>> from each other but in the given order.
>>>>>> After applying each of those the tree should build-able.
>>>>>>
>>>>>> Patches 6-10 are related to each other and need to be viewed and
>>>>>> applied
>>>>>> in relation. They are a logically split for an easier review one
>>>>>> piece
>>>>>> of functionality .
>>>>>>
>>>>>> Patch names and commit comments give an idea of what happens in each
>>>>>> patch.
>>>>>> Most complicated patches are 6, 7 and 10.
>>>>>>
>>>>>> Let me know if I missed something.
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> sssd-devel mailing list
>>>>>> sssd-devel at lists.fedorahosted.org
>>>>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>>
>>>>> [PATCH 1/10]
>>>>>
>>>>> *ACK*
>>>>>
>>>>>
>>>>> [PATCH 2/10]
>>>>>
>>>>> just a nitpick, I wouldn't delete the empty line after introductory
>>>>> comment (maybe leave it for the future code re-styling)
>>>>>
>>>>> @@ -18,7 +18,6 @@
>>>>>        You should have received a copy of the GNU Lesser General Public
>>>>> License
>>>>>        along with Collection Library.  If not, see
>>>>> <http://www.gnu.org/licenses/>.
>>>>>    */
>>>>> -
>>>>>    #define _GNU_SOURCE
>>>>>    #include <string.h>
>>>>>    #include <stdlib.h>
>>>>>
>>>>> otherwise *ACK*
>>>> A new patch is ready.
>>>>
>>>>>
>>>>>
>>>>> [PATCH 3/10]
>>>>>
>>>>> git am doesn't like the two empty lines added to the end of file
>>>> I can't find place where there are two lines at the end of the file.
>>>>> $ gam 0003-INI-Merge-and-serialize-comments.patch
>>>>> Applying: Merge and serialize comments
>>>>> /home/okos/devel/ding-libs/.git/rebase-apply/patch:89: trailing
>>>>> whitespace.
>>>>>                              struct simplebuffer *sbobj)
>>>>> warning: 1 line adds whitespace errors.
>>>> That I found and fixed.
>>>> Will be in the next set of patches.
>>>>
>>>>> and *NACK*:
>>>>>
>>>>> ini_comment_serialize doesn't work as expected
>>>>> it is appending multiple CRs, not sure when exactly, but it probably
>>>>> has something to do with multi-line comments. and those empty lines
>>>>> result into
>>>>> = =
>>>>> thus breaking config file
>>>>> example:
>>>>>
>>>>> #Hoho section
>>>>> [hoho]
>>>>> #Hoho value
>>>>> val = hoho
>>>>> #End of hoho
>>>>> #Start of section
>>>>> [foo]
>>>>> #Second value
>>>>> bar = second value
>>>>> #End of section
>>>>> = =
>>>>> #Hoho section
>>>>> [hoho]
>>>>> #Hoho value
>>>>> val = hoho
>>>>> #Hoho section
>>>>> [hoho]
>>>>> #Hoho value
>>>>> val = hoho
>>>>> #End of hoho
>>>>>
>>>>>
>>>> Are you looking at the ini_parse_ut?
>>>> Hm let me try applying these patches one by one.
>>>> May be I need to restructure unit tests
>>>>
>>>>> [PATCH 4/10]
>>>>>
>>>>> make-check fails on:
>>>>>
>>>>> Files ./merge.conf.out and ./expect.conf.out differ
>>>>> Failed to run copy command 256 1.
>>>>> FAIL: ini_parse_ut
>>>>>
>>>>> caused by PATCH 3
>>>> Same thing
>>>>
>>>>>
>>>>>
>>>>> [PATCH 5/10]
>>>>>
>>>>> *Ack* (i noticed the warning with the first patch)
>>>>>
>>>>>
>>>>>
>>>>> review for patches 6-10 will follow.
>>>>>
>>>>> Ondra
>>>>>
>>>> I will send the refined patch set shortly
>>>>
>>> I reattached the set of patches.
>>> Here is the problem. I was doing the changes jumping between different
>>> parts of code so they are interconnected.
>>> It turns out that I was wrong with my assessment that the changes are
>>> independent in the first 5 patches from the later patches.
>>> The change introduced in patch 3 to the serialization is later corrected
>>> in patch 7.
>>> Patch 3 is for comment object and as long as ini_comment_ut returns
>>> success we are OK.
>>> After patch 4 check ini_valueobj_ut - it should return success but
>>> ini_parse_ut would fail.
>>> The reason is the following lines:
>>>
>>> -    if (strncmp(key, INI_SPECIAL_KEY, sizeof(INI_SPECIAL_KEY)) == 0) {
>>> -        /* Special key carries only a comment */
>>> -        TRACE_FLOW_EXIT();
>>> -        return EOK;
>>>        }
>>
>> I assumed it will have something to do with this part of code, as I
>> briefly checked the code, but didn't see anything obvious.
>
>
> Yes the special key was a hack.
> And it became apparent that it has issues when one merges two configs.
>
> The whole idea of the INI library is to preserve the configuration
> including comments.
> So comments are saved as a part of the actual value object.
> But when you have a comment at the bottom of the config file you do not
> have a value to attach it to.
> Originally I was putting it as a special value in the last section and
> that worked fine until I started merging configurations.
> With the original implementation I ended up with the comments in the
> middle of the merged section.
> That was wrong so I had to change how I treat the last comment.
>
>>
>>>
>>> I removed this change from the patch 4 and moved it to the later patch.
>>> The regenerated patch set is attached.
>>>
>>>
>>> Thanks for the review!
>>>
>>>
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>
>>
>> [PATCH 1/10]
>>
>> *ACK*
>>
>>
>> [PATCH 2/10]
>>
>> *ACK*
>>
>>
>> [PATCH 3/10]
>>
>> *ACK*
>>
>>
>> [PATCH 4/10]
>>
>> *ACK*
>>
>>
>> [PATCH 5/10]
>>
>> *ACK*
>>
>>
>> [PATCH 6/10]
>>
>> *ACK*
>>
>>
>> [PATCH 7/10]
>>
>> *ACK*
>>
>>
>> [PATCH 8/10]
>>
>> *ACK*
>>
>>
>> [PATCH 9/10]
>>
>> *ACK*
>>
>>
>> [PATCH 10/10]
>>
>> *ACK*
>>
>>
>> I also ran clang test after applying this patchset, result:
>> https://fedorahosted.org/sssd/ticket/1700
>>
>>
>> Ondra
>>
>
>

Pushed to master

-- 
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech

phone: +420-532-294-558
cell:  +420-736-417-909
ext:   82-62558
loc:   1013 Brno 1 office
irc:   okos @ #brno



More information about the sssd-devel mailing list