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

Dmitri Pal dpal at redhat.com
Wed Aug 8 13:51:28 UTC 2012


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.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


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






More information about the sssd-devel mailing list