Hello,
Extensive travel in recent months allowed me to finish this code.
Here is the updated design: https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
Patches: 0001 - Fix in case the Ref array is empty and we need to print/debug it 0002 - Declaration of the new function to do access checks 0003 - Big patch with core functionality 0004 - Updated access check code to use new internal access control function 0005 - File with expected output for unit test validation 0006 - Makefile and related changes to start building new code
No rush, take your time. :-)
On 09/10/2014 11:31 PM, Dmitri Pal wrote:
Hello,
Extensive travel in recent months allowed me to finish this code.
Here is the updated design: https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
Patches: 0001 - Fix in case the Ref array is empty and we need to print/debug it 0002 - Declaration of the new function to do access checks 0003 - Big patch with core functionality 0004 - Updated access check code to use new internal access control function 0005 - File with expected output for unit test validation 0006 - Makefile and related changes to start building new code
I am attaching a new patch on top of the other patches. It can be squashed with patch 3 after review. It improves sorting and scanning. See patch comment.
No rush, take your time. :-)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (29/09/14 07:45), Dmitri Pal wrote:
On 09/10/2014 11:31 PM, Dmitri Pal wrote:
Hello,
Extensive travel in recent months allowed me to finish this code.
Here is the updated design: https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
Patches: 0001 - Fix in case the Ref array is empty and we need to print/debug it 0002 - Declaration of the new function to do access checks 0003 - Big patch with core functionality 0004 - Updated access check code to use new internal access control function 0005 - File with expected output for unit test validation 0006 - Makefile and related changes to start building new code
I am attaching a new patch on top of the other patches. It can be squashed with patch 3 after review. It improves sorting and scanning. See patch comment.
No rush, take your time. :-)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
From aff0f323b2996df7a8f1539f46ca651d2d9fd5e8 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:38:19 +0200 Subject: [PATCH 1/6] [RA] Print info when array is empty
refarray/ref_array.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK
From 9f80182fecbf1c1800a22ff279bf8915f4196f42 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:39:58 +0200 Subject: [PATCH 2/6] [INI] Declaring new internal access check function
ini/ini_config_priv.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h
ACK
From 8f0209cb995fd88e71e96efb0b98a0256df58d5d Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:43:31 +0200 Subject: [PATCH 4/6] [INI] Refactored access control check
The patch includes implementation of the new internal function.
ini/ini_fileobj.c | 66 +++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
ACK,
but it would be better to swap order of 3rd patch and this patch (4th) reason : * the 2nd patch add declaration to the private header file * the 3rd patch use this function * the 4th patch add definition of this function.
From 09a23b41da6f7a4584f652c4a5a95bb158b0dfe2 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:42:00 +0200 Subject: [PATCH 3/6] [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
//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)+{
//snip
/* Run through the list and save precompiled patterns */while (*pat) {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;}reg_err = regcomp(preg, *pat, REG_NOSUB);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;
// memory allocated in err_str should be released // before 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;
+}
//snip
+/* Construct snippet lists based on the directory */ +static int ini_aug_construct_list(char *dirname ,
const char **patterns,struct access_check *check_perm,struct ref_array *ra_list,struct ref_array *ra_err)+{
- int error = EOK;
- DIR *dir = NULL;
- struct dirent *entry = NULL;
- char *snipname = NULL;
- char fullname[PATH_MAX + 1] = {0};
- struct ref_array *ra_regex = NULL;
- bool match = false;
- TRACE_FLOW_ENTRY();
- /* Prepare patterns */
- error = ini_aug_regex_prepare(patterns,
ra_err,&ra_regex);- if (error) {
TRACE_ERROR_NUMBER("Failed to prepare regex array.", error);return error;- }
- /* Open directory */
- errno = 0;
- dir = opendir(dirname);
- if (!dir) {
error = errno;if (error == ENOMEM) {TRACE_ERROR_NUMBER("No memory to open dir.", ENOMEM);ref_array_destroy(ra_regex);return ENOMEM;}/* Log an error, it is a recoverable error */add_dir_open_error(error, dirname, ra_err);
//ra_regex should be destroyed here.
return EOK;- }
A directory is opened, but it is not closed in case of errors in next lines. It would be cause of potential resource leak.
- /* Loop through the directory */
- while ((entry = readdir(dir)) != NULL)
- {
TRACE_INFO_STRING("Processing", entry->d_name);/* Always skip current and parent dirs */if ((strncmp(entry->d_name,INI_CURRENT_DIR,sizeof(INI_CURRENT_DIR)) == 0) ||(strncmp(entry->d_name,INI_PARENT_DIR,sizeof(INI_PARENT_DIR)) == 0)) continue;/* Match names */match = ini_aug_match_name(entry->d_name, ra_regex);if (match) {snprintf(fullname, PATH_MAX, "%s/%s", dirname, entry->d_name);if(ini_check_file_perm(fullname, check_perm, ra_err)) {/* Dup name and add to the array */snipname = NULL;snipname = strdup(fullname);if (error) {
^^^^^ snipname shouldbe tested here.
TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM);ref_array_destroy(ra_regex);
//add closedir(dir) here
return ENOMEM;}error = ref_array_append(ra_list, (void *)&snipname);if (error) {TRACE_ERROR_NUMBER("No memory to add file to ""the snippet list.",ENOMEM);ref_array_destroy(ra_regex);
//add closedir(dir) here
return ENOMEM;}}}else {ini_aug_add_string(ra_err,"File %s did not match provided patterns."" Skipping.",entry->d_name);}- }
- closedir(dir);
- ref_array_destroy(ra_regex);
- ini_aug_sort_list(ra_list);
- TRACE_FLOW_EXIT();
- return EOK;
+}
From d91b8bd98854a00448c021a42a551e5700a6ab82 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 22:53:19 +0200 Subject: [PATCH 5/6] [INI] Test file for unit test
ini/ini.d/merge.validator | 60 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 60 insertions(+), 0 deletions(-) create mode 100644 ini/ini.d/merge.validator
diff --git a/ini/ini.d/merge.validator b/ini/ini.d/merge.validator
ACK
From 9ce3ef526118ee8cf5e5dd8d28ddce971bba606e Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 22:55:02 +0200 Subject: [PATCH 6/6] [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 +++ b/Makefile.am @@ -255,6 +255,7 @@ libini_config_la_SOURCES = \ ini/ini_get_valueobj.c \ ini/ini_get_array_valueobj.c \ ini/ini_list_valueobj.c \
- ini/ini_augment.c \ trace/trace.h
libini_config_la_DEPENDENCIES = ini/libini_config.sym libini_config_la_LIBADD = \ @@ -286,19 +287,23 @@ dist_noinst_DATA += \ ini/ini.d/real32be.conf \ ini/ini.d/real32le.conf \ ini/ini.d/symbols.conf \
- ini/ini.d/new_line.conf
- ini/ini.d/new_line.conf \
- ini/ini.d/merge.validator
check_PROGRAMS += \ ini_config_ut \ ini_comment_ut \ ini_valueobj_ut \
- ini_parse_ut
- ini_parse_ut \
- ini_augment_ut
TESTS += \ ini_config_ut \ ini_comment_ut \ ini_valueobj_ut \
- ini_parse_ut
- ini_parse_ut \
- ini_augment_ut
ini_config_ut_SOURCES = ini/ini_config_ut.c ini_config_ut_LDADD = \ @@ -314,6 +319,9 @@ ini_valueobj_ut_LDADD = libini_config.la libbasicobjects.la ini_parse_ut_SOURCES = ini/ini_parse_ut.c ini_parse_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
+ini_augment_ut_SOURCES = ini/ini_augment_ut.c +ini_augment_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
The test ini_augment_ut calls functions from libpath_utils libref_array therefore it should be linked with this libraries. Otherwise it causes problems on distributions with disabled lik all deplibs.
This version works. // you can use different style of wrapping long lines. ini_augment_ut_LDADD = libini_config.la libcollection.la \ libpath_utils.la libref_array.la
ini_config-docs: if HAVE_DOXYGEN cd ini; \ @@ -324,6 +332,7 @@ clean-local-ini_config: rm -f ./*.out rm -f test.ini rm -f ./foo.conf ./bom* #From ini_parse_ut
- rm -f ./merge.validator.in #From ini_augment_ut
############################################################################## # Additional rules
//snip
diff --git a/ini/libini_config.sym b/ini/libini_config.sym index 3ca15bb..33d2246 100644 --- a/ini/libini_config.sym +++ b/ini/libini_config.sym @@ -62,6 +62,7 @@ global: ini_config_parse; ini_config_copy; ini_config_merge;
- ini_config_augment; ini_config_set_wrap; ini_config_serialize; ini_get_section_list;
It works butit is not right solution. The purpose of version symbol file is to help linker distiguish between two version of libraries with backward compatible changes. (just new functions were added). Without version symbol file, linker cannot detect differences due to the same soname.
sh$ objdump -p .libs/libini_config.so | grep SONAME SONAME libini_config.so.5
The right approach is to add new version as in refarray/libref_array.sym. I will attach file which can be squased to your patches.
From 78c072f8be307fd5a37944274cd509b22aefa5c2 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Mon, 29 Sep 2014 07:38:17 -0400 Subject: [PATCH 7/7] [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 @@ -28,9 +28,11 @@ #include <dirent.h> #include <stdio.h> #include <string.h> +#include <stddef.h> #include <limits.h> #include <sys/types.h> #include <regex.h> +#include <unistd.h> #include "trace.h" #include "collection.h" #include "collection_tools.h" @@ -320,9 +322,9 @@ static void ini_aug_sort_list(struct ref_array *ra_list) i = k; j = k + 1;
while ((j > 0) && (strncmp(*((char **) ref_array_get(ra_list, i, NULL)),*((char **) ref_array_get(ra_list, j, NULL)),PATH_MAX) > 0)) {
while ((j > 0) &&(strcoll(*((char **) ref_array_get(ra_list, i, NULL)),*((char **) ref_array_get(ra_list, j, NULL)))) > 0) { ref_array_swap(ra_list, i, j); i--; j--;
I should wrote in 3rd patch. This version cause warning in static analysers. Variable "i" can be underflowed. It is not problem, because "j" will be zero. On the other hand, difference between "i" and "j" is constant (1). We can remove one variable.
@@ -354,10 +356,12 @@ static int ini_aug_construct_list(char *dirname , int error = EOK; DIR *dir = NULL; struct dirent *entry = NULL;
struct dirent *entryp = NULL; char *snipname = NULL; char fullname[PATH_MAX + 1] = {0}; struct ref_array *ra_regex = NULL; bool match = false;
int len = 0;
TRACE_FLOW_ENTRY();
@@ -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);
^^^^^ //result allocation should be tested here. In case of failure, directory should be closed and ra_regex destroyed.
- /* 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);
^^^ s/rid/read/ ?
ref_array_destroy(ra_regex);free(entry);
// directory should be closed here as well. (resource leak)
return error;}/* Stop looping if we reached the end */if (entryp == NULL) break;TRACE_INFO_STRING("Processing", entry->d_name); /* Always skip current and parent dirs */@@ -413,6 +432,7 @@ static int ini_aug_construct_list(char *dirname , if (error) { TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM); ref_array_destroy(ra_regex);
free(entry); return ENOMEM; }@@ -422,6 +442,7 @@ static int ini_aug_construct_list(char *dirname , "the snippet list.", ENOMEM); ref_array_destroy(ra_regex);
free(entry); return ENOMEM; } }@@ -434,6 +455,7 @@ static int ini_aug_construct_list(char *dirname , } }
- free(entry); closedir(dir); ref_array_destroy(ra_regex);
-- 1.7.1
There is also lots of coverity warnings, becaue return value of function ref_array_get is not tested in this module, but it is tested in other modules.
Error: NULL_RETURNS (CWE-476): [#def1] ding-libs-0.4.0/ini/ini_augment.c:236: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:236: dereference: Dereferencing a null pointer "ref_array_get(ra_regex, i, NULL)".
Error: FORWARD_NULL (CWE-476): [#def2] ding-libs-0.4.0/ini/ini_augment.c:237: var_deref_model: Passing "NULL" to "regexec", which dereferences it.
Error: NULL_RETURNS (CWE-476): [#def3] ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null pointer "ref_array_get(ra_list, j - 1U, NULL)".
Error: NULL_RETURNS (CWE-476): [#def4] ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null pointer "ref_array_get(ra_list, j, NULL)".
Error: CHECKED_RETURN (CWE-252): [#def5] ding-libs-0.4.0/ini/ini_augment.c:66: check_return: Calling "ref_array_append" without checking return value (as is done elsewhere 11 out of 12 times). ding-libs-0.4.0/ini/ini_augment.c:194: example_assign: Example 1: Assigning: "error" = return value from "ref_array_append(ra, (void *)&preg)". ding-libs-0.4.0/ini/ini_augment.c:195: example_checked: Example 1 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_comment.c:653: example_assign: Example 2: Assigning: "error" = return value from "ref_array_append(ic->ra, (void *)&sb_new)". ding-libs-0.4.0/ini/ini_comment.c:654: example_checked: Example 2 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_comment.c:280: example_assign: Example 3: Assigning: "error" = return value from "ref_array_append(ic->ra, (void *)&elem)". ding-libs-0.4.0/ini/ini_comment.c:281: example_checked: Example 3 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_valueobj.c:148: example_assign: Example 4: Assigning: "error" = return value from "ref_array_append(raw_lines, (void *)©)". ding-libs-0.4.0/ini/ini_valueobj.c:149: example_checked: Example 4 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_valueobj.c:503: example_assign: Example 5: Assigning: "error" = return value from "ref_array_append(raw_lines, (void *)&strvalue)". ding-libs-0.4.0/ini/ini_valueobj.c:504: example_checked: Example 5 (cont.): "error" has its value checked in "error".
Attaching two small promised patches
LS
On 10/08/2014 04:31 AM, Lukas Slebodnik wrote:
On (29/09/14 07:45), Dmitri Pal wrote:
On 09/10/2014 11:31 PM, Dmitri Pal wrote:
Hello,
Extensive travel in recent months allowed me to finish this code.
Here is the updated design: https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
Patches: 0001 - Fix in case the Ref array is empty and we need to print/debug it 0002 - Declaration of the new function to do access checks 0003 - Big patch with core functionality 0004 - Updated access check code to use new internal access control function 0005 - File with expected output for unit test validation 0006 - Makefile and related changes to start building new code
I am attaching a new patch on top of the other patches. It can be squashed with patch 3 after review. It improves sorting and scanning. See patch comment.
No rush, take your time. :-)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
From aff0f323b2996df7a8f1539f46ca651d2d9fd5e8 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:38:19 +0200 Subject: [PATCH 1/6] [RA] Print info when array is empty
refarray/ref_array.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
ACK
From 9f80182fecbf1c1800a22ff279bf8915f4196f42 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:39:58 +0200 Subject: [PATCH 2/6] [INI] Declaring new internal access check function
ini/ini_config_priv.h | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h
ACK
From 8f0209cb995fd88e71e96efb0b98a0256df58d5d Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:43:31 +0200 Subject: [PATCH 4/6] [INI] Refactored access control check
The patch includes implementation of the new internal function.
ini/ini_fileobj.c | 66 +++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
ACK,
but it would be better to swap order of 3rd patch and this patch (4th) reason : * the 2nd patch add declaration to the private header file * the 3rd patch use this function * the 4th patch add definition of this function.
From 09a23b41da6f7a4584f652c4a5a95bb158b0dfe2 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 16:42:00 +0200 Subject: [PATCH 3/6] [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
//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)+{
//snip
/* Run through the list and save precompiled patterns */while (*pat) {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;}reg_err = regcomp(preg, *pat, REG_NOSUB);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;// memory allocated in err_str should be released // before 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;
+}
//snip
+/* Construct snippet lists based on the directory */ +static int ini_aug_construct_list(char *dirname ,
const char **patterns,struct access_check *check_perm,struct ref_array *ra_list,struct ref_array *ra_err)+{
- int error = EOK;
- DIR *dir = NULL;
- struct dirent *entry = NULL;
- char *snipname = NULL;
- char fullname[PATH_MAX + 1] = {0};
- struct ref_array *ra_regex = NULL;
- bool match = false;
- TRACE_FLOW_ENTRY();
- /* Prepare patterns */
- error = ini_aug_regex_prepare(patterns,
ra_err,&ra_regex);- if (error) {
TRACE_ERROR_NUMBER("Failed to prepare regex array.", error);return error;- }
- /* Open directory */
- errno = 0;
- dir = opendir(dirname);
- if (!dir) {
error = errno;if (error == ENOMEM) {TRACE_ERROR_NUMBER("No memory to open dir.", ENOMEM);ref_array_destroy(ra_regex);return ENOMEM;}/* Log an error, it is a recoverable error */add_dir_open_error(error, dirname, ra_err);//ra_regex should be destroyed here.
return EOK;- }
A directory is opened, but it is not closed in case of errors in next lines. It would be cause of potential resource leak.
- /* Loop through the directory */
- while ((entry = readdir(dir)) != NULL)
- {
TRACE_INFO_STRING("Processing", entry->d_name);/* Always skip current and parent dirs */if ((strncmp(entry->d_name,INI_CURRENT_DIR,sizeof(INI_CURRENT_DIR)) == 0) ||(strncmp(entry->d_name,INI_PARENT_DIR,sizeof(INI_PARENT_DIR)) == 0)) continue;/* Match names */match = ini_aug_match_name(entry->d_name, ra_regex);if (match) {snprintf(fullname, PATH_MAX, "%s/%s", dirname, entry->d_name);if(ini_check_file_perm(fullname, check_perm, ra_err)) {/* Dup name and add to the array */snipname = NULL;snipname = strdup(fullname);if (error) {^^^^^ snipname shouldbe tested here.
TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM);ref_array_destroy(ra_regex);//add closedir(dir) here
return ENOMEM;}error = ref_array_append(ra_list, (void *)&snipname);if (error) {TRACE_ERROR_NUMBER("No memory to add file to ""the snippet list.",ENOMEM);ref_array_destroy(ra_regex);//add closedir(dir) here
return ENOMEM;}}}else {ini_aug_add_string(ra_err,"File %s did not match provided patterns."" Skipping.",entry->d_name);}- }
- closedir(dir);
- ref_array_destroy(ra_regex);
- ini_aug_sort_list(ra_list);
- TRACE_FLOW_EXIT();
- return EOK;
+}
From d91b8bd98854a00448c021a42a551e5700a6ab82 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 22:53:19 +0200 Subject: [PATCH 5/6] [INI] Test file for unit test
ini/ini.d/merge.validator | 60 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 60 insertions(+), 0 deletions(-) create mode 100644 ini/ini.d/merge.validator
diff --git a/ini/ini.d/merge.validator b/ini/ini.d/merge.validator
ACK
From 9ce3ef526118ee8cf5e5dd8d28ddce971bba606e Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Sep 2014 22:55:02 +0200 Subject: [PATCH 6/6] [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 +++ b/Makefile.am @@ -255,6 +255,7 @@ libini_config_la_SOURCES = \ ini/ini_get_valueobj.c \ ini/ini_get_array_valueobj.c \ ini/ini_list_valueobj.c \
- ini/ini_augment.c \ trace/trace.h
libini_config_la_DEPENDENCIES = ini/libini_config.sym libini_config_la_LIBADD = \ @@ -286,19 +287,23 @@ dist_noinst_DATA += \ ini/ini.d/real32be.conf \ ini/ini.d/real32le.conf \ ini/ini.d/symbols.conf \
- ini/ini.d/new_line.conf
- ini/ini.d/new_line.conf \
- ini/ini.d/merge.validator
check_PROGRAMS += \ ini_config_ut \ ini_comment_ut \ ini_valueobj_ut \
- ini_parse_ut
- ini_parse_ut \
- ini_augment_ut
TESTS += \ ini_config_ut \ ini_comment_ut \ ini_valueobj_ut \
- ini_parse_ut
- ini_parse_ut \
- ini_augment_ut
ini_config_ut_SOURCES = ini/ini_config_ut.c ini_config_ut_LDADD = \ @@ -314,6 +319,9 @@ ini_valueobj_ut_LDADD = libini_config.la libbasicobjects.la ini_parse_ut_SOURCES = ini/ini_parse_ut.c ini_parse_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
+ini_augment_ut_SOURCES = ini/ini_augment_ut.c +ini_augment_ut_LDADD = libini_config.la libcollection.la libbasicobjects.la
The test ini_augment_ut calls functions from libpath_utils libref_array therefore it should be linked with this libraries. Otherwise it causes problems on distributions with disabled lik all deplibs.
This version works. // you can use different style of wrapping long lines. ini_augment_ut_LDADD = libini_config.la libcollection.la \ libpath_utils.la libref_array.la
ini_config-docs: if HAVE_DOXYGEN cd ini; \ @@ -324,6 +332,7 @@ clean-local-ini_config: rm -f ./*.out rm -f test.ini rm -f ./foo.conf ./bom* #From ini_parse_ut
- rm -f ./merge.validator.in #From ini_augment_ut
############################################################################## # Additional rules
//snip
diff --git a/ini/libini_config.sym b/ini/libini_config.sym index 3ca15bb..33d2246 100644 --- a/ini/libini_config.sym +++ b/ini/libini_config.sym @@ -62,6 +62,7 @@ global: ini_config_parse; ini_config_copy; ini_config_merge;
- ini_config_augment; ini_config_set_wrap; ini_config_serialize; ini_get_section_list;
It works butit is not right solution. The purpose of version symbol file is to help linker distiguish between two version of libraries with backward compatible changes. (just new functions were added). Without version symbol file, linker cannot detect differences due to the same soname.
sh$ objdump -p .libs/libini_config.so | grep SONAME SONAME libini_config.so.5
The right approach is to add new version as in refarray/libref_array.sym. I will attach file which can be squased to your patches.
From 78c072f8be307fd5a37944274cd509b22aefa5c2 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Mon, 29 Sep 2014 07:38:17 -0400 Subject: [PATCH 7/7] [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 @@ -28,9 +28,11 @@ #include <dirent.h> #include <stdio.h> #include <string.h> +#include <stddef.h> #include <limits.h> #include <sys/types.h> #include <regex.h> +#include <unistd.h> #include "trace.h" #include "collection.h" #include "collection_tools.h" @@ -320,9 +322,9 @@ static void ini_aug_sort_list(struct ref_array *ra_list) i = k; j = k + 1;
while ((j > 0) && (strncmp(*((char **) ref_array_get(ra_list, i, NULL)),*((char **) ref_array_get(ra_list, j, NULL)),PATH_MAX) > 0)) {
while ((j > 0) &&(strcoll(*((char **) ref_array_get(ra_list, i, NULL)),*((char **) ref_array_get(ra_list, j, NULL)))) > 0) { ref_array_swap(ra_list, i, j); i--; j--;I should wrote in 3rd patch. This version cause warning in static analysers. Variable "i" can be underflowed. It is not problem, because "j" will be zero. On the other hand, difference between "i" and "j" is constant (1). We can remove one variable.
@@ -354,10 +356,12 @@ static int ini_aug_construct_list(char *dirname , int error = EOK; DIR *dir = NULL; struct dirent *entry = NULL;
struct dirent *entryp = NULL; char *snipname = NULL; char fullname[PATH_MAX + 1] = {0}; struct ref_array *ra_regex = NULL; bool match = false;
int len = 0;
TRACE_FLOW_ENTRY();
@@ -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);
^^^^^//result allocation should be tested here. In case of failure, directory should be closed and ra_regex destroyed.
- /* 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);^^^ s/rid/read/ ?
ref_array_destroy(ra_regex);free(entry);// directory should be closed here as well. (resource leak)
return error;}/* Stop looping if we reached the end */if (entryp == NULL) break;TRACE_INFO_STRING("Processing", entry->d_name); /* Always skip current and parent dirs */@@ -413,6 +432,7 @@ static int ini_aug_construct_list(char *dirname , if (error) { TRACE_ERROR_NUMBER("Failed to dup string.", ENOMEM); ref_array_destroy(ra_regex);
free(entry); return ENOMEM; }@@ -422,6 +442,7 @@ static int ini_aug_construct_list(char *dirname , "the snippet list.", ENOMEM); ref_array_destroy(ra_regex);
free(entry); return ENOMEM; } }@@ -434,6 +455,7 @@ static int ini_aug_construct_list(char *dirname , } }
- free(entry); closedir(dir); ref_array_destroy(ra_regex);
-- 1.7.1
There is also lots of coverity warnings, becaue return value of function ref_array_get is not tested in this module, but it is tested in other modules.
Error: NULL_RETURNS (CWE-476): [#def1] ding-libs-0.4.0/ini/ini_augment.c:236: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:236: dereference: Dereferencing a null pointer "ref_array_get(ra_regex, i, NULL)".
Error: FORWARD_NULL (CWE-476): [#def2] ding-libs-0.4.0/ini/ini_augment.c:237: var_deref_model: Passing "NULL" to "regexec", which dereferences it.
Error: NULL_RETURNS (CWE-476): [#def3] ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null pointer "ref_array_get(ra_list, j - 1U, NULL)".
Error: NULL_RETURNS (CWE-476): [#def4] ding-libs-0.4.0/ini/ini_augment.c:325: returned_null: "ref_array_get" returns null. ding-libs-0.4.0/refarray/ref_array.c:226:9: return_null: Explicitly returning null. ding-libs-0.4.0/ini/ini_augment.c:325: dereference: Dereferencing a null pointer "ref_array_get(ra_list, j, NULL)".
Error: CHECKED_RETURN (CWE-252): [#def5] ding-libs-0.4.0/ini/ini_augment.c:66: check_return: Calling "ref_array_append" without checking return value (as is done elsewhere 11 out of 12 times). ding-libs-0.4.0/ini/ini_augment.c:194: example_assign: Example 1: Assigning: "error" = return value from "ref_array_append(ra, (void *)&preg)". ding-libs-0.4.0/ini/ini_augment.c:195: example_checked: Example 1 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_comment.c:653: example_assign: Example 2: Assigning: "error" = return value from "ref_array_append(ic->ra, (void *)&sb_new)". ding-libs-0.4.0/ini/ini_comment.c:654: example_checked: Example 2 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_comment.c:280: example_assign: Example 3: Assigning: "error" = return value from "ref_array_append(ic->ra, (void *)&elem)". ding-libs-0.4.0/ini/ini_comment.c:281: example_checked: Example 3 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_valueobj.c:148: example_assign: Example 4: Assigning: "error" = return value from "ref_array_append(raw_lines, (void *)©)". ding-libs-0.4.0/ini/ini_valueobj.c:149: example_checked: Example 4 (cont.): "error" has its value checked in "error". ding-libs-0.4.0/ini/ini_valueobj.c:503: example_assign: Example 5: Assigning: "error" = return value from "ref_array_append(raw_lines, (void *)&strvalue)". ding-libs-0.4.0/ini/ini_valueobj.c:504: example_checked: Example 5 (cont.): "error" has its value checked in "error".
Attaching two small promised patches
LS
Hi,
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):
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.
Thanks for review!
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@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@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@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
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@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@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@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@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
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@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@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@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@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.
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
On Tue, Dec 02, 2014 at 04:33:28PM +0100, Lukas Slebodnik wrote:
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
I must be missing some patches in between b/c I can't apply the last one:
[jhrozek@hendrix] ding-libs $ [(master)] git fetch origin [jhrozek@hendrix] ding-libs $ [(master)] git reset --hard origin/master HEAD is now at ab73013 SPEC: Do not include compiled files into package libdhash-devel [jhrozek@hendrix] ding-libs $ [(master)] lsp -rw-------. 1 jhrozek jhrozek 729 Dec 2 16:40 0001-RA-Print-info-when-array-is-empty.patch -rw-------. 1 jhrozek jhrozek 881 Dec 2 16:40 0002-INI-Declaring-new-internal-access-check-function.patch -rw-------. 1 jhrozek jhrozek 4166 Dec 2 16:40 0003-INI-Refactored-access-control-check.patch -rw-------. 1 jhrozek jhrozek 42030 Dec 2 16:40 0004-INI-New-function-to-merge-snippets.patch -rw-------. 1 jhrozek jhrozek 7683 Dec 2 16:40 0005-INI-Test-file-for-unit-test.patch -rw-------. 1 jhrozek jhrozek 7855 Dec 2 16:40 0006-INI-Make-the-merge-function-build.patch [jhrozek@hendrix] ding-libs $ [(master)] git am 0001-RA-Print-info-when-array-is-empty.patch 0002-INI-Declaring-new-internal-access-check-function.patch 0003-INI-Refactored-access-control-check.patch 0004-INI-New-function-to-merge-snippets.patch 0005-INI-Test-file-for-unit-test.patch Applying: Print info when array is empty Applying: Declaring new internal access check function Applying: Refactored access control check Applying: New function to merge snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:986: new blank line at EOF. + warning: 1 line adds whitespace errors. Applying: Test file for unit test [jhrozek@hendrix] ding-libs $ [(master)] git am 0006-INI-Make-the-merge-function-build.patch Applying: Make the merge function build /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:125: trailing whitespace. * Function merges the main configuration file /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:126: trailing whitespace. * with the configuration file snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:144: trailing whitespace. * holds criteria for the error: patch failed: Makefile.am:255 error: Makefile.am: patch does not apply /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:214: new blank line at EOF. + Patch failed at 0001 Make the merge function build The copy of the patch that failed is found in: /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
On 12/02/2014 02:00 PM, Jakub Hrozek wrote:
On Tue, Dec 02, 2014 at 04:33:28PM +0100, Lukas Slebodnik wrote:
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
I must be missing some patches in between b/c I can't apply the last one:
[jhrozek@hendrix] ding-libs $ [(master)] git fetch origin [jhrozek@hendrix] ding-libs $ [(master)] git reset --hard origin/master HEAD is now at ab73013 SPEC: Do not include compiled files into package libdhash-devel [jhrozek@hendrix] ding-libs $ [(master)] lsp -rw-------. 1 jhrozek jhrozek 729 Dec 2 16:40 0001-RA-Print-info-when-array-is-empty.patch -rw-------. 1 jhrozek jhrozek 881 Dec 2 16:40 0002-INI-Declaring-new-internal-access-check-function.patch -rw-------. 1 jhrozek jhrozek 4166 Dec 2 16:40 0003-INI-Refactored-access-control-check.patch -rw-------. 1 jhrozek jhrozek 42030 Dec 2 16:40 0004-INI-New-function-to-merge-snippets.patch -rw-------. 1 jhrozek jhrozek 7683 Dec 2 16:40 0005-INI-Test-file-for-unit-test.patch -rw-------. 1 jhrozek jhrozek 7855 Dec 2 16:40 0006-INI-Make-the-merge-function-build.patch [jhrozek@hendrix] ding-libs $ [(master)] git am 0001-RA-Print-info-when-array-is-empty.patch 0002-INI-Declaring-new-internal-access-check-function.patch 0003-INI-Refactored-access-control-check.patch 0004-INI-New-function-to-merge-snippets.patch 0005-INI-Test-file-for-unit-test.patch Applying: Print info when array is empty Applying: Declaring new internal access check function Applying: Refactored access control check Applying: New function to merge snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:986: new blank line at EOF.
warning: 1 line adds whitespace errors. Applying: Test file for unit test [jhrozek@hendrix] ding-libs $ [(master)] git am 0006-INI-Make-the-merge-function-build.patch Applying: Make the merge function build /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:125: trailing whitespace.
- Function merges the main configuration file
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:126: trailing whitespace.
- with the configuration file snippets
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:144: trailing whitespace.
holds criteria for theerror: patch failed: Makefile.am:255 error: Makefile.am: patch does not apply /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:214: new blank line at EOF.
Patch failed at 0001 Make the merge function build The copy of the patch that failed is found in: /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I am not sure what I can do here. Please advise. They are all from master.
On (02/12/14 20:00), Jakub Hrozek wrote:
On Tue, Dec 02, 2014 at 04:33:28PM +0100, Lukas Slebodnik wrote:
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
I must be missing some patches in between b/c I can't apply the last one:
[jhrozek@hendrix] ding-libs $ [(master)] git fetch origin [jhrozek@hendrix] ding-libs $ [(master)] git reset --hard origin/master HEAD is now at ab73013 SPEC: Do not include compiled files into package libdhash-devel [jhrozek@hendrix] ding-libs $ [(master)] lsp -rw-------. 1 jhrozek jhrozek 729 Dec 2 16:40 0001-RA-Print-info-when-array-is-empty.patch -rw-------. 1 jhrozek jhrozek 881 Dec 2 16:40 0002-INI-Declaring-new-internal-access-check-function.patch -rw-------. 1 jhrozek jhrozek 4166 Dec 2 16:40 0003-INI-Refactored-access-control-check.patch -rw-------. 1 jhrozek jhrozek 42030 Dec 2 16:40 0004-INI-New-function-to-merge-snippets.patch -rw-------. 1 jhrozek jhrozek 7683 Dec 2 16:40 0005-INI-Test-file-for-unit-test.patch -rw-------. 1 jhrozek jhrozek 7855 Dec 2 16:40 0006-INI-Make-the-merge-function-build.patch [jhrozek@hendrix] ding-libs $ [(master)] git am 0001-RA-Print-info-when-array-is-empty.patch 0002-INI-Declaring-new-internal-access-check-function.patch 0003-INI-Refactored-access-control-check.patch 0004-INI-New-function-to-merge-snippets.patch 0005-INI-Test-file-for-unit-test.patch Applying: Print info when array is empty Applying: Declaring new internal access check function Applying: Refactored access control check Applying: New function to merge snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:986: new blank line at EOF.
warning: 1 line adds whitespace errors. Applying: Test file for unit test [jhrozek@hendrix] ding-libs $ [(master)] git am 0006-INI-Make-the-merge-function-build.patch Applying: Make the merge function build /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:125: trailing whitespace.
- Function merges the main configuration file
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:126: trailing whitespace.
- with the configuration file snippets
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:144: trailing whitespace.
holds criteria for theerror: patch failed: Makefile.am:255 error: Makefile.am: patch does not apply /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:214: new blank line at EOF.
Patch failed at 0001 Make the merge function build The copy of the patch that failed is found in: /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
I had the same problem with "git am" but i thought the problem is in the latest version of git. But applying patch with simple old way "patch -p1" works well.
LS
On Wed, Dec 03, 2014 at 10:23:15AM +0100, Lukas Slebodnik wrote:
On (02/12/14 20:00), Jakub Hrozek wrote:
On Tue, Dec 02, 2014 at 04:33:28PM +0100, Lukas Slebodnik wrote:
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
I must be missing some patches in between b/c I can't apply the last one:
[jhrozek@hendrix] ding-libs $ [(master)] git fetch origin [jhrozek@hendrix] ding-libs $ [(master)] git reset --hard origin/master HEAD is now at ab73013 SPEC: Do not include compiled files into package libdhash-devel [jhrozek@hendrix] ding-libs $ [(master)] lsp -rw-------. 1 jhrozek jhrozek 729 Dec 2 16:40 0001-RA-Print-info-when-array-is-empty.patch -rw-------. 1 jhrozek jhrozek 881 Dec 2 16:40 0002-INI-Declaring-new-internal-access-check-function.patch -rw-------. 1 jhrozek jhrozek 4166 Dec 2 16:40 0003-INI-Refactored-access-control-check.patch -rw-------. 1 jhrozek jhrozek 42030 Dec 2 16:40 0004-INI-New-function-to-merge-snippets.patch -rw-------. 1 jhrozek jhrozek 7683 Dec 2 16:40 0005-INI-Test-file-for-unit-test.patch -rw-------. 1 jhrozek jhrozek 7855 Dec 2 16:40 0006-INI-Make-the-merge-function-build.patch [jhrozek@hendrix] ding-libs $ [(master)] git am 0001-RA-Print-info-when-array-is-empty.patch 0002-INI-Declaring-new-internal-access-check-function.patch 0003-INI-Refactored-access-control-check.patch 0004-INI-New-function-to-merge-snippets.patch 0005-INI-Test-file-for-unit-test.patch Applying: Print info when array is empty Applying: Declaring new internal access check function Applying: Refactored access control check Applying: New function to merge snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:986: new blank line at EOF.
warning: 1 line adds whitespace errors. Applying: Test file for unit test [jhrozek@hendrix] ding-libs $ [(master)] git am 0006-INI-Make-the-merge-function-build.patch Applying: Make the merge function build /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:125: trailing whitespace.
- Function merges the main configuration file
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:126: trailing whitespace.
- with the configuration file snippets
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:144: trailing whitespace.
holds criteria for theerror: patch failed: Makefile.am:255 error: Makefile.am: patch does not apply /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:214: new blank line at EOF.
Patch failed at 0001 Make the merge function build The copy of the patch that failed is found in: /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
I had the same problem with "git am" but i thought the problem is in the latest version of git. But applying patch with simple old way "patch -p1" works well.
OK, I also fixed the author mail (dpal@csb -> dpal@redhat.com) and added the reviewed-by tags before pushing: 9044d853a4a6c7cf0387d07b1a4c333adc07de17 b9104ae94e9f839d263acaef5296b8676734e855 d9df58eb9be0264ee4a9e864391df2d60f33f1b5 50abe3828ab2139c4e79d96d1ccf2b76c820cedb 0c7250402e027cd325af3e06b15b365bffb19688 04d88a875e3ac5f1c707d09cc9d5cb7ebf827913
On 12/03/2014 08:19 AM, Jakub Hrozek wrote:
On Wed, Dec 03, 2014 at 10:23:15AM +0100, Lukas Slebodnik wrote:
On (02/12/14 20:00), Jakub Hrozek wrote:
On Tue, Dec 02, 2014 at 04:33:28PM +0100, Lukas Slebodnik wrote:
On (29/11/14 01:03), Dmitri Pal wrote:
On (19/11/14 18:02), Lukas Slebodnik wrote: Firstly, I would like to appologize for late review.
Thanks for review! Not a problem at all. It is not a high priority.
//snip
New set is attached.
all issues were fixed. There aren't any warnings from static analysers of c compiler.
ACK
LS
I must be missing some patches in between b/c I can't apply the last one:
[jhrozek@hendrix] ding-libs $ [(master)] git fetch origin [jhrozek@hendrix] ding-libs $ [(master)] git reset --hard origin/master HEAD is now at ab73013 SPEC: Do not include compiled files into package libdhash-devel [jhrozek@hendrix] ding-libs $ [(master)] lsp -rw-------. 1 jhrozek jhrozek 729 Dec 2 16:40 0001-RA-Print-info-when-array-is-empty.patch -rw-------. 1 jhrozek jhrozek 881 Dec 2 16:40 0002-INI-Declaring-new-internal-access-check-function.patch -rw-------. 1 jhrozek jhrozek 4166 Dec 2 16:40 0003-INI-Refactored-access-control-check.patch -rw-------. 1 jhrozek jhrozek 42030 Dec 2 16:40 0004-INI-New-function-to-merge-snippets.patch -rw-------. 1 jhrozek jhrozek 7683 Dec 2 16:40 0005-INI-Test-file-for-unit-test.patch -rw-------. 1 jhrozek jhrozek 7855 Dec 2 16:40 0006-INI-Make-the-merge-function-build.patch [jhrozek@hendrix] ding-libs $ [(master)] git am 0001-RA-Print-info-when-array-is-empty.patch 0002-INI-Declaring-new-internal-access-check-function.patch 0003-INI-Refactored-access-control-check.patch 0004-INI-New-function-to-merge-snippets.patch 0005-INI-Test-file-for-unit-test.patch Applying: Print info when array is empty Applying: Declaring new internal access check function Applying: Refactored access control check Applying: New function to merge snippets /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:986: new blank line at EOF.
warning: 1 line adds whitespace errors. Applying: Test file for unit test [jhrozek@hendrix] ding-libs $ [(master)] git am 0006-INI-Make-the-merge-function-build.patch Applying: Make the merge function build /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:125: trailing whitespace.
- Function merges the main configuration file
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:126: trailing whitespace.
- with the configuration file snippets
/home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:144: trailing whitespace.
holds criteria for theerror: patch failed: Makefile.am:255 error: Makefile.am: patch does not apply /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch:214: new blank line at EOF.
Patch failed at 0001 Make the merge function build The copy of the patch that failed is found in: /home/remote/jhrozek/devel/ding-libs/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
I had the same problem with "git am" but i thought the problem is in the latest version of git. But applying patch with simple old way "patch -p1" works well.
OK, I also fixed the author mail (dpal@csb -> dpal@redhat.com) and added the reviewed-by tags before pushing: 9044d853a4a6c7cf0387d07b1a4c333adc07de17 b9104ae94e9f839d263acaef5296b8676734e855 d9df58eb9be0264ee4a9e864391df2d60f33f1b5 50abe3828ab2139c4e79d96d1ccf2b76c820cedb 0c7250402e027cd325af3e06b15b365bffb19688 04d88a875e3ac5f1c707d09cc9d5cb7ebf827913 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks!
On (29/09/14 07:45), Dmitri Pal wrote:
On 09/10/2014 11:31 PM, Dmitri Pal wrote:
Hello,
Extensive travel in recent months allowed me to finish this code.
Here is the updated design: https://fedorahosted.org/sssd/wiki/DesignDocs/ding-libs/INIConfigMerge
Patches: 0001 - Fix in case the Ref array is empty and we need to print/debug it 0002 - Declaration of the new function to do access checks 0003 - Big patch with core functionality 0004 - Updated access check code to use new internal access control function 0005 - File with expected output for unit test validation 0006 - Makefile and related changes to start building new code
I am attaching a new patch on top of the other patches. It can be squashed with patch 3 after review. It improves sorting and scanning. See patch comment.
No rush, take your time. :-)
I forgot to write most important info in my previous mail. The augment unit test failed on my machine.
sh-4.3$ cat ./ini_augment_ut.log rm: cannot remove ‘../ini/ini.d/*~’: No such file or directory Files ./merge.validator.in and ./merge.validator.out differ Failed to run diff command 256 1. Failed with error -1! sh-4.3$ sh-4.3$ diff ./merge.validator.in ./merge.validator.out 3,4d2 < File new_line.conf did not match provided patterns. Skipping. < File real32be.conf did not match provided patterns. Skipping. 6c4 < File real16be.conf did not match provided patterns. Skipping. ---
File real32be.conf did not match provided patterns. Skipping.
7a6,7
File real16be.conf did not match provided patterns. Skipping. File new_line.conf did not match provided patterns. Skipping.
Order of lines is different.
LS
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Lukas Slebodnik