[SSSD] [PATCHES] Merging INI snippets

Dmitri Pal dpal at redhat.com
Sat Nov 29 06:03:44 UTC 2014


On 11/19/2014 03:16 PM, Lukas Slebodnik wrote:
> On (19/11/14 18:02), Lukas Slebodnik wrote:
>> On (19/10/14 22:36), Dmitri Pal wrote:
>> Firstly, I would like to appologize for late review.

Thanks for review!
Not a problem at all.
It is not a high priority.

>>
>>> On top of the original 6+1 patches which I did not touch and squash (I just
>>> changed the order of them as suggested by Lukas plus put 7th patch right
>>> after the change to module ini_augment.c)
>>> I added 3 patches:
>>> - 0006-Fixing-the-main.path - addresses all feedback provided above. I Hope I
>>> did not miss anything. I did not squash it so it is easy to see that things
>>> have been fixed.
>>> - 0008-Cleaning-Unit-test.patch - does some cleaning of the code in the unit
>>> test and also makes it not fail in your case by sorting the results before
>>> comparison. See the not in the patch
>>> - 0010-Correction-to-make-file.patch - corrects the make file and fixes
>>> symbols file as Lukas proposed. The only thing that is not addressed is the
>>> recommendation for linking ref_array explicitly. I am not sure I get it. The
>>> library libini_config_la already includes everything needed see below (line
>>> 261 Makefile.am):
>> There are distributions which have disabled link_all_dep_libs (debian)
>> It means that linker will not use dependencies from your libraries.
>> And it will cause linker errors.
>>
>>   CCLD     ini_augment_ut
>> /usr/bin/ld: ini/ini_augment_ut.o: undefined reference to symbol 'make_normalized_absolute_path@@PATH_UTILS_0.2.1'
>> .libs/libpath_utils.so.1: error adding symbols: DSO missing from command line
>> collect2: error: ld returned 1 exit status
>> Makefile:1140: recipe for target 'ini_augment_ut' failed
>> make[1]: *** [ini_augment_ut] Error 1
>>
>> and after linking with libpath_utils, there is an another error.
>>
>> /usr/bin/ld: ini/ini_augment_ut.o: undefined reference to symbol 'ref_array_get@@REF_ARRAY_0.1.1'

I added libraries to build unit test.

>> .libs/libref_array.so.1: error adding symbols: DSO missing from command line
>> collect2: error: ld returned 1 exit status
>> Makefile:1140: recipe for target 'ini_augment_ut' failed
>> make[1]: *** [ini_augment_ut] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make[1]: Leaving directory '/dev/shm/ding-libs_build'
>>
>>
>>
>> You need to link unit tests with libraries which are used in unit test:
>> Here is a prrof of used functions in ini_augment_ut. (weak symbols and glibc
>> functions are filtered out).
>> sh-4.3$ objdump -T .libs/ini_augment_ut | grep -v GLIBC | grep -v " w "
>>
>> .libs/ini_augment_ut:     file format elf64-x86-64
>>
>> DYNAMIC SYMBOL TABLE:
>> 0000000000000000      DF *UND*  0000000000000000  INI_CONFIG_1.1.0 ini_config_augment
>> 0000000000000000      DF *UND*  0000000000000000  INI_CONFIG_1.1.0 ini_config_destroy
>> 0000000000000000      DF *UND*  0000000000000000  INI_CONFIG_1.1.0 ini_config_create
>> 0000000000000000      DF *UND*  0000000000000000  REF_ARRAY_0.1.1 ref_array_destroy
>> 0000000000000000      DF *UND*  0000000000000000  REF_ARRAY_0.1.1 ref_array_get
>> 0000000000000000      DF *UND*  0000000000000000  PATH_UTILS_0.2.1 make_normalized_absolute_path
>> 0000000000000000      DF *UND*  0000000000000000  COLLECTION_0.6.2 col_debug_collection
>>                                                   ^^^^^^^^^^^^^^^^
>>                                 BTW: This is a benefit of version symbol files.
>>> libini_config_la_LIBADD = \
>>>     libcollection.la \
>>>     libpath_utils.la \
>>>     libref_array.la \
>>>     libbasicobjects.la
>>>
>>
>>> Also I did not merge the proposed sorting function patch. I had to address
>>> de-referencing issue to I blended it in first and then add additional cleanup
>>> on top.
>>>
>>> Once reviewed the patches 4-5-6, 7-8, 9-10 can be squashed respectfully
>>> before pushing.
>>>
>> It is not problem to see changes between two patches with utility interdiff
>> I would prefer to squash changes; then it's easier to review patches.

