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@(a)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@(a)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(a)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(a)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(a)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(a)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.