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(a)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(a)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