OK squashed.

>>
>>> Thanks for review!
>>>
>>> -- 
>>> Thank you,
>>> Dmitri Pal
>>>
>>> Sr. Engineering Manager IdM portfolio
>>> Red Hat, Inc.
>>>
>> >From c443d984a19df8fbbdb7337563ea4ec0ea2d693d Mon Sep 17 00:00:00 2001
>>> From: Dmitri Pal <dpal at dpal.csb>
>>> Date: Sun, 7 Sep 2014 22:55:02 +0200
>>> Subject: [PATCH 09/10] [INI] Make the merge function build
>>>
>>> ---
>>> Makefile.am           |   15 +++++-
>>> ini/ini_configobj.h   |  115 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> ini/libini_config.sym |    1 +
>>> 3 files changed, 128 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index 32fcfae..7f38e50 100644
>>> --- a/Makefile.am
>> //snip
>>
>>> +ini_augment_ut_SOURCES = ini/ini_augment_ut.c
>>> +ini_augment_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
>>> +
>> libraries should be fixed as I explained few lines above.

Yes, done.

>>
>> >From 9bad4d86ab68b1af1c0afd06ae826dd27aed4ef3 Mon Sep 17 00:00:00 2001
>>> From: Dmitri Pal <dpal at dpal.csb>
>>> Date: Sun, 7 Sep 2014 16:42:00 +0200
>>> Subject: [PATCH 04/10] [INI] New function to merge snippets
>>>
>>> The patch includes the implementation of the function
>>> and the unit test
>>> ---
>>> ini/ini_augment.c    |  924 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> ini/ini_augment_ut.c |  353 +++++++++++++++++++
>>> 2 files changed, 1277 insertions(+), 0 deletions(-)
>>> create mode 100644 ini/ini_augment.c
>>> create mode 100644 ini/ini_augment_ut.c
>>>
>>> diff --git a/ini/ini_augment.c b/ini/ini_augment.c
>>> new file mode 100644
>>> index 0000000..7f59d38
>>> --- /dev/null
>>> +++ b/ini/ini_augment.c
>>> @@ -0,0 +1,924 @@
>>> +/*
>>> +    INI LIBRARY
>>> +
>>> +    Module represents part of the INI interface.
>>> +    The main function in this module allows to merge
>>> +    snippets of different config files.
>>> +
>>> +    Copyright (C) Dmitri Pal <dpal at redhat.com> 2014
>>> +
>>> +    INI Library is free software: you can redistribute it and/or modify
>>> +    it under the terms of the GNU Lesser General Public License as published by
>>> +    the Free Software Foundation, either version 3 of the License, or
>>> +    (at your option) any later version.
>>> +
>>> +    INI Library is distributed in the hope that it will be useful,
>>> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +    GNU Lesser General Public License for more details.
>>> +
>>> +    You should have received a copy of the GNU Lesser General Public License
>>> +    along with INI Library.  If not, see <http://www.gnu.org/licenses/>.
>>> +*/
>>> +
>>> +#define _GNU_SOURCE
>>           ^^^^^^^^^^^
>> Which function requires this defined macro?
>> I didn't have a problem after removing this macro.
>> It would be good to add comment (at least) or remove it.


Comment added.

