#2986 Add possibility to parse only some sections in ini files
Closed: Invalid None Opened 4 years ago by mzidek.

ini_config_file_parse always parses the entire config file and if some section contain attributes in format that libini does not understand, then the parsing process fails.

In some cases (parsing GPO files) we are interested only in specific sections and the errors from other sections should not stop us from parsing the ini file.

It would be good to add new function to libini that would pass list of sections that we are interested in and completely ignore any other sections.

Related ticket:
https://fedorahosted.org/sssd/ticket/2751


There is already a way to ignore errors.
IMO instead of skipping sections the calling application should use the flags properly when calling parsing.

See error_level parameter below. Instead of failing on errors the parser should be instructed to not fail and the calling code should inspect errors. I suggest that if wee need more information stored during error processing we should do that but I would argue that it would be better to look at errors and check if they are critical after the parsing rather than specify sections to ignore.

/**
 * @brief Parse the file and populate a configuration object
 *
 * Function parses the file. It is assumed that
 * the configuration object was just created.
 * Using a non empty configuration object in
 * a parsing operation would fail with EINVAL.
 *
 * @param[in]  file_ctx         Configuration file object.
 * @param[in]  error_level      Flags that control actions
 *                              in case of parsing error.
 * @param[in]  collision_flags  Flags that control handling
 *                              of the duplicate sections or keys.
 * @param[in]  parse_flags      Flags that control parsing process,
 *                              for example how to handle spaces at
 *                              the beginning of the line.
 * @param[out] ini_config       Configuration object.
 *
 * @return 0 - Success.
 * @return EINVAL - Invalid parameter.
 * @return ENOMEM - No memory.
 */
int ini_config_parse(struct ini_cfgfile *file_ctx,
                     int error_level,
                     uint32_t collision_flags,
                     uint32_t parse_flags,
                     struct ini_cfgobj *ini_config);

Hi Dmitri,

The effect of INI_STOP_ON_NONE is that the parse does not stop when encountering error and continues parsing to the end, which allows us to get all errors from file instead of just the first one (as in case of INI_STOP_ON_ANY or INI_STOP_ON_ERROR). This allows for better error reporting, but still the returned ini_cfgobj structure can not be used if errors where encountered (it does not contain the configuration, only the errors can be extracted from it).

Ah. OK. I thought it returns the object but I guess you are right. Then I suggest we fix that and rather return the configuration object with whatever we managed to parse. Maybe it will be another flag INI_STOP_ON_NONE_BO (best effort) and then check if the config object is not null. If it is not NULL the parser was OK to return it to us. We then internally can decide what kind of parsing errors are recoverable and which ones would be so bad that it is unsafe to have an object returned to caller.

I can take a look at what that would take. I might not produce a patch but at least see what would be the effort if we go this route.

OK, I looked at the code. The easiest way to handle the GPO situation would be to introduce a new parse flag that would allow skipping lines that are not KVPs:

  1. In ini_configobj.h line 366 add a new constant
    {{{
    /* @brief Skip lines that are not KVPs /

define INI_PARSE_IGNORE_NON_KVP 0x0010

  1. In ini_parse.c In function parse_kvp line 944 change lines 966-972 to something like this:

    / Check if we have the key /
    if (*(str) == '=') {
    TRACE_ERROR_STRING("No key", str);

        if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {
            /* Clean everything as if nothing happened  */
            free(po->last_read);
            po->last_read = NULL;
            po->last_read_len = 0;
            *action = PARSE_READ;
            TRACE_FLOW_EXIT();
            return EOK;
        }
        else {
            po->last_error = ERR_NOKEY;
            *action = PARSE_ERROR;
            return EOK;
        }
    }
    

I think this would be sufficient. Please make sure you run it under valgrind to see if there is some other clean-up that is needed.

This would look better

    /* Check if we have the key */
    if (*(str) == '=') {
        TRACE_ERROR_STRING("No key", str);

        if (po->parse_flags & INI_PARSE_IGNORE_NON_KVP) {
            /* Clean everything as if nothing happened  */
            free(po->last_read);
            po->last_read = NULL;
            po->last_read_len = 0;
            *action = PARSE_READ;
        }
        else {
            po->last_error = ERR_NOKEY;
            *action = PARSE_ERROR;
        }
        TRACE_FLOW_EXIT();
        return EOK;
    }

I like the solution that Dmitri suggests for the GPO. Since there is no other real world use case for the feature described in this ticket I am closing it as WONTFIX.

resolution: => wontfix
status: new => closed

Replying to [comment:6 mzidek]:

I like the solution that Dmitri suggests for the GPO. Since there is no other real world use case for the feature described in this ticket I am closing it as WONTFIX.

Was a different ticket open to track the work that needs to be done here?

There is the original GPO issue ticket. Since the solution you provided is basically solution of that issue I do not think we need new ticket. I originally opened this ticket because I thought we will use my hotfix patch to solve the GPO issue (and close the GPO ticket) and later solve it inside ding libs (this was to track the ding libs). But it seems that we will not need to hurry with the SSSD only hotfix and we can wait for ding libs update to close the GPO ticket.

However I will modify the description of the GPO ticket to make it clear that it is solved on ding libs side and how.

Metadata Update from @mzidek:
- Issue set to the milestone: NEEDS_TRIAGE

3 years ago

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/4027

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Login to comment on this ticket.

Metadata