[INI] Assorted patches for ding-libs

[Dmitri Pal]

Hello,

Here is a bunch of patches for review:

Patch 1:
 The unit test was not correct. The paths to files used in the unit test
were wrong. It used function exec instead of system which is bad too.
 It was hard to see what is going on so some more verbose output added.
 The config files are now copied our of the ini.d directory. The
permission test is adjusted.

Patch 2:
Can be squashed into previous one but I decided against it as it is a
change in configure.am not in code.
Patch 1 does all the copying of the files used in the unit test so there
is no need to copy file at the configure stage.

Patch 3:
Couple convenience functions for the value object that turned out to be
missing

Patches 4-10 (big!!!):
Definition, implementation, unit tests and docs for the new INI
interface that uses value object instead of the bare strings.
There is a lot of code there but it is mostly inspired by existing
interface. It exists in parallel for the backward compatibility.
Old inteface is still built, just not advertised via the docs. It needs
couple more layers of polish before it can be called complete.
The new interface is mostly a copy of the old interface so no big
logical differences other than ability to have keys with multiple values
in the same file.
For example there is now a way to deal with case like this:

file=x1
file=x2
file=x3

and get all the values for key "file" one at a time.

Plans for the near future:
1) Finish the section merge - it is not complete but when it is this
interface will be completely functional. The goal to that during August.
2) Provide a patch for SSSD to switch to the new interface - for August too.
This is the moment when we can call it v1.

Longer term plans:
1) Add functions to create config file to the interface. Currently it is
a the processing/parsing part, but you can't easily construct the
configuration file, only read it.
2) Add validation as it was originally planned.

I will try to find the victim to do the long term changes as I do not
scale for this any more.

[Jan Zelený]

Dne pátek 20 července 2012 16:36:57, Dmitri Pal napsal(a):
> Hello,
>
> Here is a bunch of patches for review:

Hi,
it took me a while but I finally finished the review.
> Patch 1:
>  The unit test was not correct. The paths to files used in the unit test
> were wrong. It used function exec instead of system which is bad too.
>  It was hard to see what is going on so some more verbose output added.
>  The config files are now copied our of the ini.d directory. The
> permission test is adjusted.

Nack,
- a bunch of trailing whitespace errors
- wrong position of "%s" on line 98 of the patch, similarly on lines 195 and 
306
  (IMO it should read "open file %s for reading")
- delete builddir variables defined on lines 108 and 142 of the patch, they are 
useless in this patch
- printf on line 196 should say "Src dir is %s" instead of "Src is %s"
- please replace all sprintf calls in the patch with snprintf. The same 
applies for all sprintf statements in ini/ini_parse_ut.c. Perhaps a separate 
patch should be done for this issue?
- could you please explain why you are calling system("chmod ...") instead of 
just chmod(...)?
> Patch 2:
> Can be squashed into previous one but I decided against it as it is a
> change in configure.am not in code.
> Patch 1 does all the copying of the files used in the unit test so there
> is no need to copy file at the configure stage.

Ack
> Patch 3:
> Couple convenience functions for the value object that turned out to be
> missing

Nack
- a bunch of trailing whitespace errors
- value_get_concatenated_len() should return the length and in case of NULL on 
input, it should return 0
- I don't fully understand the condition at the end of the patch. The second 
part is constraining the strncmp (consider two different strings of the same 
length). The only alternative making sense would be if (len != expected_len || 
strncmp() != 0) ... please note the order of both conditions and the logical 
or instead of logical and
> Patches 4-10 (big!!!):
> Definition, implementation, unit tests and docs for the new INI
> interface that uses value object instead of the bare strings.
> There is a lot of code there but it is mostly inspired by existing
> interface. It exists in parallel for the backward compatibility.
> Old inteface is still built, just not advertised via the docs. It needs
> couple more layers of polish before it can be called complete.
> The new interface is mostly a copy of the old interface so no big
> logical differences other than ability to have keys with multiple values
> in the same file.
> For example there is now a way to deal with case like this:
>
> file=x1
> file=x2
> file=x3
>
> and get all the values for key "file" one at a time.

#0004: Nack
- a bunch of trailing whitespace errors
- there are some typos and bad phrasing (I'd prefer native English speaker to 
confirm this, consider this to be only a proposal):
-- line 49: from the INI file -> from an INI file
-- lines 65 and 66: to address our requirements fully -> to fully address our 
requirements
-- in the paragraph between lines 65 and 71: I don't think we want to tell 
about our future plans since these plans won't be always *future* plans and it 
is likely that this text will remain forgotten at that point
-- line 76: differences -> advantages (sounds better)
-- line 78: data not managing it -> data, not managing it
-- line 81: have been -> has been
-- lines 90-92: drop "the" from the beginning of each line
-- line 96: library now support -> library now supports
-- line 96: The values -> Values
-- line 96: span multiple lines -> span across multiple lines
-- line 98: the line should better read something line "After being read, both 
keys and values are stored in internal objects"
-- line 100: more than one file -> multiple files
-- line 100: the application -> an application
-- line 101: the /etc -> /etc (drop "the")
-- line 102: all the applications -> all applications
-- line 104: for the application -> for each application
-- line 106: both files -> all files
-- line 106: the files need -> all files need
-- line 107: merged -> merged together
-- line 109: section those -> section, those
-- line 111: the values are treated -> all values are treated
-- line 113: If any value is too long, an error will be returned.
-- line 114: the values -> a value
-- line 114: the caller -> caller
-- line 117: the functions -> parsing functions
-- line 118: into the array elements -> into the array of elements
-- line 120: parsing the section -> parsing section
-- line 123: of the individual functions -> of individual functions
-- lines 124 and 127: Library -> The library
-- line 124: one section -> a section
-- line 125: The flags control -> Flags control
-- line 131: supported for backwards -> supported only for backwards
-- line 177: Configuiration -> Configuration
-- line 223: The functions -> Functions
-- line 267: through -> over
-- line 279: Name of the ini file -> Path to the ini file
-- use consistenly either "configuration object" or "configuration file object"
-- line 348: was -> were
-- line 349: operations -> operation
-- line 361 is reduntant
-- line 393: Sends -> Prints
-- line 461: file configuration -> configuration file
-- line 534: a creates -> and creates
-- line 539: be merged to. -> be merged.
-- line 621: getting the lists of the sections -> getting lists of sections
-- line 709: interpret the values -> interpret values
-- line 764, 855, 902, 948, 998: The results -> The result
-- line 899: an long -> a long
-- line 988: integer -> int32_t
- what about @defgroup structures Structures - is something supposed to be 
there? In any case I think it should be removed for now.
- INI_GET_NEXT_VALUE  should better be INI_GET_LAST_VALUE
- ini_config_get_filename() is missing description: what is fully resolved file 
name?
- please use consistenly either @ref or \ref

