[SSSD] [PATCHES] Merging INI snippets

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 19 20:16:15 UTC 2014


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.
>
>>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'
>.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.
>
>>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.
>
>>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.
>
>>+#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.
>>+
>>+        /* 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; 
>
>>+            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?
>
>>+    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.
>
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);


>     /* 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



More information about the sssd-devel mailing list