>>
>>> +#include "config.h"
>>> +#include <errno.h>
>>> +#include <stdarg.h>
>>> +#include <dirent.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <limits.h>
>>> +#include <sys/types.h>
>>> +#include <regex.h>
>>> +#include "trace.h"
>>> +#include "collection.h"
>>> +#include "collection_tools.h"
>>> +#include "ini_configobj.h"
>>> +#include "ini_config_priv.h"
>>> +#include "ini_defines.h"
>>> +#include "path_utils.h"
>>> +
>>> +/* Constants to match */
>>> +#define INI_CURRENT_DIR "."
>>> +#define INI_PARENT_DIR ".."
>>> +
>>> +/* Size of incremental growth for ref of the array of strings */
>>> +#define INI_AUG_ARR_SIZE_INC 50
>>> +
>>> +
>> //snip
>>
>>> +
>>> +/* Prepare array of regular expressions */
>>> +static int ini_aug_regex_prepare(const char **patterns,
>>> +                                 struct ref_array *ra_err,
>>> +                                 struct ref_array **ra_regex)
>>> +{
>>> +    int error = EOK;
>>> +    int reg_err = 0;
>>> +    char **pat = NULL;
>>> +    struct ref_array *ra = NULL;
>>> +    regex_t *preg = NULL;
>>> +    size_t buf_size = 0;
>>> +    char *err_str = NULL;
>>> +
>>> +    TRACE_FLOW_ENTRY();
>>> +
>>> +    if (patterns) {
>>> +
>>> +        /* Create array to mark bad patterns */
>>> +        error = ref_array_create(&ra,
>>> +                                 sizeof(regex_t *),
>>> +                                 INI_AUG_ARR_SIZE_INC,
>>> +                                 regex_cleanup,
>>> +                                 NULL);
>>> +        if (error) {
>>> +            TRACE_ERROR_NUMBER("Failed to create array.", error);
>>> +            return error;
>>> +        }
>>> +
>>> +        memcpy(&pat, &patterns, sizeof(char **));
>>           You are assigning pointer "patterns" to pointer "pat" with memcpy.

Because I could not figure out how to deal with consts and had to work 
around it.
But your suggestion below finally hinted to how it can be done without 
hacks.


>>> +
>>> +        /* Run through the list and save precompiled patterns */
>>> +        while (*pat) {
>>           ^^^^^^^^^^^^^^
>>           the for loop will improve readability of this section and
>>           remove incrementing of "pat" on two places.
>>           Something like this:
>>
>>           char *pattern;
>>           size_t i;
>>           for (i = 0; patterns[i] != NULL; i++) {
>>               pattern = patterns[i];
>>
>>           // or to use pointer arithmetics
>>           // and we needn't use temporary variable "pat" here
>>           // we can directly use function argument "patterns"
>>           for (; *patterns != NULL; patterns++) {
>>               pattern = *patterns;


Addressed.

>>
>>> +            TRACE_INFO_STRING("Pattern:", *pat);
>>> +
>>> +            preg = calloc(1, sizeof(regex_t));
>>> +            if (preg == NULL) {
>>> +                TRACE_ERROR_NUMBER("Failed to create array.", ENOMEM);
>>> +                ref_array_destroy(ra);
>>> +                return ENOMEM;
>>> +            }
>>> +            if (reg_err) {
>>> +                /* Get size, allocate buffer, record error... */
>>> +                buf_size = regerror(reg_err, preg, NULL, 0);
>>> +                err_str = malloc (buf_size);
>>> +                if (err_str == NULL) {
>>> +                    TRACE_ERROR_NUMBER("Failed to create array.", ENOMEM);
>>> +                    ref_array_destroy(ra);
>>> +                    free(preg);
>>> +                    return ENOMEM;
>>> +                }
>>> +                regerror(reg_err, preg, err_str, buf_size);
>>> +                free(preg);
>>> +                ini_aug_add_string(ra_err,
>>> +                                   "Failed to process expression: %s."
>>> +                                   " Compilation returned error: %s",
>>> +                                   *pat, err_str);
>>> +
>>> +                /* All error processing is done - advance to next pattern */
>>> +                pat++;
>>> +                continue;
>>> +            }
>>> +            /* In case of no error add compiled expression into the buffer */
>>> +            error = ref_array_append(ra, (void *)&preg);
>>> +            if (error) {
>>> +                TRACE_ERROR_NUMBER("Failed to add element to array.", error);
>>> +                ref_array_destroy(ra);
>>> +                free(preg);
>>> +                return error;
>>> +            }
>>> +            /* Advance */
>>> +            pat++;
>>> +        }
>>> +    }
>>> +
>>> +    *ra_regex = ra;
>>> +    /* ref_array_debug(*ra_regex, 1); */
>>> +
>>> +    TRACE_FLOW_EXIT();
>>> +    return EOK;
>>> +}
>>> +/* Check if this is a file and validate permission */
>>> +static bool ini_check_file_perm(char *name,
>>> +                                struct access_check *check_perm,
>>> +                                struct ref_array *ra_err)
>>> +{
>>> +    bool ret = false;
>>> +    int error = EOK;
>>> +    struct stat file_info;
>>> +
>>> +    TRACE_FLOW_ENTRY();
>>> +
>>> +    errno = 0;
>>> +    if (stat(name, &file_info) == -1) {
>>> +        error = errno;
>>> +        TRACE_ERROR_NUMBER("Failed to get file stats", error);
>>> +        ini_aug_add_string(ra_err,
>>> +                           "Failed to read metadata for file %s."
>>> +                           " Skipping.",
>>> +                           name);
>>> +        return false;
>>> +    }
>>> +
>>> +    if (!S_ISREG(file_info.st_mode)) {
>>> +        ini_aug_add_string(ra_err,
>>> +                           "File %s is not a regular file. Skipping.",
>>> +                           name);
>>> +        return false;
>>> +    }
>>> +
>>> +    if ((check_perm) && (check_perm->flags)) {
>>> +        error = access_check_int(&file_info,
>>> +                                 check_perm->flags,
>>> +                                 check_perm->uid,
>>> +                                 check_perm->gid,
>>> +                                 check_perm->mode,
>>> +                                 check_perm->mask);
>>> +        if(error) {
>>> +            TRACE_ERROR_NUMBER("Access check returned", error);
>>> +            ini_aug_add_string(ra_err,
>>> +                               "File %s did not pass access check. Skipping.",
>>> +                               name);
>>> +            return false;
>>> +        }
>>> +    }
>>> +
>>> +    ret = true;
>>> +
>>> +    TRACE_INFO_STRING("Returning", (ret ? "true" : "false"));
>>                                       ^^^^
>>                                      ret will be always true here.
>>           Did you plan to have label "done" and to use got done instead of
>>           return false?

Leftover. Removed.

>>
>>> +    TRACE_FLOW_EXIT();
>>> +
>>> +    return ret;
>>> +}
>>> +/* Function to merge additional snippets of the config file
>>> + * from a provided directory.
>>> + */
>>> +int ini_config_augment(struct ini_cfgobj *base_cfg,
>>> +                       const char *path,
>>> +                       const char **patterns,
>>> +                       const char **sections,
>>                          ^^^^^^^^^^^^^^^^^^^^^
>>       It would be good to change type of arguments "patterns" and "sections"
>>                "const char **" -> "const char * var_name[]"
>>       It will be clear that we expect array of constant strings and they aren't
>>       output arguments. After this change, you need't use explicit casting
>>       in unit tests.
>>

Fixed as mentioned above.

> I found yet another note after cleaning my workspace. (I forgot about it)
>
>
> >From 442bfc2c665ff18fce539106812e855ca542a5be Mon Sep 17 00:00:00 2001
>> From: Dmitri Pal <dpal at dpal.csb>
>> Date: Mon, 29 Sep 2014 07:38:17 -0400
>> Subject: [PATCH 05/10] [INI] Improve sorting and scanning
>>
>> The sort function is replced with locale based one.
>> The function to read directory entries is replaced with the reentrant one.
>> ---
>> ini/ini_augment.c |   30 ++++++++++++++++++++++++++----
>> 1 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/ini/ini_augment.c b/ini/ini_augment.c
>> index 7f59d38..13ab882 100644
>> --- a/ini/ini_augment.c
>> +++ b/ini/ini_augment.c
>>
> //snip
>
>> @@ -385,9 +389,24 @@ static int ini_aug_construct_list(char *dirname ,
>>          return EOK;
>>      }
>>
>> +    /* Allocate memory for entry (as said in man pages)*/
>> +    len = offsetof(struct dirent, d_name) + pathconf(dirname, _PC_NAME_MAX) + 1;
>> +    entry = malloc(len);
> It would be better to use the appoach as in manual page
>             name_max = pathconf(dirpath, _PC_NAME_MAX);
>             if (name_max == -1)         /* Limit not defined, or error */
>                 name_max = 255;         /* Take a guess */
>             len = offsetof(struct dirent, d_name) + name_max + 1;
>             entryp = malloc(len);

My man page showed exactly what I did but I added the code you 
suggested. I prefer s/255/1024

>
>>      /* Loop through the directory */
>> -    while ((entry = readdir(dir)) != NULL)
>> +    while (true)
>>      {
>> +        error = readdir_r(dir, entry, &entryp);
>> +        if (error) {
>> +            TRACE_ERROR_NUMBER("Failed to rid directory.", error);
>> +            ref_array_destroy(ra_regex);
>> +            free(entry);
>> +            return error;
>> +        }
> LS

New set is attached.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-RA-Print-info-when-array-is-empty.patch
Type: text/x-patch
Size: 730 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-INI-Declaring-new-internal-access-check-function.patch
Type: text/x-patch
Size: 882 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-INI-Refactored-access-control-check.patch
Type: text/x-patch
Size: 4167 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-INI-New-function-to-merge-snippets.patch
Type: text/x-patch
Size: 42031 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-INI-Test-file-for-unit-test.patch
Type: text/x-patch
Size: 7684 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-INI-Make-the-merge-function-build.patch
Type: text/x-patch
Size: 7856 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141129/140882d2/attachment-0005.bin>


More information about the sssd-devel mailing list