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

Ondrej Kos okos at redhat.com
Thu Dec 6 13:19:46 UTC 2012


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.

>
> 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

-- 
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