[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