#0005: Ack

#0006: Nack,
- a bunch of trailing whitespace errors
- please improve your commit message:
-- inspured -> inspired
-- relaced -> replaced
- in get_str_cfg_array()
-- handling separators is suspicious: why did you set fixed length of the array 
to 4? If we are to leave fixed-length array, you should use a constant to define 
this and use that constant in the "else" branch of separator handling 
condition on line 88 and 89 of ini_get_valueobj.c
- typo on line 101 of ini_get_valueobj.c: Syppress
- please don't use memcpy to assign from one variable to another (line 102 of 
ini_get_valueobj.c). The correct approach is to declare two variables (char 
*copy_start; and const char *start;) - one for each array. No warnings and the 
approach is much more readable.
- would it be possible to change prototype to the one we use in SSSD? That is: 
error code as a return value and last two arguments being array size and 
output array? I'm not exactly sure if this interface is already used or 
anything, that's why I ask. If it is already used, I guess you can ignore this 
point.
- please make the "size" argument mandatory in get_str_cfg_array(), as it is 
the same with the rest of these functions
- in ini_get_double_config_array() the first *error = EOK (line 323) is 
redundant. Similarly in ini_get_long_config_array()
- add some input checking to is_same_section() and is_same_name(). I know 
those functions are static but then again, so are other functions in the code 
and they have the checking in place.
- please check if you are calling TRACE_FLOW_RETURN() correctly in 
ini_get_valueobj.c. There are few cases where it should be called with 1 
instead of 0 IMO
- in ini_get_config_array() the check of "mode" isn't quite correct. If you 
want to be defensive, you will check for non-equivalence with both balues. But 
at least you should check if the mode isn't lower than the INI_GET_FIRST_VALUE
- lines 228 and 229 are redundant (if the condition is evaluated as true, the 
name will always be freed before the while cycle above)
- please add a trace in case val is out of range in functions 
ini_get_*_config_value()
- in functions like ini_get_int32_config_value(): please note that it is 
dangerous to assume that int has the same size as int32_t or any other type. 
If you want these functions to remain, please add some checks and/or proper 
conversions
- check if any conversion was performed in ini_get_double_config_value() the 
same way as you chaeck in ini_get_llong_config_value()
- the first error = EOK in ini_get_string_config_value() is redundant. The same 
for ini_get_bin_config_value()
- the construct on lines 720 and 721 of ini_get_valueobj.c is needlessly 
complicated, you can just iterate like this: for (i=0;i<len;i++) if 
(isxdigit(buff[i])) ....
- some helper macros for the conversion starting on line 737 would be really 
nice. If designed properly, it would be the possible to do just something line 
hex = 16*first_val+second_val

#0007: Ack

#0008: Ack

#0009:
- a bunch of trailing whitespace errors
- while you at editing this file, there is one compilation warning:
ini/ini_parse_ut.c: In function ‘read_again_test’:
ini/ini_parse_ut.c:208:11: warning: variable ‘srcdir’ set but not used [-
Wunused-but-set-variable]
- after you call ini_get_int_config_value() on line 1192 of the file, you should 
check for unexpected state and return if such state occurred (didn't receive 
error)
- delete confusing comments on lines 1209 and 1216 of the file (/* We expected 
error in this case */)
- nitpick: you are missing space after if on line 1298 of the file 
(if(number_unsigned != 3) {)
- as a general comment/question: do your test fail if the expected value 
differs from the value found in config file? It doesn't look like that and for 
some tests it could be helpful (for example those int/double arrays, I was 
confused at to what values should I expect)

#0010: Ack but please correct the typo in commit message (descryption -> 
description)

Thanks
Jan

[Dmitri Pal]

