7:38 p.m.
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.
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.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.
2:15 p.m.
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.
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.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.
6:56 p.m.
----- Original Message -----
From: "Dmitri Pal" <dpal(a)redhat.com>
To: sssd-devel(a)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):
1. The header starts with...
-- snip --
#ifndef INI_CONFIGMOD_H
#define INI_CONFIGMOD_H
-- snip --
... but at the end there is no |#endif|
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).
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.) ?
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 --
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) rmainz(a)redhat.com
\__\/\/__/ IPA/Kerberos5 team
/O /==\ O\
(;O/ \/ \O;)
9:15 p.m.
On 11/30/2014 07:56 PM, Roland Mainz wrote:
----- Original Message -----
> From: "Dmitri Pal" <dpal(a)redhat.com>
> To: sssd-devel(a)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.
10:52 a.m.
On Sun, 30 Nov 2014 22:15:01 -0500
Dmitri Pal <dpal(a)redhat.com> wrote:
> 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.
wchar_t is also completely deprecated and discourage, and should never
be used anyway.
Although you could perhaps get a string as separator (char *sep),
rather than a single char.
This way you could, in theory, use utf-8 multibyte chars, it also means
some people may decide to use multiple chars as a separtor (like := ),
not sure if this is a problem or an advantage.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11:06 a.m.
On 12/01/2014 11:52 AM, Simo Sorce wrote:
On Sun, 30 Nov 2014 22:15:01 -0500
Dmitri Pal <dpal(a)redhat.com> wrote:
>> 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.
wchar_t is also completely deprecated and discourage, and should never
be used anyway.
Although you could perhaps get a string as separator (char *sep),
rather than a single char.
This way you could, in theory, use utf-8 multibyte chars, it also means
some people may decide to use multiple chars as a separtor (like := ),
not sure if this is a problem or an advantage.
This is not supported on the parsing side and I am not aware of any use
case where it is actually needed.
I think making it a string would be really asking for trouble.
Simo.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.
6:50 a.m.
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?
3:53 p.m.
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.
3433
days inactive
3435
days old
sssd-devel@lists.fedorahosted.org
7 comments
4 participants
participants (4)
-
Dmitri Pal -
Pavel Březina -
Roland Mainz -
Simo Sorce