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

Dmitri Pal dpal at redhat.com
Tue Sep 25 23:06:00 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"

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.

-- 
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 --------------
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/20120925/4aba1693/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: 33944 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120925/4aba1693/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/20120925/4aba1693/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: 39951 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120925/4aba1693/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/20120925/4aba1693/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/20120925/4aba1693/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/20120925/4aba1693/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/20120925/4aba1693/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/20120925/4aba1693/attachment-0008.bin>


More information about the sssd-devel mailing list