[SSSD] [INI] Assorted patches for ding-libs

Jan Zelený jzeleny at redhat.com
Fri Aug 3 12:28:04 UTC 2012


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120803/d38c751f/attachment.sig>


More information about the sssd-devel mailing list