On 08/03/2012 08:28 AM, Jan Zelený wrote:
> Dne pátek 20 července 2012 16:36:57, Dmitri Pal napsal(a):
> > Hello,
> >
> > Here is a bunch of patches for review:
>
> Hi,
> it took me a while but I finally finished the review.
> > Patch 1:
> >  The unit test was not correct. The paths to files used in the unit test
> > were wrong. It used function exec instead of system which is bad too.
> >  It was hard to see what is going on so some more verbose output added.
> >  The config files are now copied our of the ini.d directory. The
> > permission test is adjusted.
>
> Nack,
> a bunch of trailing whitespace errors
> wrong position of "%s" on line 98 of the patch, similarly on lines 195 and
>
> 306
>   (IMO it should read "open file %s for reading")
> delete builddir variables defined on lines 108 and 142 of the patch, they are
>
> useless in this patch
> printf on line 196 should say "Src dir is %s" instead of "Src is %s"
> please replace all sprintf calls in the patch with snprintf. The same
>
> applies for all sprintf statements in ini/ini_parse_ut.c. Perhaps a separate 
> patch should be done for this issue?
> could you please explain why you are calling system("chmod ...") instead of
>
> just chmod(...)?
> > Patch 2:
> > Can be squashed into previous one but I decided against it as it is a
> > change in configure.am not in code.
> > Patch 1 does all the copying of the files used in the unit test so there
> > is no need to copy file at the configure stage.
>
> Ack
> > Patch 3:
> > Couple convenience functions for the value object that turned out to be
> > missing
>
> Nack
> a bunch of trailing whitespace errors
> value_get_concatenated_len() should return the length and in case of NULL on
>
> input, it should return 0
> I don't fully understand the condition at the end of the patch. The second
>
> part is constraining the strncmp (consider two different strings of the same 
> length). The only alternative making sense would be if (len != expected_len || 
> strncmp() != 0) ... please note the order of both conditions and the logical 
> or instead of logical and
> > Patches 4-10 (big!!!):
> > Definition, implementation, unit tests and docs for the new INI
> > interface that uses value object instead of the bare strings.
> > There is a lot of code there but it is mostly inspired by existing
> > interface. It exists in parallel for the backward compatibility.
> > Old inteface is still built, just not advertised via the docs. It needs
> > couple more layers of polish before it can be called complete.
> > The new interface is mostly a copy of the old interface so no big
> > logical differences other than ability to have keys with multiple values
> > in the same file.
> > For example there is now a way to deal with case like this:
> >
> > file=x1
> > file=x2
> > file=x3
> >
> > and get all the values for key "file" one at a time.
>
> #0004: Nack
> a bunch of trailing whitespace errors
> there are some typos and bad phrasing (I'd prefer native English speaker to
>
> confirm this, consider this to be only a proposal):
> -- line 49: from the INI file -> from an INI file
> -- lines 65 and 66: to address our requirements fully -> to fully address our 
> requirements
> -- in the paragraph between lines 65 and 71: I don't think we want to tell 
> about our future plans since these plans won't be always *future* plans and it 
> is likely that this text will remain forgotten at that point
> -- line 76: differences -> advantages (sounds better)
> -- line 78: data not managing it -> data, not managing it
> -- line 81: have been -> has been
> -- lines 90-92: drop "the" from the beginning of each line
> -- line 96: library now support -> library now supports
> -- line 96: The values -> Values
> -- line 96: span multiple lines -> span across multiple lines
> -- line 98: the line should better read something line "After being read, both 
> keys and values are stored in internal objects"
> -- line 100: more than one file -> multiple files
> -- line 100: the application -> an application
> -- line 101: the /etc -> /etc (drop "the")
> -- line 102: all the applications -> all applications
> -- line 104: for the application -> for each application
> -- line 106: both files -> all files
> -- line 106: the files need -> all files need
> -- line 107: merged -> merged together
> -- line 109: section those -> section, those
> -- line 111: the values are treated -> all values are treated
> -- line 113: If any value is too long, an error will be returned.
> -- line 114: the values -> a value
> -- line 114: the caller -> caller
> -- line 117: the functions -> parsing functions
> -- line 118: into the array elements -> into the array of elements
> -- line 120: parsing the section -> parsing section
> -- line 123: of the individual functions -> of individual functions
> -- lines 124 and 127: Library -> The library
> -- line 124: one section -> a section
> -- line 125: The flags control -> Flags control
> -- line 131: supported for backwards -> supported only for backwards
> -- line 177: Configuiration -> Configuration
> -- line 223: The functions -> Functions
> -- line 267: through -> over
> -- line 279: Name of the ini file -> Path to the ini file
> -- use consistenly either "configuration object" or "configuration file object"
> -- line 348: was -> were
> -- line 349: operations -> operation
> -- line 361 is reduntant
> -- line 393: Sends -> Prints
> -- line 461: file configuration -> configuration file
> -- line 534: a creates -> and creates
> -- line 539: be merged to. -> be merged.
> -- line 621: getting the lists of the sections -> getting lists of sections
> -- line 709: interpret the values -> interpret values
> -- line 764, 855, 902, 948, 998: The results -> The result
> -- line 899: an long -> a long
> -- line 988: integer -> int32_t
> what about @defgroup structures Structures - is something supposed to be
>
> there? In any case I think it should be removed for now.
> INI_GET_NEXT_VALUE  should better be INI_GET_LAST_VALUE
> ini_config_get_filename() is missing description: what is fully resolved file
>
> name?
> please use consistenly either @ref or \ref
>
> #0005: Ack
>
> #0006: Nack,
> a bunch of trailing whitespace errors
> please improve your commit message:
>
> -- inspured -> inspired
> -- relaced -> replaced
> in get_str_cfg_array()
>
> -- handling separators is suspicious: why did you set fixed length of the array 
> to 4? If we are to leave fixed-length array, you should use a constant to define 
> this and use that constant in the "else" branch of separator handling 
> condition on line 88 and 89 of ini_get_valueobj.c
> typo on line 101 of ini_get_valueobj.c: Syppress
> please don't use memcpy to assign from one variable to another (line 102 of
>
> ini_get_valueobj.c). The correct approach is to declare two variables (char 
> *copy_start; and const char *start;) - one for each array. No warnings and the 
> approach is much more readable.
> would it be possible to change prototype to the one we use in SSSD? That is:
>
> error code as a return value and last two arguments being array size and 
> output array? I'm not exactly sure if this interface is already used or 
> anything, that's why I ask. If it is already used, I guess you can ignore this 
> point.
> please make the "size" argument mandatory in get_str_cfg_array(), as it is
>
> the same with the rest of these functions
> in ini_get_double_config_array() the first *error = EOK (line 323) is
>
> redundant. Similarly in ini_get_long_config_array()
> add some input checking to is_same_section() and is_same_name(). I know
>
> those functions are static but then again, so are other functions in the code 
> and they have the checking in place.
> please check if you are calling TRACE_FLOW_RETURN() correctly in
>
> ini_get_valueobj.c. There are few cases where it should be called with 1 
> instead of 0 IMO
> in ini_get_config_array() the check of "mode" isn't quite correct. If you
>
> want to be defensive, you will check for non-equivalence with both balues. But 
> at least you should check if the mode isn't lower than the INI_GET_FIRST_VALUE
> lines 228 and 229 are redundant (if the condition is evaluated as true, the
>
> name will always be freed before the while cycle above)
> please add a trace in case val is out of range in functions
>
> ini_get_*_config_value()
> in functions like ini_get_int32_config_value(): please note that it is
>
> dangerous to assume that int has the same size as int32_t or any other type. 
> If you want these functions to remain, please add some checks and/or proper 
> conversions
> check if any conversion was performed in ini_get_double_config_value() the
>
> same way as you chaeck in ini_get_llong_config_value()
> the first error = EOK in ini_get_string_config_value() is redundant. The same
>
> for ini_get_bin_config_value()
> the construct on lines 720 and 721 of ini_get_valueobj.c is needlessly
>
> complicated, you can just iterate like this: for (i=0;i<len;i++) if 
> (isxdigit(buff[i])) ....
> some helper macros for the conversion starting on line 737 would be really
>
> nice. If designed properly, it would be the possible to do just something line 
> hex = 16*first_val+second_val
>
> #0007: Ack
>
> #0008: Ack
>
> #0009:
> a bunch of trailing whitespace errors
> while you at editing this file, there is one compilation warning:
>
> ini/ini_parse_ut.c: In function ‘read_again_test’:
> ini/ini_parse_ut.c:208:11: warning: variable ‘srcdir’ set but not used [-
> Wunused-but-set-variable]
> after you call ini_get_int_config_value() on line 1192 of the file, you should
>
> check for unexpected state and return if such state occurred (didn't receive 
> error)
> delete confusing comments on lines 1209 and 1216 of the file (/* We expected
>
> error in this case */)
> nitpick: you are missing space after if on line 1298 of the file
>
> (if(number_unsigned != 3) {)
> as a general comment/question: do your test fail if the expected value
>
> differs from the value found in config file? It doesn't look like that and for 
> some tests it could be helpful (for example those int/double arrays, I was 
> confused at to what values should I expect)
>
> #0010: Ack but please correct the typo in commit message (descryption -> 
> description)
>
> Thanks
> Jan

