On 10/17/2013 10:39 AM, Lukas Slebodnik wrote:
> On (06/10/13 12:29), Dmitri Pal wrote:
>> Hello
>>
>> The following patch set addresses ticket
>> https://fedorahosted.org/sssd/ticket/2108
>>
>> Please see commit messages for more details about what each patch does.
>> Please follow the order of the patches in the set.
>>
>> --
>> Thank you,
>> Dmitri Pal
>>
>> Sr. Engineering Manager for IdM portfolio
>> Red Hat Inc.
>>
>>
> Code looks good to me and is quite simple.
> I would like to give an ACK, but I have a few notes.
>
> The most confusing thing to me was:
> c-style comments *HAVE TO* start at the beginning of line.
>
> I think it can be also confusing for other users,
> because a lot of people use /* */ anywhere.
> For example to describe members of structure and users may expect the same
> behaviour in libini.
This is why it is documented that it is not allowed to start not at the
beginning of the line.
The support of c-style comments is added to be able to parse windows
config files.
So far all files I have seen follow the convention.
If the comment starts not in the first position there is ambiguity: is
it a part of the value or not?
IMO if you have
key = /* */
Then value is '/* */'
I do not think it makes sense to add complexity of letting caller decide
as the caller might not know.
So far we stayed away from escaping if we want to support what you
suggest we would hate to introduce escaping and require people to enter
key = //* *//
if they want to get value '/* */'
This is complexity that so far was not need and I think we should stay
this way until the use case emerges.
You can file and RFE.
>
> >From 8d3d072aca6761303d3c5817d530adda5bdd3dfa Mon Sep 17 00:00:00 2001
>> From: Dmitri Pal <d...@redhat.com>
>> Date: Sun, 6 Oct 2013 11:29:25 -0400
>> Subject: [PATCH 1/5] [INI] Do not check validity of comments
>>
>> The validity of comment was checked on the low level.
>> Now when we support c-style comments any line can be
>> a part of comment so vality check needs to be removed.
>> Comment object should trust parser to do the right thing.
>> ---
> check_for_comment looks like inspired by removed function ini_comment_is_valid
The function was checking something that assumed only one line comments
this is why it was removed.
And it was doing the check at a wrong level (too deep).
>
> INI_PARSE_NO_C_COMMENTS was introduced in another commit.
>
> Do we want to check validity of comment with enabled flag
> INI_PARSE_NO_C_COMMENTS? I think libini should not change behaviour or am I
> missed something?
Before c-style comments would have generated an error so it was not
possible to add them so existing ini files do not have them.
Now by default they can be added unless the library caller explicitly
suppresses their presence in the application.
It is sort of enablement of the behavior. I saw it as a value. Should I
make this turned off and require caller to turn it on to enable the
functionality?
Sounded backwards to me.
Falls into the same category as wrapping. We decided that wrapping
should be turned on by default. I assumed that with comments the case
should be the same.
>
>> ini/ini_comment.c | 47 -----------------------------------------------
>> 1 files changed, 0 insertions(+), 47 deletions(-)
>>
>> diff --git a/ini/ini_comment.c b/ini/ini_comment.c
>> index 3d25562..53a84f8 100644
>> --- a/ini/ini_comment.c
>> +++ b/ini/ini_comment.c
>> @@ -200,45 +200,6 @@ int ini_comment_copy(struct ini_comment *ic,
>> return error;
>> }
>>
>> -/* Is the comment valid? */
>> -static int ini_comment_is_valid(const char *line)
>> -{
>> - int i;
>> -
>> - TRACE_FLOW_ENTRY();
>> -
>> - /* Null is ok */
>> - if (!line) {
>> - TRACE_FLOW_STRING("ini_comment_is_valid", "Exit - NULL str");
>> - return 1;
>> - }
>> -
>> - /* Empty is Ok or starts with a special symbol */
>> - if ((line[0] == '\0') ||
>> - (line[0] == ';') ||
>> - (line[0] == '#')) {
>> - TRACE_FLOW_STRING("ini_comment_is_valid", "Exit - empty or
>> comment");
>> - return 1;
>> - }
>> -
>> - /* All spaces is Ok too */
>> - TRACE_INFO_STRING("Line to eval", line);
>> -
>> - i = 0;
>> - while (line[i] != '\0') {
>> - if (!isspace(line[i])) {
>> - TRACE_ERROR_STRING("ini_comment_is_valid", "Invalid comment");
>> - return 0;
>> - }
>> - i++;
>> - }
>> -
>> - TRACE_FLOW_STRING("ini_comment_is_valid", "Exit - empty str");
>> - return 1;
>> -
>> -}
>> -
>> -
>> /*
>> * Modify the comment object
>> */
>> @@ -281,14 +242,6 @@ static int ini_comment_modify(struct ini_comment *ic,
>> memcpy(&input, &line, sizeof(char *));
>>
>> if (mode != INI_COMMENT_MODE_REMOVE) {
>> - /*
>> - * Check that provided line is a comment or an empty line.
>> - * Can be NULL too.
>> - */
>> - if (!ini_comment_is_valid(input)) {
>> - TRACE_ERROR_NUMBER("Invalid comment", EINVAL);
>> - return EINVAL;
>> - }
>>
>> error = simplebuffer_alloc(&elem);
>> if (error) {
>> --
>> 1.7.1
>>
> LS
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
