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 <d...@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 <d...@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 <d...@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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
