[SSSD] Request for review of the new INI interface

Dmitri Pal dpal at redhat.com
Tue Dec 2 21:53:25 UTC 2014


On 12/02/2014 07:50 AM, Pavel Březina wrote:
> 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.

Several questions:
1) So the signature should be something like: ini_config_save(configobj, 
const char *path, int flags, struct access);
where
- "flags" is enum that controls what to do with the file if it already 
exists.
- access should define permissions and ownership of the new file being 
created.
IMO not providing a way to set permissions and ownership will be wrong 
but also makes the function quite complex so it is not sure whether a 
generic function like this is really granted.
Is it?

2) What is the pattern I should use for bakup? Should I keep only one 
backup and rewrite previous backup if any? Or should function should 
always find a way to make a unique name by adding numbers or some other 
suffix? Which? What should be the permissions and ownership on the 
backup file? Should they be carried forward from the old config, or the 
ownership for bakup should be the same as for the new file or should 
there be another argument that would define permissions for backup? 
Should caller be able to specify template for the backup names? Should 
he be able to specify a directory where the backup should be made?



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

I am not sure I agree. The file can become a mess quite quickly if 
different parts of it are changed by different puppet scripts and in 
addition admin messed with it manually.
Sort will sort of bring the file to a canonical form.

>
> My comments to the header file:
>
> 1.
> Can you make INI_VA_* constants an enum type since they are exclusive?

OK, I will.

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

The define what to do: insert new key-value without any checks, update 
only if key exists, return error if key exists, remove all instances of 
the key first and then add to the end, etc.
It really defines the operation to perform. I will make it more clear.


> 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?
Yes. I would say "use 5th instance in the operation that it needs to 
perform".

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

You are right, I will review those.

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

/* Let me just explain
  * why one line comments
  * are not sufficient.
  * Comments can span multiple lines and also
  * empty lines before and after the comments text
  *  are a part of the comment.
  */

key = value
...




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


There will be in unit test but in a nutshell:
position in general for keys and sections defines where to put the 
key/section being added.
If we talk about keys the options are: at the beginning of the section, 
at the end, as Xth key in the section, before other key, after other 
key, or as a duplicate of the other key.
In cases when "other key" is needed function uses the value of "other 
key" as a relation point.
If we want to do something with Xth key in the section or Yth duplicate 
of the key the idx is used as index.

May be I need to create a table that describes all the combinations.

Similar for sections.



>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.




More information about the sssd-devel mailing list