Wow, thanks a lot for a through review.
I will try to address them ASAP.

[Dmitri Pal]

On 08/03/2012 08:28 AM, Jan Zelený wrote:
> Dne pátek 20 července 2012 16:36:57, Dmitri Pal napsal(a):
> > Hello,
> >
> > Here is a bunch of patches for review:
>
> Hi,
> it took me a while but I finally finished the review.
> > Patch 1:
> >  The unit test was not correct. The paths to files used in the unit test
> > were wrong. It used function exec instead of system which is bad too.
> >  It was hard to see what is going on so some more verbose output added.
> >  The config files are now copied our of the ini.d directory. The
> > permission test is adjusted.
>
> Nack,
> a bunch of trailing whitespace errors
> wrong position of "%s" on line 98 of the patch, similarly on lines 195 and
>
> 306
>   (IMO it should read "open file %s for reading")
> delete builddir variables defined on lines 108 and 142 of the patch, they are
>
> useless in this patch
> printf on line 196 should say "Src dir is %s" instead of "Src is %s"

Fixed all above.
Also there was a missing function declaration. I added it.
> please replace all sprintf calls in the patch with snprintf. The same
>
> applies for all sprintf statements in ini/ini_parse_ut.c. Perhaps a separate 
> patch should be done for this issue?

There will be a separate patch. I opened a ticket.
https://fedorahosted.org/sssd/ticket/1532
> could you please explain why you are calling system("chmod ...") instead of
>
> just chmod(...)?

Fixed.
I also made sure that it works both in in tree and parallel builds.
> > Patch 2:
> > Can be squashed into previous one but I decided against it as it is a
> > change in configure.am not in code.
> > Patch 1 does all the copying of the files used in the unit test so there
> > is no need to copy file at the configure stage.
>
> Ack

Unchanged
> > Patch 3:
> > Couple convenience functions for the value object that turned out to be
> > missing
>
> Nack
> a bunch of trailing whitespace errors

Fixed
> value_get_concatenated_len() should return the length and in case of NULL on
>
> input, it should return 0

I disagree with this comment. I think returning 0 when the required
argument is NULL is an error and should be treated as such.
Otherwise the error would be obscured and most likely will lead to crash.
Current function also consistent with other get functions in the API.

I also noticed that in all get functions against value object I do not
check that the other parameter is not NULL.
I added this check.
> I don't fully understand the condition at the end of the patch. The second
>
> part is constraining the strncmp (consider two different strings of the same 
> length). The only alternative making sense would be if (len != expected_len || 
> strncmp() != 0) ... please note the order of both conditions and the logical 
> or instead of logical and

I agree it should be || not &&.
Good catch.
As for the order it is manor optimization but i agree with it too.
> > Patches 4-10 (big!!!):
> > Definition, implementation, unit tests and docs for the new INI
> > interface that uses value object instead of the bare strings.
> > There is a lot of code there but it is mostly inspired by existing
> > interface. It exists in parallel for the backward compatibility.
> > Old inteface is still built, just not advertised via the docs. It needs
> > couple more layers of polish before it can be called complete.
> > The new interface is mostly a copy of the old interface so no big
> > logical differences other than ability to have keys with multiple values
> > in the same file.
> > For example there is now a way to deal with case like this:
> >
> > file=x1
> > file=x2
> > file=x3
> >
> > and get all the values for key "file" one at a time.
>
> #0004: Nack

All fixed.
> a bunch of trailing whitespace errors
> there are some typos and bad phrasing (I'd prefer native English speaker to
>
> confirm this, consider this to be only a proposal):
> -- line 49: from the INI file -> from an INI file
> -- lines 65 and 66: to address our requirements fully -> to fully address our 
> requirements
> -- in the paragraph between lines 65 and 71: I don't think we want to tell 
> about our future plans since these plans won't be always *future* plans and it 
> is likely that this text will remain forgotten at that point
> -- line 76: differences -> advantages (sounds better)
> -- line 78: data not managing it -> data, not managing it
> -- line 81: have been -> has been
> -- lines 90-92: drop "the" from the beginning of each line
> -- line 96: library now support -> library now supports
> -- line 96: The values -> Values
> -- line 96: span multiple lines -> span across multiple lines
> -- line 98: the line should better read something line "After being read, both 
> keys and values are stored in internal objects"
> -- line 100: more than one file -> multiple files
> -- line 100: the application -> an application
> -- line 101: the /etc -> /etc (drop "the")
> -- line 102: all the applications -> all applications
> -- line 104: for the application -> for each application
> -- line 106: both files -> all files
> -- line 106: the files need -> all files need
> -- line 107: merged -> merged together
> -- line 109: section those -> section, those
> -- line 111: the values are treated -> all values are treated
> -- line 113: If any value is too long, an error will be returned.
> -- line 114: the values -> a value
> -- line 114: the caller -> caller
> -- line 117: the functions -> parsing functions
> -- line 118: into the array elements -> into the array of elements
> -- line 120: parsing the section -> parsing section
> -- line 123: of the individual functions -> of individual functions
> -- lines 124 and 127: Library -> The library
> -- line 124: one section -> a section
> -- line 125: The flags control -> Flags control
> -- line 131: supported for backwards -> supported only for backwards
> -- line 177: Configuiration -> Configuration
> -- line 223: The functions -> Functions
> -- line 267: through -> over
> -- line 279: Name of the ini file -> Path to the ini file
> -- use consistenly either "configuration object" or "configuration file object"

