Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
On 01/02/2015 02:47 PM, Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Any takers?
On Thu, Jan 15, 2015 at 10:54:39AM -0500, Dmitri Pal wrote:
On 01/02/2015 02:47 PM, Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Any takers?
Lukas said he'd take a look on our call on Thursday..
On (19/01/15 23:16), Jakub Hrozek wrote:
On Thu, Jan 15, 2015 at 10:54:39AM -0500, Dmitri Pal wrote:
On 01/02/2015 02:47 PM, Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
Any takers?
Lukas said he'd take a look on our call on Thursday..
I need to correct you. I said: "It is very likely I'll review patches because nobody else will do"
LS
On (02/01/15 14:47), Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
I would like to appolozige for late review. I started to review them few times but never finished and lost notes.
Patches look good. I have just few small comments.
From 2d08f30671042e4b499df7cd5e3055a2e9b72af6 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 9 Nov 2014 19:44:38 -0500 Subject: [PATCH 01/16] [COLLECTION] Function to return one of the dups
The internal search functoins we not able to deal with the duplicates that might be scattered across a collection. This change creates a new search function that is more robust. The old function becomes a wrapper. The code is adjusted in the cases where the functionality of the more advanced functions is needed.
diff --git a/collection/libcollection.sym b/collection/libcollection.sym index 9110cae513482564cf43e1b7892a5a208d32a2d0..fab025b0a8dbbe6dfa7a9e2ff2b6935640ad52e6 100644 --- a/collection/libcollection.sym +++ b/collection/libcollection.sym @@ -143,3 +143,9 @@ global: local: *; };
+COLLECTION_0.7 { +global:
- /* collection.h */
- col_get_dup_item;
+};
^ It would be good to add old version here. The same as it is in http://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html#SEC25 or info ld -> 3.9 VERSION Command or here http://people.redhat.com/drepper/dsohowto.pdf
I was not able to find any difference in binary but it's better to follow conventions.
BTW you can also update version of libcollection in version.m4 (in the same patch)
From 9ced5af17034a50567d7032f9972254b55899b39 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Dec 2014 16:03:33 -0500 Subject: [PATCH 02/16] [COLLECTION] Allow to modify item name
It was not possible to rename an item that is a collection or collection reference. With this patch it becomes possible.
ACK
From 28edbb158976447809ce42db53a574fc4d2e833f Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Dec 2014 17:07:08 -0500 Subject: [PATCH 03/16] [COLLECTION] Expose delete with callback function
Expose internal function to be able to delete collection elements with callback.
ACK
From e9b3934157af66c6c7ba0bb658ce2da6298de1f6 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 9 Nov 2014 16:38:14 -0500 Subject: [PATCH 04/16] [INI] Comment creation helper
Patch adds a function that makes creating a comment from a list of strings easier.
ini/ini_comment.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ ini/ini_comment.h | 13 ++++++++++ ini/ini_comment_ut.c | 38 +++++++++++++++++++++++++++++++ ini/libini_config.sym | 2 + 4 files changed, 112 insertions(+), 0 deletions(-)
diff --git a/ini/libini_config.sym b/ini/libini_config.sym index d6a0afd307343748049b64da3f33291144e7a613..4dbafa02d2dc249181869ab921f8d930f91d2d3c 100644 --- a/ini/libini_config.sym +++ b/ini/libini_config.sym @@ -141,5 +141,7 @@ INI_CONFIG_1.2.0 { global: /* ini_configobj.h */ ini_config_augment;
- /* ini_comment.h */
- ini_comment_construct;
} INI_CONFIG_1.1.0;
BTW you can use INI_CONFIG_2.0 if you think it worths. You added many functions in this patch-set :-)
From 7eb753cd891d260d5516a6b2f4c64394b7d8b7b5 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Fri, 28 Nov 2014 21:25:15 -0500 Subject: [PATCH 05/16] [INI] Comment can be NULL
It is valid to set comment to NULL. The function is adjusted to work when caller wants to remove the comment by passing NULL.
ACK
From d4d2b5ce6a3ed50530eee06b09acc3881ca5dba7 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sat, 6 Dec 2014 14:17:05 -0500 Subject: [PATCH 06/16] [INI] Move definition to common header
The constants will be reused so they belong to common header.
ACK
From cf86848991dbc68c1b5ceed7bd7a7d431b374c20 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sat, 6 Dec 2014 14:18:28 -0500 Subject: [PATCH 07/16] [INI] Fix wrapping error
The values starting with space were automatically wrapped by that first space. This is wrong. It should not wrap on the first space automatically.
ACK
From c590b522cf1927fb1c7a9b45e5a0e648f620d242 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Dec 2014 17:10:07 -0500 Subject: [PATCH 08/16] [INI] New interface to modify configuration
This patch adds a new interface to modify configuration.
ini/ini_configmod.c | 1533 ++++++++++++++++++++++++++++++++++++++++++++++++ ini/ini_configmod.h | 744 +++++++++++++++++++++++ ini/ini_configmod_ut.c | 1152 ++++++++++++++++++++++++++++++++++++ 3 files changed, 3429 insertions(+), 0 deletions(-) create mode 100644 ini/ini_configmod.c create mode 100644 ini/ini_configmod.h create mode 100644 ini/ini_configmod_ut.c
diff --git a/ini/ini_configmod.c b/ini/ini_configmod.c new file mode 100644 index 0000000000000000000000000000000000000000..11c41358264d93842886f597ea5cd2958627f148 --- /dev/null +++ b/ini/ini_configmod.c @@ -0,0 +1,1533 @@ +/*
- INI LIBRARY
- Implementation of the modification interface.
- 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/.
+*/
+/* Add or modify a value that stores array of integers */ +int ini_config_add_int_arr_value(struct ini_cfgobj *ini_config,
const char *section,const char *key,int *value_int_arr,size_t count_int,char sep,const char *comments[],size_t count_comment,int border,int position,const char *other_key,int idx,enum INI_VA flags)+{
- int error = EOK;
- int ret = 0;
- char **str_arr = NULL;
- size_t i = 0;
- size_t j = 0;
- TRACE_FLOW_ENTRY();
- if (!count_int) {
TRACE_ERROR_STRING("Invalid argument","count_int");return EINVAL;- }
- str_arr = malloc(count_int * sizeof(char *));
Please use calloc here otherwise unitialized variable would be used in case of releasing in error handling. reported by coverity
--- a/ini/ini_configmod.c +++ b/ini/ini_configmod.c @@ -214,7 +214,7 @@ int ini_config_add_int_arr_value(struct ini_cfgobj *ini_config, return EINVAL; }
- str_arr = malloc(count_int * sizeof(char *)); + str_arr = calloc(count_int, sizeof(char *)); if (!str_arr) { TRACE_ERROR_STRING("Failed to allocate memory for string array", ENOMEM); @@ -281,7 +281,7 @@ int ini_config_add_long_arr_value(struct ini_cfgobj *ini_config, return EINVAL; }
- str_arr = malloc(count_long * sizeof(char *)); + str_arr = calloc(count_long, sizeof(char *)); if (!str_arr) { TRACE_ERROR_STRING("Failed to allocate memory for string array", ENOMEM); @@ -348,7 +348,7 @@ int ini_config_add_double_arr_value(struct ini_cfgobj *ini_config, return EINVAL; }
- str_arr = malloc(count_double * sizeof(char *)); + str_arr = calloc(count_double, sizeof(char *)); if (!str_arr) { TRACE_ERROR_STRING("Failed to allocate memory for string array", ENOMEM);
- if (!str_arr) {
TRACE_ERROR_STRING("Failed to allocate memory for string array",ENOMEM);return ENOMEM;- }
- for (i = 0; i < count_int; i++) {
ret = asprintf(&str_arr[i], "%d", value_int_arr[i]);if (ret == -1) {TRACE_ERROR_NUMBER("Asprintf failed.", ret);/* The main reason is propbaly memory allocation */for (j = 0; j < i; j++) free(str_arr[j]);free(str_arr);return ENOMEM;}- }
- error = ini_config_add_str_arr_value(ini_config,
section,key,(const char **)str_arr,
There are warnings due to this casting.
ini/ini_configmod.c: In function 'ini_config_add_int_arr_value': ini/ini_configmod.c:238:42: warning: to be safe all intermediate pointers in cast from 'char **' to 'const char **' must be 'const' qualified [-Wcast-qual] (const char **)str_arr, ^ ini/ini_configmod.c: In function 'ini_config_add_long_arr_value': ini/ini_configmod.c:305:42: warning: to be safe all intermediate pointers in cast from 'char **' to 'const char **' must be 'const' qualified [-Wcast-qual] (const char **)str_arr, ^ ini/ini_configmod.c: In function 'ini_config_add_double_arr_value': ini/ini_configmod.c:372:42: warning: to be safe all intermediate pointers in cast from 'char **' to 'const char **' must be 'const' qualified [-Wcast-qual] (const char **)str_arr, ^ CCLD libini_config.la
New compilers (gcc 4.9 and clang) complains if the "const is removes" in casting
void** -> int** FINE const void** -> int ** BAD void ** -> const int ** BAD
We have a macro to suppres such warning in sssd #ifndef discard_const_p #if defined(__intptr_t_defined) || defined(HAVE_INTPTR_T) # define discard_const_p(type, ptr) ((type *)((intptr_t)(ptr))) #else # define discard_const_p(type, ptr) ((type *)(ptr)) #endif #endif
But the warning is on 3 places. So other people can have the same problem. So we can have two functions. ini_config_add_str_arr_value ini_config_add_const_str_arr_value
One would be wrapper around the second one and can use trick with discard_const.
count_int,sep,comments,count_comment,border,position,other_key,idx,flags);- for (i = 0; i < count_int; i++) free(str_arr[i]);
- free(str_arr);
- TRACE_FLOW_RETURN(error);
- return error;
+}
+/* Function to add int32 value */ +int ini_config_add_int32_value(struct ini_cfgobj *ini_config,
const char *section,const char *key,int32_t value,const char **comments,size_t count_comment,int border,int position,const char *other_key,int idx,enum INI_VA flags)+{
- int error = EOK;
- int ret = 0;
- char *strval = NULL;
- TRACE_FLOW_ENTRY();
- ret = asprintf(&strval, "%d", value);
^^ It would be better to use predefined constant for uint*_t types. They are defined in header file inttypes.h @see inttypes.h
For int32_t use "%"PRId32 uint32_t -> "%"PRIu32 uint64_t -> "%"PRIu64 ...
It is more portable an you can avoid casting to (long long ...)
- if (ret == -1) {
TRACE_ERROR_NUMBER("Asprintf failed.", ret);/* The main reason is propbaly memory allocation */return ENOMEM;- }
- /* Call string function */
- error = ini_config_add_str_value(ini_config,
section,key,strval,comments,count_comment,border,position,other_key,idx,flags);- TRACE_FLOW_RETURN(error);
- free(strval);
- return error;
+}
diff --git a/ini/ini_configmod.h b/ini/ini_configmod.h new file mode 100644 index 0000000000000000000000000000000000000000..e25a2756fab9121e64829aa844e28062cc78df23 --- /dev/null +++ b/ini/ini_configmod.h @@ -0,0 +1,744 @@ +/*
- INI LIBRARY
- Interface that allows modification of the INI file.
- 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/.
+*/
+#ifndef INI_CONFIGMOD_H +#define INI_CONFIGMOD_H
There are doxygen warnings ini/ini_configmod.h:40: warning: Member INI_VA (enumeration) of group insflags is not documented. ini/ini_configmod.h:309: warning: Found unknown command `\bNOTE'
From 71971ea237bdb30dfaa1b1bcf0cced155e711e8a Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sat, 6 Dec 2014 14:21:42 -0500 Subject: [PATCH 09/16] [INI] Build new interface
Add new interface to makefile so that it can be built.
Makefile.am | 19 ++++++++++++++++--- ini/libini_config.sym | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 100bc1368a5325e553beaf1dcdc80569a84f0c91..7d67c5f90a1b427bec5639bd7549be08e020ac7a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -230,7 +230,9 @@ endif
lib_LTLIBRARIES += libini_config.la dist_pkgconfig_DATA += ini/ini_config.pc -dist_include_HEADERS += ini/ini_config.h ini/ini_configobj.h ini/ini_valueobj.h ini/ini_comment.h +dist_include_HEADERS += ini/ini_config.h ini/ini_configobj.h \
ini/ini_valueobj.h ini/ini_comment.h \ini/ini_configmod.h
Plaease add ini_configmod.h to spec file as well. otherwise "make rpms" fails
diff --git a/contrib/ding-libs.spec.in b/contrib/ding-libs.spec.in index 87f2a1a..54d8b97 100644 --- a/contrib/ding-libs.spec.in +++ b/contrib/ding-libs.spec.in @@ -311,6 +311,7 @@ structure %{_includedir}/ini_configobj.h %{_includedir}/ini_valueobj.h %{_includedir}/ini_comment.h +%{_includedir}/ini_configmod.h %{_libdir}/libini_config.so %{_libdir}/pkgconfig/ini_config.pc %doc ini/doc/html/
diff --git a/ini/libini_config.sym b/ini/libini_config.sym index 4dbafa02d2dc249181869ab921f8d930f91d2d3c..e97881bd62a60ea9bd57d8a34fe9477002fd103c 100644 --- a/ini/libini_config.sym +++ b/ini/libini_config.sym @@ -143,5 +143,28 @@ global: ini_config_augment; /* ini_comment.h */ ini_comment_construct;
- /* ini_configmod.h */
- ini_config_add_section;
- ini_config_comment_section;
- ini_config_rename_section;
- ini_config_delete_section_by_name;
- ini_config_delete_section_by_position;
- ini_config_add_str_value;
- ini_config_add_int_value;
- ini_config_add_long_value;
- ini_config_add_ulong_value;
- ini_config_add_unsigned_value;
- ini_config_add_int32_value;
- ini_config_add_uint32_value;
- ini_config_add_int64_value;
- ini_config_add_uint64_value;
- ini_config_add_double_value;
- ini_config_add_bin_value;
- ini_config_add_str_arr_value;
- ini_config_add_int_arr_value;
- ini_config_add_long_arr_value;
- ini_config_add_double_arr_value;
- ini_config_delete_value;
- ini_config_update_comment;
} INI_CONFIG_1.1.0;
This change could be part of previous patch becuase you added functions there. But you can let them in this patch as well. It's not a big issue.
From 3eb2c7bcb2706553411aa12f662c97cbb88875f6 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sat, 6 Dec 2014 14:20:11 -0500 Subject: [PATCH 10/16] [INI] Generate doxy doc for INI modification API
Add configuration so that doc generater pick another header.
ACK
From 42875077a51dc9a0c5f76bea73dbc22676ca9334 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@dpal.csb Date: Sun, 7 Dec 2014 16:05:31 -0500 Subject: [PATCH 11/16] [INI] Cleaning doxygen comments
Patch makes changes to better comment the access structure so it is readable in doxygen doc.
LGTM
From 781310dd5aed5d162a10eb4118c2a4717ec3ec9b Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Sun, 14 Dec 2014 21:19:29 -0500 Subject: [PATCH 12/16] [INI] Change order of the headers
The headers have been changed and thus require a specific order for forward declarations. Commit order changes first.
ACK
From a0361a4876f36d9303c4207397624a335fcb3816 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Sun, 14 Dec 2014 21:21:14 -0500 Subject: [PATCH 13/16] [INI] New interface to save configuration in a file
The patch adds declaration of the new interface that allows saving configuration in a file, creating backups and changing file permissions and ownership.
ini/ini_config_priv.h | 3 + ini/ini_configobj.h | 184 +++++++++++++++++++++++++++++++++++++++++++++++++ ini/libini_config.sym | 6 ++ 3 files changed, 193 insertions(+), 0 deletions(-)
diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h index 331363f340b84db734db760c54449dd7058eefd3..429bf40584bf135a6a99b6963f66699bb7066311 100644 --- a/ini/ini_config_priv.h +++ b/ini/ini_config_priv.h @@ -53,6 +53,7 @@ struct ini_cfgobj { /*... */ };
+enum index_utf_t;
If you remove previous line then warnings "-Wgnu-redeclared-enum" will be fixed. In file included from ini/ini_print.c:33: ./ini/ini_config_priv.h:56:6: warning: redeclaration of already-defined enum 'index_utf_t' is a GNU extension [-Wgnu-redeclared-enum] enum index_utf_t; ^ ./ini/ini_configobj.h:141:6: note: previous definition is here enum index_utf_t { ^
I think that's the reason why you changed order of including header files in previous patch.
/* Configuration file object */ struct ini_cfgfile { @@ -74,6 +75,8 @@ struct ini_cfgfile { int stats_read; /* Internal buffer */ struct simplebuffer *file_data;
- /* BOM indicator */
- enum index_utf_t bom;
};
/* Parsing error */ diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h index 8cfe74d921d265f2be544961167b7e4b921df096..363d9f93021c16f58c6a6e1aefb200c85d0da43b 100644 --- a/ini/ini_configobj.h +++ b/ini/ini_configobj.h @@ -130,6 +130,29 @@
/**
- @defgroup bomType Types of configutration file encodings
- Constants that define how configuration file is encoded.
- @{
- */
+/** Enumeration of the encoding types. */
+enum index_utf_t {
- INDEX_UTF32BE = 0, /**< The file is encoded in 'big-endian' 32-bit */
- INDEX_UTF32LE = 1, /**< The file is encoded in 'little-endian' 32-bit */
- INDEX_UTF16BE = 2, /**< The file is encoded in 'big-endian' 16-bit */
- INDEX_UTF16LE = 3, /**< The file is encoded in 'little-endian' 16-bit */
- INDEX_UTF8 = 4, /**< The file is encoded in standard UTF8 but has BOM */
- INDEX_UTF8NOBOM = 5 /**< The file is encoded in standard UTF8 without BOM */
+};
You "moved" enum index_utf_t to header file in this patch but it is removed in next patch. It cause compilation error.
ini/ini_fileobj.c:41:6: error: redeclaration of 'enum index_utf_t' In file included from ini/ini_fileobj.c:31:0: ini/ini_configobj.h:141:6: note: originally defined here enum index_utf_t { ^ ini/ini_fileobj.c:42:5: error: redeclaration of enumerator 'INDEX_UTF32BE' INDEX_UTF32BE = 0,
I know it is fixed in next patch but it is very useful to have each patch compilable for git bisect.
diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c index 252c23e..5d46562 100644 --- a/ini/ini_fileobj.c +++ b/ini/ini_fileobj.c @@ -38,14 +38,6 @@ #define BOM3_SIZE 3 #define BOM2_SIZE 2
-enum index_utf_t { - INDEX_UTF32BE = 0, - INDEX_UTF32LE = 1, - INDEX_UTF16BE = 2, - INDEX_UTF16LE = 3, - INDEX_UTF8 = 4 -}; -
+/**
- @}
- */
+/**
- @defgroup errorlevel Error tolerance constants
- Constants in this section define what to do if
@@ -389,6 +412,7 @@ struct access_check { * One can check file * permissions with mask, * uid, and gid of the file.
* See \ref accesscheck constants. */@@ -583,6 +607,166 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in, void ini_config_file_destroy(struct ini_cfgfile *file_ctx);
From f618a8f24a584db52ea767eea130b6370da6d7df Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Sun, 14 Dec 2014 21:22:49 -0500 Subject: [PATCH 14/16] [INI] Implementation of the interface to save configuration
The patch adds implmentation of the new interface that allows saving configuration in a file, creating backups and changing file permissions and ownership.
ini/ini_fileobj.c | 814 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 798 insertions(+), 16 deletions(-)
diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c index 252c23e92be4e31840388ed15bbd334583b9d689..7b22ecc5d6b0d68590b0443dcec91b8f974e03e1 100644 --- a/ini/ini_fileobj.c +++ b/ini/ini_fileobj.c @@ -18,14 +18,17 @@ 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 /* for asprintf */ #include "config.h" #include <errno.h> #include <sys/types.h> #include <sys/stat.h> +#include <unistd.h> #include <fcntl.h> #include <string.h> #include <stdlib.h> #include <iconv.h> +#include <dirent.h> #include "trace.h" #include "ini_defines.h" #include "ini_configobj.h" @@ -38,13 +41,13 @@ #define BOM3_SIZE 3 #define BOM2_SIZE 2
-enum index_utf_t {
- INDEX_UTF32BE = 0,
- INDEX_UTF32LE = 1,
- INDEX_UTF16BE = 2,
- INDEX_UTF16LE = 3,
- INDEX_UTF8 = 4
-};
+const char *encodings[] = { "UTF-32BE",
"UTF-32LE","UTF-16BE","UTF-16LE","UTF-8","UTF-8" };
Please define this variable with static modifier. So it will be crystal clear there isn't any exter declaration in another hedaer file.
/* Close file but not destroy the object */ void ini_config_file_close(struct ini_cfgfile *file_ctx) @@ -193,15 +196,11 @@ static int initialize_conv(unsigned char *read_buf,
+/* Function to check the template
- Template is allowed to have '%%' as many times and caller wants
- but only one %d. No other combination with a percent is allowed.
- */
+static int check_template(const char *tpl) +{
- char *ptr = NULL;
- char *ptr_pcnt = NULL;
- TRACE_FLOW_ENTRY();
- /* To be able to scan const char we need a non const pointer */
- memcpy(&ptr, &tpl, sizeof(char *));
It would be better to use trick with discard_const_p
- for (;;) {
/* Find first % */ptr = strchr(ptr, '%');if (ptr == NULL) {TRACE_ERROR_NUMBER("No '%%d' found in format", EINVAL);return EINVAL;}else { /* Found */if (*(ptr + 1) == 'd') {ptr_pcnt = ptr + 2;/* We got a valid %d. Check the rest of the string. */for (;;) {ptr_pcnt = strchr(ptr_pcnt, '%');if (ptr_pcnt) {ptr_pcnt++;if (*ptr_pcnt != '%') {TRACE_ERROR_NUMBER("Single '%%' ""symbol after '%%d'.", EINVAL);return EINVAL;}ptr_pcnt++;}else break;}break;}/* This is %% - skip */else if (*(ptr + 1) == '%') {ptr += 2;continue;}else {TRACE_ERROR_NUMBER("Single '%%' ""symbol before '%%d'.", EINVAL);return EINVAL;}}- }
- TRACE_FLOW_EXIT();
- return EOK;
+}
From 8314f7f383d50fcb149eba96b1038d95b478b381 Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Thu, 25 Dec 2014 12:48:14 -0500 Subject: [PATCH 15/16] [INI] Unit test for the save interface
Patch adds a unit test and files used by the unit test. It is more convenient to use a different directory.
ini/ini2.d/real16be.conf | Bin 0 -> 2418 bytes ini/ini2.d/real16le.conf | Bin 0 -> 2418 bytes ini/ini2.d/real32be.conf | Bin 0 -> 4836 bytes ini/ini2.d/real32le.conf | Bin 0 -> 4836 bytes ini/ini2.d/real8.conf | 54 ++++++++++ ini/ini_save_ut.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 313 insertions(+), 0 deletions(-) create mode 100644 ini/ini2.d/real16be.conf create mode 100644 ini/ini2.d/real16le.conf create mode 100644 ini/ini2.d/real32be.conf create mode 100644 ini/ini2.d/real32le.conf create mode 100644 ini/ini2.d/real8.conf create mode 100644 ini/ini_save_ut.c
ACK
From 66fc3e50d0800e0cca57a83be080b0e54630646a Mon Sep 17 00:00:00 2001 From: Dmitri Pal dpal@redhat.com Date: Thu, 25 Dec 2014 12:49:49 -0500 Subject: [PATCH 16/16] [INI] Build new tests for the save interface
Patch adds changes to the make file to handle unit test for the new "save" interface functions.
Makefile.am | 20 +++++++++++++++++--- 1 files changed, 17 insertions(+), 3 deletions(-)
ACK
BTW Some functions are not tested
ini_config_add_int_arr_value ini_config_add_long_arr_value ini_config_add_double_arr_value ini_config_add_ulong_value ini_config_add_int32_value ini_config_add_uint32_value ini_config_add_uint64_value ini_config_add_double_value ini_config_delete_section_by_name
Most of them should be almost copy and paste. I mentioned it because ding-libs has very high code coverage and it would be good to do not decrease it.
If you do not have a time for creating unit tests You can just fix small updates and I can write tests instead of you.
LS
On (02/01/15 14:47), Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
Dimitri, I was writing additional unit tests for missing parts and I found a small problem with INI_VA_MOD and INI_VA_MODADD
The documentation says: /** * @brief Update a specific value (best effort). * * Value of the index is used to determine which one of the duplicates * needs to be updated. Index is 0-based. If the index is out of range * the function will do best effort and return the last instance of the key. * For example if you have five duplicates and you are searching for the tenth * the function will find and return the fifth instance. */ INI_VA_MOD = 1,
Input config: key0 = valuer0 key1 = value1a key1 = value1b key1 = value1c key1 = value1d key2 = value2 key3 = value3
Expected: Result: [zero] [zero] [one] [one] key0 = valuer0 key0 = valuer0 key1 = value1a key1 = value1a key1 = value1b key1 = value1b key1 = value1c key1 = value1c key1 = newvalue <<<<<<<<<< key1 = value1d <<<<<<< key2 = value2 <<<<<<<<<< key2 = newvalue <<<<<<< key3 = value3 key3 = value3
I need the second pair of eyes to look into this issue. I will appreciate if you could find few minutes. Attached is updated patches with check-based unit for this problem. (ini_configmod_ut_check)
BTW. It's not clear to me waht is a difference between INI_VA_MOD and INI_VA_MODADD or between INI_VA_MOD_E and INI_VA_MODADD_E. The code is the same.
LS
On 05/30/2015 03:49 PM, Lukas Slebodnik wrote:
On (02/01/15 14:47), Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
Dimitri, I was writing additional unit tests for missing parts and I found a small problem with INI_VA_MOD and INI_VA_MODADD
The documentation says: /**
- @brief Update a specific value (best effort).
- Value of the index is used to determine which one of the duplicates
- needs to be updated. Index is 0-based. If the index is out of range
- the function will do best effort and return the last instance of the key.
- For example if you have five duplicates and you are searching for the tenth
- the function will find and return the fifth instance.
*/ INI_VA_MOD = 1,
Input config: key0 = valuer0 key1 = value1a key1 = value1b key1 = value1c key1 = value1d key2 = value2 key3 = value3
Expected: Result: [zero] [zero] [one] [one] key0 = valuer0 key0 = valuer0 key1 = value1a key1 = value1a key1 = value1b key1 = value1b key1 = value1c key1 = value1c key1 = newvalue <<<<<<<<<< key1 = value1d <<<<<<< key2 = value2 <<<<<<<<<< key2 = newvalue <<<<<<< key3 = value3 key3 = value3
I need the second pair of eyes to look into this issue. I will appreciate if you could find few minutes. Attached is updated patches with check-based unit for this problem. (ini_configmod_ut_check)
BTW. It's not clear to me waht is a difference between INI_VA_MOD and INI_VA_MODADD or between INI_VA_MOD_E and INI_VA_MODADD_E. The code is the same.
LS
I will take a look when I have a moment. Thanks!
On 05/30/2015 03:49 PM, Lukas Slebodnik wrote:
On (02/01/15 14:47), Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
Dimitri, I was writing additional unit tests for missing parts and I found a small problem with INI_VA_MOD and INI_VA_MODADD
The documentation says: /**
- @brief Update a specific value (best effort).
- Value of the index is used to determine which one of the duplicates
- needs to be updated. Index is 0-based. If the index is out of range
- the function will do best effort and return the last instance of the key.
- For example if you have five duplicates and you are searching for the tenth
- the function will find and return the fifth instance.
*/ INI_VA_MOD = 1,
Input config: key0 = valuer0 key1 = value1a key1 = value1b key1 = value1c key1 = value1d key2 = value2 key3 = value3
Expected: Result: [zero] [zero] [one] [one] key0 = valuer0 key0 = valuer0 key1 = value1a key1 = value1a key1 = value1b key1 = value1b key1 = value1c key1 = value1c key1 = newvalue <<<<<<<<<< key1 = value1d <<<<<<< key2 = value2 <<<<<<<<<< key2 = newvalue <<<<<<< key3 = value3 key3 = value3
I need the second pair of eyes to look into this issue. I will appreciate if you could find few minutes. Attached is updated patches with check-based unit for this problem. (ini_configmod_ut_check)
BTW. It's not clear to me waht is a difference between INI_VA_MOD and INI_VA_MODADD or between INI_VA_MOD_E and INI_VA_MODADD_E. The code is the same.
LS
Hi Lukas,
I took some time and reviewed the test module and the code. Here are my findings:
1. In the test module system() call can return 0 but still fail. Please see how to check errors for the system() call in other ut modules in "ini". 2. I noticed that you do not check the status of f_memstream = open_memstream call. You probably should. 3. The main issue that you are asking about above looks like a bug to me. Under no conditions the key2 should be touched if you are adding or modifying key1. I looked at the code. The core of the issue is in the collection.c in col_parent_traverse_handler function Here is the section of code from the function where it handles the processing of the list
if (to_find->interrupt) { /* As soon as we found first non matching one but there was a match * return the parent of the last found item. */ if (((!match) || (current->next == NULL)) && (to_find->index != 0) && (to_find->found)) { *stop = 1; if (to_find->index == -2) *((struct collection_item **)traverse_data) = to_find->parent; else if (to_find->exact) { /* This means that we need to match the exact * index but we did not */ to_find->parent = NULL; to_find->found = 0; } else *((struct collection_item **)traverse_data) = to_find->parent->next; <----- WRONG for the use case we test. I think it should be just parent and not parent->next
However I suspect that some other test case will start to fail. This would mean that an if statement is missing within this last else above. Give it a try and let me know the results.
On (14/06/15 11:47), Dmitri Pal wrote:
On 05/30/2015 03:49 PM, Lukas Slebodnik wrote:
On (02/01/15 14:47), Dmitri Pal wrote:
Hello,
Please find attached patches for the new interface to modify configuration files using libini_config.
Dimitri, I was writing additional unit tests for missing parts and I found a small problem with INI_VA_MOD and INI_VA_MODADD
The documentation says: /**
- @brief Update a specific value (best effort).
- Value of the index is used to determine which one of the duplicates
- needs to be updated. Index is 0-based. If the index is out of range
- the function will do best effort and return the last instance of the key.
- For example if you have five duplicates and you are searching for the tenth
- the function will find and return the fifth instance.
*/ INI_VA_MOD = 1,
Input config: key0 = valuer0 key1 = value1a key1 = value1b key1 = value1c key1 = value1d key2 = value2 key3 = value3
Expected: Result: [zero] [zero] [one] [one] key0 = valuer0 key0 = valuer0 key1 = value1a key1 = value1a key1 = value1b key1 = value1b key1 = value1c key1 = value1c key1 = newvalue <<<<<<<<<< key1 = value1d <<<<<<< key2 = value2 <<<<<<<<<< key2 = newvalue <<<<<<< key3 = value3 key3 = value3
I need the second pair of eyes to look into this issue. I will appreciate if you could find few minutes. Attached is updated patches with check-based unit for this problem. (ini_configmod_ut_check)
BTW. It's not clear to me waht is a difference between INI_VA_MOD and INI_VA_MODADD or between INI_VA_MOD_E and INI_VA_MODADD_E. The code is the same.
LS
Hi Lukas,
I took some time and reviewed the test module and the code. Here are my findings:
- In the test module system() call can return 0 but still fail. Please see
how to check errors for the system() call in other ut modules in "ini".
I compare in memory configurations with memcmp. The call system (with utility diff) is used just for printing diff to stdout in case of different results.
- I noticed that you do not check the status of f_memstream = open_memstream
call. You probably should.
Thank you. An asserion was added.
- The main issue that you are asking about above looks like a bug to me.
Under no conditions the key2 should be touched if you are adding or modifying key1. I looked at the code. The core of the issue is in the collection.c in col_parent_traverse_handler function Here is the section of code from the function where it handles the processing of the list
if (to_find->interrupt) { /* As soon as we found first non matching one but there was amatch * return the parent of the last found item. */ if (((!match) || (current->next == NULL)) && (to_find->index != 0) && (to_find->found)) { *stop = 1; if (to_find->index == -2) *((struct collection_item **)traverse_data) = to_find->parent; else if (to_find->exact) { /* This means that we need to match the exact * index but we did not */ to_find->parent = NULL; to_find->found = 0; } else *((struct collection_item **)traverse_data) = to_find->parent->next; <----- WRONG for the use case we test. I think it should be just parent and not parent->next
I tried many times but any change to col_parent_traverse_handler caused failed collection unit test. So I decided to slightly modify function col_get_dup_item.
However I suspect that some other test case will start to fail. This would mean that an if statement is missing within this last else above. Give it a try and let me know the results.
Patches 1-16 are Dimitri's patches and other patches are my bug fixes + unit test. It's quite big but it covers all functionality of function ini_config_add_str_value which is reused by other public functions.
LS
On Tue, Jun 23, 2015 at 10:36:30AM +0200, Lukas Slebodnik wrote:
Patches 1-16 are Dimitri's patches and other patches are my bug fixes + unit test. It's quite big but it covers all functionality of function ini_config_add_str_value which is reused by other public functions.
I'll only review Lukas' fixes: * ini_config_ut: enable verbose mode with env variable - ACK
* collection: Add new function col_remove_item_with_cb - the changes are fine, but do you want to also change the TRACE_FLOW_STRING from Exit/Exit to Enter/Exit in col_remove_item_with_cb()?
Also col_remove_item() has no tracing, but I guess that's OK since it's just a wrapper.
* INI: Fix memory leak with INI_VA_CLEAN - ACK, value_destroy() would now be called
* COLLECTION: Return the last duplicate for big index - sorry, I'm too detached from the code to understand the change, can you add a comment what does this block do? Style-wise, why did you use Yoda-condition?
* INI: Fix adding string with INI_VA_MODADD_E and big index - this seems a bit strange to me, wouldn't it be better to INI_VA_MODADD/INI_VA_MODADD_E cases? In particular, using exact case always to 1 even for INI_VA_MODADD seems a bit odd?
* INI: Add check based test ini_configmod_ut_check Seems to cover the code well, runs fine and has no leaks: ==18028== total heap usage: 20,176 allocs, 20,176 frees, 4,786,296 bytes allocated
On (23/06/15 11:25), Jakub Hrozek wrote:
On Tue, Jun 23, 2015 at 10:36:30AM +0200, Lukas Slebodnik wrote:
Patches 1-16 are Dimitri's patches and other patches are my bug fixes + unit test. It's quite big but it covers all functionality of function ini_config_add_str_value which is reused by other public functions.
I'll only review Lukas' fixes:
ini_config_ut: enable verbose mode with env variable - ACK
collection: Add new function col_remove_item_with_cb - the changes are fine, but do you want to also change the TRACE_FLOW_STRING from Exit/Exit to Enter/Exit in col_remove_item_with_cb()?
Nice catch. Fixed.
Also col_remove_item() has no tracing, but I guess that's OK since it's just a wrapper.
INI: Fix memory leak with INI_VA_CLEAN - ACK, value_destroy() would now be called
COLLECTION: Return the last duplicate for big index - sorry, I'm too detached from the code to understand the change, can you add a comment what does this block do?
col_get_dup_item should return last duplicate for very big index (out of bound) and not different property.
Comment was added.
Style-wise, why did you use Yoda-condition?
Updated.
- INI: Fix adding string with INI_VA_MODADD_E and big index - this seems a bit strange to me, wouldn't it be better to INI_VA_MODADD/INI_VA_MODADD_E cases? In particular, using exact case always to 1 even for INI_VA_MODADD seems a bit odd?
I split INI_VA_MODADD INI_VA_MODADD_E. But code INI_VA_MODADD_E is almost the same as in previous patch. Because We need to distinguish between non-existing property (ENOENT) and very big index + "exact index flag" (ENOENT)
- INI: Add check based test ini_configmod_ut_check
Seems to cover the code well, runs fine and has no leaks: ==18028== total heap usage: 20,176 allocs, 20,176 frees, 4,786,296 bytes allocated
Updated version is attached.
LS
On Tue, Jun 23, 2015 at 12:59:31PM +0200, Lukas Slebodnik wrote:
On (23/06/15 11:25), Jakub Hrozek wrote:
On Tue, Jun 23, 2015 at 10:36:30AM +0200, Lukas Slebodnik wrote:
Patches 1-16 are Dimitri's patches and other patches are my bug fixes + unit test. It's quite big but it covers all functionality of function ini_config_add_str_value which is reused by other public functions.
I'll only review Lukas' fixes:
ini_config_ut: enable verbose mode with env variable - ACK
collection: Add new function col_remove_item_with_cb - the changes are fine, but do you want to also change the TRACE_FLOW_STRING from Exit/Exit to Enter/Exit in col_remove_item_with_cb()?
Nice catch. Fixed.
ACK
Also col_remove_item() has no tracing, but I guess that's OK since it's just a wrapper.
INI: Fix memory leak with INI_VA_CLEAN - ACK, value_destroy() would now be called
COLLECTION: Return the last duplicate for big index - sorry, I'm too detached from the code to understand the change, can you add a comment what does this block do?
col_get_dup_item should return last duplicate for very big index (out of bound) and not different property.
Comment was added.
Thank you, ACK
Style-wise, why did you use Yoda-condition?
Updated.
- INI: Fix adding string with INI_VA_MODADD_E and big index - this seems a bit strange to me, wouldn't it be better to INI_VA_MODADD/INI_VA_MODADD_E cases? In particular, using exact case always to 1 even for INI_VA_MODADD seems a bit odd?
I split INI_VA_MODADD INI_VA_MODADD_E. But code INI_VA_MODADD_E is almost the same as in previous patch. Because We need to distinguish between non-existing property (ENOENT) and very big index + "exact index flag" (ENOENT)
I think that's fine, because the code is now easier to read even if you don't know the code too deep. It's just clearer.
ACK.
- INI: Add check based test ini_configmod_ut_check
Seems to cover the code well, runs fine and has no leaks: ==18028== total heap usage: 20,176 allocs, 20,176 frees, 4,786,296 bytes allocated
Updated version is attached.
LS
ACK to all your patches.
On Tue, Jun 23, 2015 at 01:29:57PM +0200, Jakub Hrozek wrote:
ACK to all your patches.
Lukas acked all Dmitri's patches and pushed the whole lot to master: 4d7e92836d8582a8af2f2885d4fcae34fd14b662 c26d21b8702112145c2076e7140e390a5647bc72 a223f05ccc6346d086519342bc3027ed5d0027e1 1d8d283f7788b0993db7defdbb400901eb9d9d53 57b647bd1877d61d400ae9ed9648564f6c0bda7b 8622a49fbdf77cff22244f866cb978cfbfdd3d16 9c1a5dcc940bca6ee52f3d2feb0c54460c468d2e 40e13331ad5534a05510a7fac3000067a573c70e 5108e04823a6ec0fc39519df3bce768e3e976ee2 3b7e0fee81b74d4dd853f6d13e2eab5d9747db40 7f52581ddad5e30bf254900da3e1f05a92952822 dd9bf166cce68a8e5d6928f61dfcbb45d1e1bd0f ad5b5830484c3951d5e954133c58263e1fe7e2d1 d302c00a74d7526ba5f151169b35f5ed459e02d0 8dce07627b72adaaafe1462338a4dba44247bb85 2aec9f8ac2e3b554298461ed45ea6cc2f4bc5c22 c4e84b7f9236712c917b71fe781334aae4625b3a 8f2937e056a49a9288b1e8ac1d7d5f8800f72c8d 447779a744b80e58c0af40cec91140895adae49f 8d5b84d3f3503a7dd552efc41e383d744d5abf17 6bd0e75686dfb80ad1c4079b13bfcc79384372ab 081ed5117807eabbc5d56a7592163eb3a519789a
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Lukas Slebodnik