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

Dmitri Pal dpal at redhat.com
Wed Sep 26 13:03:20 UTC 2012


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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-INI-Fix-permission-checking-unit-test.patch
Type: text/x-patch
Size: 17742 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-INI-Added-new-tests-for-the-multi-value-keys.patch
Type: text/x-patch
Size: 35396 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-INI-Start-building-the-new-interface.patch
Type: text/x-patch
Size: 700 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-INI-The-implementation-of-the-new-interface.patch
Type: text/x-patch
Size: 39965 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-INI-Add-a-search-state-to-the-config-object.patch
Type: text/x-patch
Size: 2911 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-INI-Definition-of-the-new-INI-interface.patch
Type: text/x-patch
Size: 59278 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-INI-Adding-couple-functions-to-value-object.patch
Type: text/x-patch
Size: 4772 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-INI-No-need-to-copy-file-in-configure-any-more.patch
Type: text/x-patch
Size: 858 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-INI-Build-docs-for-new-interface.patch
Type: text/x-patch
Size: 972 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120926/593d475e/attachment-0008.bin>


More information about the sssd-devel mailing list