[SSSD] [PATCHES] Merging INI snippets

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 19 17:02:37 UTC 2014


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.

LS



More information about the sssd-devel mailing list