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.
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.
From 8d3d072aca6761303d3c5817d530adda5bdd3dfa Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@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
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?
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
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 dpal@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
On (19/10/13 01:08), Dmitri Pal wrote:
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.
I tried to find c-style comments in "*.ini" files on windows, but I cannot find any. Comments in windows ini files start with char ';'
If we want to be able parse windows config files we should support semicolon-style comments
LS
On 10/21/2013 12:15 PM, Lukas Slebodnik wrote:
On (19/10/13 01:08), Dmitri Pal wrote:
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.
I tried to find c-style comments in "*.ini" files on windows, but I cannot find any. Comments in windows ini files start with char ';'
If we want to be able parse windows config files we should support semicolon-style comments
LS
The files that are used in GPO at not exactly ini files. They have other extensions. Here is the file that comes from Windows that I have been testing with: https://fedorahosted.org/sssd/attachment/ticket/2119/symbols.conf It is added to unit tests at the end of the big patch set.
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Lukas Slebodnik