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
sssd-devel@lists.fedorahosted.org
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.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/