These are two different terms.
One stores the configuration i.e. is "configuration object" another
stores metadata about the configuration file this it is configuration
file object.
I will add a comment to make it more clear.
> -- line 348: was -> were
> -- line 349: operations -> operation
> -- line 361 is reduntant
> -- line 393: Sends -> Prints
> -- line 461: file configuration -> configuration file
> -- line 534: a creates -> and creates
> -- line 539: be merged to. -> be merged.
> -- line 621: getting the lists of the sections -> getting lists of sections
> -- line 709: interpret the values -> interpret values
> -- line 764, 855, 902, 948, 998: The results -> The result
> -- line 899: an long -> a long
> -- line 988: integer -> int32_t
> what about @defgroup structures Structures - is something supposed to be
>
> there? In any case I think it should be removed for now.

No, I disagree. This would show in the doc and explain why no structure
is actually documented.
> INI_GET_NEXT_VALUE  should better be INI_GET_LAST_VALUE

While fixing the code I realized that I should add INI_GET_LAST_VALUE
and ability to fetch the last one.
https://fedorahosted.org/sssd/ticket/1536
> ini_config_get_filename() is missing description: what is fully resolved file
>
> name?

I added a more verbose description.
> please use consistenly either @ref or \ref

Fixed.
> #0005: Ack
>
> #0006: Nack,

All fixed.
> a bunch of trailing whitespace errors
> please improve your commit message:
>
> -- inspured -> inspired
> -- relaced -> replaced
> in get_str_cfg_array()
>
> -- handling separators is suspicious: why did you set fixed length of the array 
> to 4?

Because it was decided some time ago that 3 separators is good enough.
> If we are to leave fixed-length array, you should use a constant to define 
> this and use that constant in the "else" branch of separator handling 
> condition on line 88 and 89 of ini_get_valueobj.c

OK, added MAX_SEP_LEN
> typo on line 101 of ini_get_valueobj.c: Syppress
> please don't use memcpy to assign from one variable to another (line 102 of
>
> ini_get_valueobj.c). The correct approach is to declare two variables (char 
> *copy_start; and const char *start;) - one for each array. No warnings and the 
> approach is much more readable.

It took me a while to figure that part out otherwise I would have done
this initially.
Fixed
> would it be possible to change prototype to the one we use in SSSD? That is:
>
> error code as a return value and last two arguments being array size and 
> output array? I'm not exactly sure if this interface is already used or 
> anything, that's why I ask. If it is already used, I guess you can ignore this 
> point.

I do not really see a reason to change it. The array functions follow
the same paradigm as non array functions.
And it follows the already existing interface. The idea was that you
might not always care about the error.
Having functions to always return error forces more code to be written
by the consumer of the API.
I will leave it as is for now.

But I am open to changing it. Now is the right time to do so not because
the new interface is not used yet.
If we want to switch the functions to always return error we can do so,
however it will require a separate ticket and patch.
> please make the "size" argument mandatory in get_str_cfg_array(), as it is
>
> the same with the rest of these functions

This does not make sense. It is optional for all these functions. So
common function get_str_cfg_array initializes it if the passed in
pointer is provided.
> in ini_get_double_config_array() the first *error = EOK (line 323) is
>
> redundant. Similarly in ini_get_long_config_array()

It is. But I like to initialize variables. ;-)
Removed.
> add some input checking to is_same_section() and is_same_name(). I know
>
> those functions are static but then again, so are other functions in the code 
> and they have the checking in place.

Those are internal functions and the checks that they get the right
input have been done at the high level functions.
This is a general rule I follow: check input at the API boundary, do not
second check if the checked input passed forward to the internal functions.
> please check if you are calling TRACE_FLOW_RETURN() correctly in
>
> ini_get_valueobj.c. There are few cases where it should be called with 1 
> instead of 0 IMO

Fixed
> in ini_get_config_array()

I suspect you mean ini_get_config_valueobj
> the check of "mode" isn't quite correct. If you 
> want to be defensive, you will check for non-equivalence with both balues. But 
> at least you should check if the mode isn't lower than the INI_GET_FIRST_VALUE

Mode should have been unsigned... ;-) but I added the check instead.
> lines 228 and 229 are redundant (if the condition is evaluated as true, the
>
> name will always be freed before the while cycle above)

Removed
> please add a trace in case val is out of range in functions
>
> ini_get_*_config_value()

Done
> in functions like ini_get_int32_config_value(): please note that it is
>
> dangerous to assume that int has the same size as int32_t or any other type. 
> If you want these functions to remain, please add some checks and/or proper 
> conversions

Done
> check if any conversion was performed in ini_get_double_config_value() the
>
> same way as you chaeck in ini_get_llong_config_value()

In case of double it is OK to treat ERANGE as EIO. If you could not read
double from config file it is wrong. In the case of integers it makes
sense to deal with "value is too big" case but not for double.
> the first error = EOK in ini_get_string_config_value() is redundant. The same
>
> for ini_get_bin_config_value()

Fixed
> the construct on lines 720 and 721 of ini_get_valueobj.c is needlessly
>
> complicated, you can just iterate like this: for (i=0;i<len;i++) if 
> (isxdigit(buff[i])) ....

Done
> some helper macros for the conversion starting on line 737 would be really
>
> nice. If designed properly, it would be the possible to do just something line 
> hex = 16*first_val+second_val

Done
> #0007: Ack
>
> #0008: Ack

Next patch got squashed with this one.
> #0009:
> a bunch of trailing whitespace errors
> while you at editing this file, there is one compilation warning:
>
> ini/ini_parse_ut.c: In function ‘read_again_test’:
> ini/ini_parse_ut.c:208:11: warning: variable ‘srcdir’ set but not used [-
> Wunused-but-set-variable]

