Hi,
I started looking at the final patch to do the merging of the sections
and thus two independent config files in INI. I looked at the patch I
started a year ago. Looks like a reasonable start however I have a question.
Right now entirely from the POW of completeness the following merge
options are identified:
/**
* @defgroup collisionflags Flags that define collision resolution logic.
*
* @{
*/
/**
* @defgroup onesecvalue Colliding values come from one section
*
* Flags that define collision resolution logic for values in
* the same section.
* These flags should be used during parsing to handle duplicate
* keys in the same section of the ini file.
*
* @{
*/
/** @brief Value with same key is ovewritten */
#define INI_MV1S_OVERWRITE 0x0000
/** @brief Collision causes error */
#define INI_MV1S_ERROR 0x0001
/** @brief Second value is discarded */
#define INI_MV1S_PRESERVE 0x0002
/** @brief Duplicates are allowed */
#define INI_MV1S_ALLOW 0x0003
/** @brief Duplicates are allowed but errors are logged */
#define INI_MV1S_DETECT 0x0004
/**
* @}
*/
/**
* @defgroup twosecvalue Colliding values come from two sections
*
* Flags that define collision resolution logic between two values
* that come from two sections with the same name.
* These flags should be used during parsing to handle duplicate
* keys coming from the same section scattered across the ini file.
* These flags also can be used to specify the rules of merging
* values that come from two files separate configuration files.
*
* @{
*/
/** @brief Value with same key is ovewritten */
#define INI_MV2S_OVERWRITE 0x0000
/** @brief Collision causes error */
#define INI_MV2S_ERROR 0x0010
/** @brief Second value is discarded */
#define INI_MV2S_PRESERVE 0x0020
/** @brief Duplicates are allowed */
#define INI_MV2S_ALLOW 0x0030
/** @brief Duplicates are allowed but errors are logged */
#define INI_MV2S_DETECT 0x0040
/**
* @}
*/
/**
* @defgroup mergesec Collision in two sections
*
* Flags that define collision resolution logic between two sections.
* These flags should be used during parsing to handle duplicate
* sections scattered across the ini file.
* These flags also can be used to specify the rules of merging
* sections that come from two separate configuration files.
*
* @{
*/
/** @brief Sections are merged */
#define INI_MS_MERGE 0x0000
/** @brief Collision causes error */
#define INI_MS_ERROR 0x0100
/** @brief First section is discarded */
#define INI_MS_OVERWRITE 0x0200
/** @brief Second section is discarded */
#define INI_MS_PRESERVE 0x0300
/** @brief Duplicates are allowed */
#define INI_MS_ALLOW 0x0400
/** @brief Duplicates are allowed but errors are logged */
#define INI_MS_DETECT 0x0500
/**
* @}
*/
/**
* @}
*/
The question is about the ALLOW and DETECT options. As a result of the
merge operation using those options one can end up with more than one
section with the same name or more than one key with the same name
inside the same section. The result will be a merged config with
potential duplicates but in case of ALLOW there will be no errors while
in case of DETECT there will be some merge errors (not implemented yet)
that can be printed later to provide the information about what is wrong
with the config.
The ALLOW mode does not make much sense for SSSD but in general case it
might be useful. However current interface that deals with querying the
configuration and fetching configuration data does not have a way to
deal with the values that can have more then one value. Duplicate values
would not cause any trouble they are just not accessible via existing
interface. The interface that is available is open for changes anyways
so there is not problem to extend it to support access to the dup keys
(if needed), however it is extra work and it is not clear whether it is
actually needed or not.
I think we need to finish the DETECT mode. It has a lot of value.
It is ALLOW mode that I am on the fence about.
So the question is what to do in this situation.
I see several options:
1) Decide that allowing duplicates is a bad idea and we will not going
to support it so wipe out ALLOW option
2) Keep the ALLOW option in header as unsupported but do not implement
it (that would mean wiping out some partial code that is already there)
3) Keep the ALLOW option in header as unsupported and do not implement
the final complex section merge logic for those cases leaving already
implemented code in place (this would mean refactoring of the patch that
otherwise is more or less OK)
4) Complete the implementation of the merge logic and indicate that
ALLOW option has no practical use (a bit ugly...).
5) Implement everything and extend the interface.
Currently I am leaning to option 4). The only concern I have is that
once this work is completed the interface would be more or less
finalized. Good news it would not be different from what we have right
now so no ABI issues but we effectively shift the ABI change into future
(if ever). I guess it is OK.
I do not like 5) because it is a lot of work. I do not like 1-3 because
it is related to wiping out useful work that might be valuable in future
(some day).
Any objections regarding my approach?
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/