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

Nack,
- a bunch of trailing whitespace errors
- wrong position of "%s" on line 98 of the patch, similarly on lines 195 and
306
- 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 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

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
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
-- 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

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

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: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/