[SSSD] [INI] Patches for the INI configuration modification
Lukas Slebodnik
lslebodn at redhat.com
Wed May 6 14:55:52 UTC 2015
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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at 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 at redhat.com> 2014
>+
>+ INI Library is free software: you can redistribute it and/or modify
>+ it under the terms of the GNU Lesser General Public License as published by
>+ the Free Software Foundation, either version 3 of the License, or
>+ (at your option) any later version.
>+
>+ INI Library is distributed in the hope that it will be useful,
>+ but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+ GNU Lesser General Public License for more details.
>+
>+ You should have received a copy of the GNU Lesser General Public License
>+ along with INI Library. If not, see <http://www.gnu.org/licenses/>.
>+*/
>+/* 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 at redhat.com> 2014
>+
>+ INI Library is free software: you can redistribute it and/or modify
>+ it under the terms of the GNU Lesser General Public License as published by
>+ the Free Software Foundation, either version 3 of the License, or
>+ (at your option) any later version.
>+
>+ INI Library is distributed in the hope that it will be useful,
>+ but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+ GNU Lesser General Public License for more details.
>+
>+ You should have received a copy of the GNU Lesser General Public License
>+ along with INI Library. If not, see <http://www.gnu.org/licenses/>.
>+*/
>+
>+#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 at 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 at 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 at 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 at 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 at 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.
> */
> uid_t uid; /**< Expected uid of the file. */
> gid_t gid; /**< Expected gid of the file.*/
>@@ -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 at 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 at 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 at 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
More information about the sssd-devel
mailing list