Removed in the re-factored first patch
> after you call ini_get_int_config_value() on line 1192 of the file, you should
>
> check for unexpected state and return if such state occurred (didn't receive 
> error)
> delete confusing comments on lines 1209 and 1216 of the file (/* We expected
>
> error in this case */)
> nitpick: you are missing space after if on line 1298 of the file
>
> (if(number_unsigned != 3) {)

All addressed.
> as a general comment/question: do your test fail if the expected value
>
> differs from the value found in config file? It doesn't look like that and for 
> some tests it could be helpful (for example those int/double arrays, I was 
> confused at to what values should I expect)

Test should fail if function returned something we did not expect.
> #0010: Ack but please correct the typo in commit message (descryption -> 
> description)

Done.
> Thanks
> Jan

Next version of patches (9 of them now) attached.

[Dmitri Pal]

On 09/25/2012 07:06 PM, Dmitri Pal wrote:
> On 08/03/2012 08:28 AM, Jan Zelený wrote:
> > Dne pátek 20 července 2012 16:36:57, Dmitri Pal napsal(a):
> > > Hello,
> > >
> > > Here is a bunch of patches for review:
> >
> > Hi,
> > it took me a while but I finally finished the review.
> > > Patch 1:
> > >  The unit test was not correct. The paths to files used in the unit test
> > > were wrong. It used function exec instead of system which is bad too.
> > >  It was hard to see what is going on so some more verbose output added.
> > >  The config files are now copied our of the ini.d directory. The
> > > permission test is adjusted.
> >
> > Nack,
> > a bunch of trailing whitespace errors
> > wrong position of "%s" on line 98 of the patch, similarly on lines 195 and
> >
> > 306
> >   (IMO it should read "open file %s for reading")
> > delete builddir variables defined on lines 108 and 142 of the patch, they are
> >
> > useless in this patch
> > printf on line 196 should say "Src dir is %s" instead of "Src is %s"
>
> Fixed all above.
> Also there was a missing function declaration. I added it.
> > please replace all sprintf calls in the patch with snprintf. The same
> >
> > applies for all sprintf statements in ini/ini_parse_ut.c. Perhaps a separate 
> > patch should be done for this issue?
>
> There will be a separate patch. I opened a ticket.
> https://fedorahosted.org/sssd/ticket/1532
> > could you please explain why you are calling system("chmod ...") instead of
> >
> > just chmod(...)?
>
> Fixed.
> I also made sure that it works both in in tree and parallel builds.
> > > Patch 2:
> > > Can be squashed into previous one but I decided against it as it is a
> > > change in configure.am not in code.
> > > Patch 1 does all the copying of the files used in the unit test so there
> > > is no need to copy file at the configure stage.
> >
> > Ack
>
> Unchanged
> > > Patch 3:
> > > Couple convenience functions for the value object that turned out to be
> > > missing
> >
> > Nack
> > a bunch of trailing whitespace errors
>
> Fixed
> > value_get_concatenated_len() should return the length and in case of NULL on
> >
> > input, it should return 0
>
> I disagree with this comment. I think returning 0 when the required
> argument is NULL is an error and should be treated as such.
> Otherwise the error would be obscured and most likely will lead to crash.
> Current function also consistent with other get functions in the API.
>
> I also noticed that in all get functions against value object I do not
> check that the other parameter is not NULL.
> I added this check.
> > I don't fully understand the condition at the end of the patch. The second
> >
> > part is constraining the strncmp (consider two different strings of the same 
> > length). The only alternative making sense would be if (len != expected_len || 
> > strncmp() != 0) ... please note the order of both conditions and the logical 
> > or instead of logical and
>
> I agree it should be || not &&.
> Good catch.
> As for the order it is manor optimization but i agree with it too.
> > > Patches 4-10 (big!!!):
> > > Definition, implementation, unit tests and docs for the new INI
> > > interface that uses value object instead of the bare strings.
> > > There is a lot of code there but it is mostly inspired by existing
> > > interface. It exists in parallel for the backward compatibility.
> > > Old inteface is still built, just not advertised via the docs. It needs
> > > couple more layers of polish before it can be called complete.
> > > The new interface is mostly a copy of the old interface so no big
> > > logical differences other than ability to have keys with multiple values
> > > in the same file.
> > > For example there is now a way to deal with case like this:
> > >
> > > file=x1
> > > file=x2
> > > file=x3
> > >
> > > and get all the values for key "file" one at a time.
> >
> > #0004: Nack
>
> All fixed.
> > a bunch of trailing whitespace errors
> > there are some typos and bad phrasing (I'd prefer native English speaker to
> >
> > confirm this, consider this to be only a proposal):
> > -- line 49: from the INI file -> from an INI file
> > -- lines 65 and 66: to address our requirements fully -> to fully address our 
> > requirements
> > -- in the paragraph between lines 65 and 71: I don't think we want to tell 
> > about our future plans since these plans won't be always *future* plans and it 
> > is likely that this text will remain forgotten at that point
> > -- line 76: differences -> advantages (sounds better)
> > -- line 78: data not managing it -> data, not managing it
> > -- line 81: have been -> has been
> > -- lines 90-92: drop "the" from the beginning of each line
> > -- line 96: library now support -> library now supports
> > -- line 96: The values -> Values
> > -- line 96: span multiple lines -> span across multiple lines
> > -- line 98: the line should better read something line "After being read, both 
> > keys and values are stored in internal objects"
> > -- line 100: more than one file -> multiple files
> > -- line 100: the application -> an application
> > -- line 101: the /etc -> /etc (drop "the")
> > -- line 102: all the applications -> all applications
> > -- line 104: for the application -> for each application
> > -- line 106: both files -> all files
> > -- line 106: the files need -> all files need
> > -- line 107: merged -> merged together
> > -- line 109: section those -> section, those
> > -- line 111: the values are treated -> all values are treated
> > -- line 113: If any value is too long, an error will be returned.
> > -- line 114: the values -> a value
> > -- line 114: the caller -> caller
> > -- line 117: the functions -> parsing functions
> > -- line 118: into the array elements -> into the array of elements
> > -- line 120: parsing the section -> parsing section
> > -- line 123: of the individual functions -> of individual functions
> > -- lines 124 and 127: Library -> The library
> > -- line 124: one section -> a section
> > -- line 125: The flags control -> Flags control
> > -- line 131: supported for backwards -> supported only for backwards
> > -- line 177: Configuiration -> Configuration
> > -- line 223: The functions -> Functions
> > -- line 267: through -> over
> > -- line 279: Name of the ini file -> Path to the ini file
> > -- use consistenly either "configuration object" or "configuration file object"
>
> These are two different terms.
> One stores the configuration i.e. is "configuration object" another
> stores metadata about the configuration file this it is configuration
> file object.
> I will add a comment to make it more clear.
> > -- line 348: was -> were
> > -- line 349: operations -> operation
> > -- line 361 is reduntant
> > -- line 393: Sends -> Prints
> > -- line 461: file configuration -> configuration file
> > -- line 534: a creates -> and creates
> > -- line 539: be merged to. -> be merged.
> > -- line 621: getting the lists of the sections -> getting lists of sections
> > -- line 709: interpret the values -> interpret values
> > -- line 764, 855, 902, 948, 998: The results -> The result
> > -- line 899: an long -> a long
> > -- line 988: integer -> int32_t
> > what about @defgroup structures Structures - is something supposed to be
> >
> > there? In any case I think it should be removed for now.
>
> No, I disagree. This would show in the doc and explain why no structure
> is actually documented.
> > INI_GET_NEXT_VALUE  should better be INI_GET_LAST_VALUE
>
> While fixing the code I realized that I should add INI_GET_LAST_VALUE
> and ability to fetch the last one.
> https://fedorahosted.org/sssd/ticket/1536
> > ini_config_get_filename() is missing description: what is fully resolved file
> >
> > name?
>
> I added a more verbose description.
> > please use consistenly either @ref or \ref
>
> Fixed.
> > #0005: Ack
> >
> > #0006: Nack,
>
> All fixed.
> > a bunch of trailing whitespace errors
> > please improve your commit message:
> >
> > -- inspured -> inspired
> > -- relaced -> replaced
> > in get_str_cfg_array()
> >
> > -- handling separators is suspicious: why did you set fixed length of the array 
> > to 4?
>
> Because it was decided some time ago that 3 separators is good enough.
> > If we are to leave fixed-length array, you should use a constant to define 
> > this and use that constant in the "else" branch of separator handling 
> > condition on line 88 and 89 of ini_get_valueobj.c
>
> OK, added MAX_SEP_LEN
> > typo on line 101 of ini_get_valueobj.c: Syppress
> > please don't use memcpy to assign from one variable to another (line 102 of
> >
> > ini_get_valueobj.c). The correct approach is to declare two variables (char 
> > *copy_start; and const char *start;) - one for each array. No warnings and the 
> > approach is much more readable.
>
> It took me a while to figure that part out otherwise I would have done
> this initially.
> Fixed
> > would it be possible to change prototype to the one we use in SSSD? That is:
> >
> > error code as a return value and last two arguments being array size and 
> > output array? I'm not exactly sure if this interface is already used or 
> > anything, that's why I ask. If it is already used, I guess you can ignore this 
> > point.
>
> I do not really see a reason to change it. The array functions follow
> the same paradigm as non array functions.
> And it follows the already existing interface. The idea was that you
> might not always care about the error.
> Having functions to always return error forces more code to be written
> by the consumer of the API.
> I will leave it as is for now.
>
> But I am open to changing it. Now is the right time to do so not because
> the new interface is not used yet.
> If we want to switch the functions to always return error we can do so,
> however it will require a separate ticket and patch.
> > please make the "size" argument mandatory in get_str_cfg_array(), as it is
> >
> > the same with the rest of these functions
>
> This does not make sense. It is optional for all these functions. So
> common function get_str_cfg_array initializes it if the passed in
> pointer is provided.
> > in ini_get_double_config_array() the first *error = EOK (line 323) is
> >
> > redundant. Similarly in ini_get_long_config_array()
>
> It is. But I like to initialize variables. ;-)
> Removed.
> > add some input checking to is_same_section() and is_same_name(). I know
> >
> > those functions are static but then again, so are other functions in the code 
> > and they have the checking in place.
>
> Those are internal functions and the checks that they get the right
> input have been done at the high level functions.
> This is a general rule I follow: check input at the API boundary, do not
> second check if the checked input passed forward to the internal functions.
> > please check if you are calling TRACE_FLOW_RETURN() correctly in
> >
> > ini_get_valueobj.c. There are few cases where it should be called with 1 
> > instead of 0 IMO
>
> Fixed
> > in ini_get_config_array()
>
> I suspect you mean ini_get_config_valueobj
> > the check of "mode" isn't quite correct. If you 
> > want to be defensive, you will check for non-equivalence with both balues. But 
> > at least you should check if the mode isn't lower than the INI_GET_FIRST_VALUE
>
> Mode should have been unsigned... ;-) but I added the check instead.
> > lines 228 and 229 are redundant (if the condition is evaluated as true, the
> >
> > name will always be freed before the while cycle above)
>
> Removed
> > please add a trace in case val is out of range in functions
> >
> > ini_get_*_config_value()
>
> Done
> > in functions like ini_get_int32_config_value(): please note that it is
> >
> > dangerous to assume that int has the same size as int32_t or any other type. 
> > If you want these functions to remain, please add some checks and/or proper 
> > conversions
>
> Done
> > check if any conversion was performed in ini_get_double_config_value() the
> >
> > same way as you chaeck in ini_get_llong_config_value()
>
> In case of double it is OK to treat ERANGE as EIO. If you could not read
> double from config file it is wrong. In the case of integers it makes
> sense to deal with "value is too big" case but not for double.
> > the first error = EOK in ini_get_string_config_value() is redundant. The same
> >
> > for ini_get_bin_config_value()
>
> Fixed
> > the construct on lines 720 and 721 of ini_get_valueobj.c is needlessly
> >
> > complicated, you can just iterate like this: for (i=0;i<len;i++) if 
> > (isxdigit(buff[i])) ....
>
> Done
> > some helper macros for the conversion starting on line 737 would be really
> >
> > nice. If designed properly, it would be the possible to do just something line 
> > hex = 16*first_val+second_val
>
> Done
> > #0007: Ack
> >
> > #0008: Ack
>
> Next patch got squashed with this one.
> > #0009:
> > a bunch of trailing whitespace errors
> > while you at editing this file, there is one compilation warning:
> >
> > ini/ini_parse_ut.c: In function ‘read_again_test’:
> > ini/ini_parse_ut.c:208:11: warning: variable ‘srcdir’ set but not used [-
> > Wunused-but-set-variable]
>
> Removed in the re-factored first patch
> > after you call ini_get_int_config_value() on line 1192 of the file, you should
> >
> > check for unexpected state and return if such state occurred (didn't receive 
> > error)
> > delete confusing comments on lines 1209 and 1216 of the file (/* We expected
> >
> > error in this case */)
> > nitpick: you are missing space after if on line 1298 of the file
> >
> > (if(number_unsigned != 3) {)
>
> All addressed.
> > as a general comment/question: do your test fail if the expected value
> >
> > differs from the value found in config file? It doesn't look like that and for 
> > some tests it could be helpful (for example those int/double arrays, I was 
> > confused at to what values should I expect)
>
> Test should fail if function returned something we did not expect.
> > #0010: Ack but please correct the typo in commit message (descryption -> 
> > description)
>
> Done.
> > Thanks
> > Jan
>
> Next version of patches (9 of them now) attached.
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I found an issue and refined patch
0006-INI-The-implementation-of-the-new-interface.patch
I also added a new test for the condition into the patch
0008-INI-Added-new-tests-for-the-multi-value-keys.patch
All the rest is the same.
New patches are attached.

