7:17 p.m.
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.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
7:28 a.m.
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*
[PATCH 3/10]
git am doesn't like the two empty lines added to the end of 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.
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
[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
[PATCH 5/10]
*Ack* (i noticed the warning with the first patch)
review for patches 6-10 will follow.
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
8:35 a.m.
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
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
10:43 a.m.
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 removed this change from the patch 4 and moved it to the later patch.
The regenerated patch set is attached.
Thanks for the review!
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
7:19 a.m.
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.
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
--
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
5:20 p.m.
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
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
3:12 a.m.
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(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
4151
days inactive
4158
days old
sssd-devel@lists.fedorahosted.org
6 comments
2 participants
participants (2)
-
Dmitri Pal -
Ondrej Kos