[SSSD] Request for review of the new INI interface

Pavel Březina pbrezina at redhat.com
Tue Dec 2 12:50:00 UTC 2014


On 11/30/2014 09:15 PM, Dmitri Pal wrote:
> 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
>> alreadyavailable 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.

I think there should be one function that would serialize the ini object 
and save it to a path. Possibly with creating backup of the original 
configuration.

>> 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.

Sort may be useful when creating a new file but I think that can be 
easily done on the caller side by simply sorting the 'add' directives.

But when you modify an existing configuration file IMHO you want to keep 
the structure intact to not mess with admin's ordering.

My comments to the header file:

1.
Can you make INI_VA_* constants an enum type since they are exclusive? 
Also the description of those constants is not very clear to me, Are 
they supposed to tell which element will be updated or which element 
will be returned? Because you are not returning any element from the 
functions.

  * For example if you have five duplicates and you are searching for 
the tenth
  * the function will find and return fifth instance.
s/return/update, maybe?

2.
I think that many sentences are missing comma. For example:
"Since duplicate sections are not allowed any constants that allow 
addition of duplicates will cause function to return an error."

I'm not sure whether the comma is mandatory or not, but the sentence 
definitely reads better when you put comma after 'allowed' (i.e. 
subordinate clause ends).
"Since duplicate sections are not allowed, any constants that allow 
addition of duplicates will cause function to return an error."

3.
const char *comments[],

It is not clear from the description why it is an array. Does each 
element represent a single line? Isn't a single string enough?

4.
int position,
const char *other_section,
int idx

I'd like to see some usage examples of those three parameters in 
description. And what happens if other_section is NULL?




More information about the sssd-devel mailing list