[Dmitri Pal]

Hi,

And again the same set + one patch that fixes Makefile.am

[Dmitri Pal]

On 09/26/2012 02:40 PM, Dmitri Pal wrote:
> Hi,
>
> And again the same set + one patch that fixes Makefile.am
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Gunther provided a review and found two issues.

--------------------------------------------------

In ini/ini_get_valueobj.c:ini_get_config_valueobj() line 132 the 
function returns "error" which is still set to EOK while I think EINVAL 
is more appropriate here:

ini/ini_get_valueobj.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ini/ini_get_valueobj.c b/ini/ini_get_valueobj.c

index 6ed822e..6587e1d 100644

--- a/ini/ini_get_valueobj.c

+++ b/ini/ini_get_valueobj.c

@@ -129,7 +129,7 @@ int ini_get_config_valueobj(const char *section,

/* Do we have a name ? */

if (name == NULL) {

TRACE_ERROR_NUMBER("Name is NULL it will not be found.", EINVAL);

-        return error;

+        return EINVAL;

}

/* Empty section means look for the default one */

Then I think there are some pointer dereferences, which theoretically 
can be NULL pointers and there are no explicit checks, like here in

ini/ini_list_valueobj.c:

ini/ini_list_valueobj.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ini/ini_list_valueobj.c b/ini/ini_list_valueobj.c

index ef53e10..dc0992f 100644

--- a/ini/ini_list_valueobj.c

+++ b/ini/ini_list_valueobj.c

@@ -116,7 +116,7 @@ char **ini_get_attribute_list(struct ini_cfgobj 
*ini_config,

/* Pass it to the function from collection API */

list = col_collection_to_list(subcollection, size, error);

-

+    /* cant list be NULL at this point ? */

col_destroy_collection(subcollection);

/* Our list of attributes has a special extra attribute - remove it */

--------------------------------------------------------------------

Both are in patch 6.
At first I was not sure about the first issue but after a discussion with Stephen I decided to make a change.
The new patch set together with the coverity fix that was sent separately is included.

[Jakub Hrozek]

On Tue, Oct 02, 2012 at 01:03:28PM -0400, Dmitri Pal wrote:
> On 09/26/2012 02:40 PM, Dmitri Pal wrote:
> > Hi,
> >
> > And again the same set + one patch that fixes Makefile.am
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Gunther provided a review and found two issues.
>
> In ini/ini_get_valueobj.c:ini_get_config_valueobj() line 132 the 
> function returns "error" which is still set to EOK while I think EINVAL 
> is more appropriate here:
>
> ini/ini_get_valueobj.c | 2 +-
>
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ini/ini_get_valueobj.c b/ini/ini_get_valueobj.c
>
> index 6ed822e..6587e1d 100644
>
> --- a/ini/ini_get_valueobj.c
>
> +++ b/ini/ini_get_valueobj.c
>
> @@ -129,7 +129,7 @@ int ini_get_config_valueobj(const char *section,
>  /* Do we have a name ? */
>
>  if (name == NULL) {
>
>      TRACE_ERROR_NUMBER("Name is NULL it will not be found.", EINVAL);
>
>    return error;
>
>    return EINVAL;
>
> }
>
> /* Empty section means look for the default one */
>
> Then I think there are some pointer dereferences, which theoretically 
> can be NULL pointers and there are no explicit checks, like here in
>
> ini/ini_list_valueobj.c:
>
> ini/ini_list_valueobj.c | 2 +-
>
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ini/ini_list_valueobj.c b/ini/ini_list_valueobj.c
>
> index ef53e10..dc0992f 100644
>
> --- a/ini/ini_list_valueobj.c
>
> +++ b/ini/ini_list_valueobj.c
>
> @@ -116,7 +116,7 @@ char **ini_get_attribute_list(struct ini_cfgobj 
> *ini_config,
>  /* Pass it to the function from collection API */
>
>  list = col_collection_to_list(subcollection, size, error);
>
> /* cant list be NULL at this point ? */
>
> col_destroy_collection(subcollection);
>
> /* Our list of attributes has a special extra attribute - remove it */
>
> Both are in patch 6.
> At first I was not sure about the first issue but after a discussion with Stephen I decided to make a change.
> The new patch set together with the coverity fix that was sent separately is included.
>
> -- 
> Thank you,
> Dmitri Pal
>
> Sr. Engineering Manager for IdM portfolio
> Red Hat Inc.
>
> Looking to carve out IT costs?
> www.redhat.com/carveoutcosts/

As all Guenther's review issues were addressed, I pushed the patches to
master.