[SSSD] Request for review of the new INI interface

Dmitri Pal dpal at redhat.com
Mon Dec 1 03:15:01 UTC 2014


On 11/30/2014 07:56 PM, Roland Mainz wrote:
>
> ----- Original Message -----
>> From: "Dmitri Pal" <dpal at redhat.com>
>> To: sssd-devel at lists.fedorahosted.org
>> Sent: Sunday, November 30, 2014 9:15:50 PM
>> Subject: Re: [SSSD] Request for review of the new INI interface
>>
>> On 11/29/2014 08:38 PM, Dmitri Pal wrote:
>>
>>
>> Hello,
>>
>> I have been working on an interface to modify configuration file (yes
>> alternative to augeas) as promised.
>> INI supports multi valued keys so the interface needs to be able to manage
>> duplicates.
>>
>> The attached header file contains the prototype of the interface.
>> I have implemented value management and about to do the section management.
>> That part would be quite simple.
>> Before I polish everything up, hook together and start sending patches I
>> wanted to make sure the interface is OK and is not missing anything.
>>
>> I was not sure how much of the design page is needed.
>> I will probably create one based on this thread with a pointer to header
>> file.
>>
>> The assumption is that:
>> - the caller will read an existing configuration object from a file or create
>> a configuration object himself. These calls are already available for some
>> time.
>> - the caller will then add/updated/delete the configuration object as he sees
>> fit. This is this interface.
>> - the caller then will call already existing serialization function that will
>> return a simple buffer object that will contain a serialized configuration
>> ready to be stored in a file.
>> - the caller can use simple buffer methods to write content of the buffer to
>> a file descriptor or just create a file (FILE *), get the serialized buffer
>> and write to that file. It is up to the caller and there is so far no plan
>> to provide a specific "save function" as it is unclear what would be a good
>> function to have.
>>
>>
>> Let me know if any of these assumptions wrong.
>> Please take a look at the file, it has doxygen comments. It is a sanity check
>> exercise not a thorough review :-).
>> The comments refer to collection interface so I included it too for easier
>> reading.
>>
>> Please see attached a new version of the header.
>>
>> Also I wonder if I should provide a way to sort the sections and keys in a
>> specific order.
>> Would it make sense to have a way to say:
>> - sort the keys in sections that match a specific pattern in the following
>> way: first keys that match pattern a, then b, then c... and then all the
>> rest as they are.
>> - sort the sections in such a way that sections with name matching the
>> following patter should go first, then other pattern next, then other
>> pattern and so on.
>>
>> Would that will be useful? I think so, but before I spend time on it I wanted
>> to double check.
> Nitpicking (mostly minor nits):

Thanks for the review.

> 1. The header starts with...
> -- snip --
> #ifndef INI_CONFIGMOD_H
> #define INI_CONFIGMOD_H
> -- snip --
> ... but at the end there is no |#endif|

Yes, I figured that when I started compiling.

> 2. |int idx| screams for trouble if the API is used for something larger than the current consumers (AFAIK |ssize_t| should be used instead).

I am not sure I follow.
Anyways I can make it size_t but it will be inconsistent with the 
underlying functions from the "collection" interface.
I am not sure I see the trouble though.
There is no array under. This is a "pseudo" index based on counting the 
elements of the link list rather than an offset into an array.
I will make sure I check for negatives but other than that it is pretty 
safe.
> 3. You have support for |double| ... what about other precisions (e.g. |binary16|, |binary32|(=|float|), etc. - likely no demand, right ?) and supporting C99 hexfloat format (which you should get automagically if you use POSIX |strtod()|&&co.) ?

So far everything can be expressed as strings. All the rest is 
convenience. So that lack of these functions does not mean the lack of 
functionality. If there will be a demand they indeed can be added.

> 4. From an API standpoint it's not wise to use bytes as characters, e.g. |sep| should be a |wchar_t| (=uses current locale widechar representation, or |char32_t| from C1X, which is UTF32-encoded then, both |wchar_t| and |char32_t| use ASCII as basis so there is no actual conversion needed for now) below (not that the code is handling multibyte characters right now... but at least the API should anticipate this):
> -- snip --
> int ini_config_add_str_arr_value(struct ini_cfgobj *ini_config,
>                                   const char *section,
>                                   const char *key,
>                                   const char *value_str_arr[],
>                                   size_t count_str,
>                                   char sep,
> -- snip --

While I agree in general I think that requiring separators to be ASCII 
characters in the value in an INI file is an acceptable simplification.
It is also defined in parsing that INI file can contain separators that 
are ASCII characters.

> ----
>
> Bye,
> Roland
>


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.




More information about the sssd-devel mailing list