From 6f9edcb8a8677e87ff65e0cea3d5d86d98607061 Mon Sep 17 00:00:00 2001
From: Dmitri Pal <dpal@redhat.com>
Date: Mon, 24 Dec 2012 17:45:00 -0500
Subject: [PATCH 3/4] [INI] More interface refactoring

I also realized that error list processing should not be
bound to the file object.
This patch corrects that by moving the error_list and
corresponding count from file object to the config object.
Things updated by the patch:
1. The internal variables are moved from file obj to config obj.
2. The external header is updated to reflect the change
3. Functions are moved from file obj module to config obj module.
4. Parser code is updated because error validation was in
   the wrong place
5. Unit test is adjusted to get error list from the right object.

I had to adjust the copy function for the config object.
Copy function does not copy over the error list.
---
 ini/ini_config_priv.h |    8 ++--
 ini/ini_configobj.c   |  142 +++++++++++++++++++++++++++++++++++++++++++++++++
 ini/ini_configobj.h   |    9 ++--
 ini/ini_fileobj.c     |  136 ----------------------------------------------
 ini/ini_parse.c       |   27 +++------
 ini/ini_parse_ut.c    |   24 ++++----
 6 files changed, 172 insertions(+), 174 deletions(-)

diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h
index ee6a4d7eda3fee645388bbfbdc17f121f67dd1a5..31cee84cf4815dab2378c06af1878cb82c913585 100644
--- a/ini/ini_config_priv.h
+++ b/ini/ini_config_priv.h
@@ -42,6 +42,10 @@ struct ini_cfgobj {
     int section_len;
     int name_len;
     struct collection_iterator *iterator;
+    /* Collection of errors detected during parsing */
+    struct collection_item *error_list;
+    /* Count of error lines */
+    unsigned count;
 
     /*...         */
     /* Statistics? Timestamps? When created? Modified? - TBD */
@@ -63,12 +67,8 @@ struct ini_cfgfile {
     /**********************/
     /* Internal variables */
     /**********************/
-    /* Collection of errors detected during parsing */
-    struct collection_item *error_list;
     /* File stats */
     struct stat file_stats;
-    /* Count of error lines */
-    unsigned count;
 };
 
 /* Parsing error */
diff --git a/ini/ini_configobj.c b/ini/ini_configobj.c
index 4a3e96d2645f853a9f0b392fe9b3ffc206c11411..c3ce27c6aa3b5edd7426fa4feba247214311787c 100644
--- a/ini/ini_configobj.c
+++ b/ini/ini_configobj.c
@@ -24,8 +24,14 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdint.h>
+#include <stdlib.h>
+/* For error text */
+#include <libintl.h>
+#define _(String) gettext (String)
 #include "trace.h"
 #include "collection.h"
+#include "collection_tools.h"
+#include "ini_configobj.h"
 #include "ini_config_priv.h"
 #include "ini_defines.h"
 #include "ini_valueobj.h"
@@ -99,6 +105,11 @@ void ini_config_destroy(struct ini_cfgobj *ini_config)
         if (ini_config->last_comment) {
             ini_comment_destroy(ini_config->last_comment);
         }
+
+        if (ini_config->error_list) {
+            col_destroy_collection(ini_config->error_list);
+        }
+
         free(ini_config);
     }
 
@@ -132,6 +143,8 @@ int ini_config_create(struct ini_cfgobj **ini_config)
     new_co->section_len = 0;
     new_co->name_len = 0;
     new_co->iterator = NULL;
+    new_co->error_list = NULL;
+    new_co->count = 0;
 
     /* Create a collection to hold configuration data */
     error = col_create_collection(&(new_co->cfg),
@@ -143,6 +156,16 @@ int ini_config_create(struct ini_cfgobj **ini_config)
         return error;
     }
 
+    /* Create error list collection */
+    error = col_create_collection(&(new_co->error_list),
+                                  INI_ERROR,
+                                  COL_CLASS_INI_PERROR);
+    if (error) {
+        TRACE_ERROR_NUMBER("Failed to create error list", error);
+        ini_config_destroy(new_co);
+        return error;
+    }
+
     *ini_config = new_co;
 
     TRACE_FLOW_EXIT();
@@ -274,6 +297,8 @@ int ini_config_copy(struct ini_cfgobj *ini_config,
     new_co->section_len = 0;
     new_co->name_len = 0;
     new_co->iterator = NULL;
+    new_co->error_list = NULL;
+    new_co->count = 0;
 
     error = col_copy_collection_with_cb(&(new_co->cfg),
                                         ini_config->cfg,
@@ -898,3 +923,120 @@ int ini_config_merge(struct ini_cfgobj *first,
     return error;
 
 }
+
+/* How many errors do we have in the list ? */
+unsigned ini_config_error_count(struct ini_cfgobj *cfg_ctx)
+{
+    unsigned count = 0;
+
+    TRACE_FLOW_ENTRY();
+
+    count = cfg_ctx->count;
+
+    TRACE_FLOW_EXIT();
+    return count;
+
+}
+
+/* Free error strings */
+void ini_config_free_errors(char **errors)
+{
+    TRACE_FLOW_ENTRY();
+
+    col_free_property_list(errors);
+
+    TRACE_FLOW_EXIT();
+}
+
+/* Get the list of error strings */
+int ini_config_get_errors(struct ini_cfgobj *cfg_ctx,
+                          char ***errors)
+{
+    char **errlist = NULL;
+    struct collection_iterator *iterator = NULL;
+    int error;
+    struct collection_item *item = NULL;
+    struct ini_parse_error *pe;
+    unsigned int count = 0;
+    char *line;
+
+    TRACE_FLOW_ENTRY();
+
+    /* If we have something to print print it */
+    if ((!errors) || (!cfg_ctx)) {
+        TRACE_ERROR_NUMBER("Invalid parameter.", EINVAL);
+        return EINVAL;
+    }
+
+    errlist = calloc(cfg_ctx->count + 1, sizeof(char *));
+    if (!errlist) {
+        TRACE_ERROR_NUMBER("Failed to allocate memory for errors.", ENOMEM);
+        return ENOMEM;
+    }
+
+    /* Bind iterator */
+    error =  col_bind_iterator(&iterator,
+                               cfg_ctx->error_list,
+                               COL_TRAVERSE_DEFAULT);
+    if (error) {
+        TRACE_ERROR_NUMBER("Faile to bind iterator:", error);
+        ini_config_free_errors(errlist);
+        return error;
+    }
+
+    while(1) {
+        /* Loop through a collection */
+        error = col_iterate_collection(iterator, &item);
+        if (error) {
+            TRACE_ERROR_NUMBER("Error iterating collection", error);
+            col_unbind_iterator(iterator);
+            ini_config_free_errors(errlist);
+            return error;
+        }
+
+        /* Are we done ? */
+        if (item == NULL) break;
+
+        /* Process collection header */
+        if (col_get_item_type(item) == COL_TYPE_COLLECTION) {
+            continue;
+        }
+        else {
+            /* Put error into provided format */
+            pe = (struct ini_parse_error *)(col_get_item_data(item));
+
+            /* Would be nice to have asprintf function...
+             * ...but for now we know that all the errors
+             * are pretty short and will fir into the predefined
+             * error length buffer.
+             */
+            line = malloc(MAX_ERROR_LINE + 1);
+            if (!line) {
+                TRACE_ERROR_NUMBER("Failed to get memory for error.", ENOMEM);
+                col_unbind_iterator(iterator);
+                ini_config_free_errors(errlist);
+                return ENOMEM;
+            }
+
+            snprintf(line, MAX_ERROR_LINE, LINE_FORMAT,
+                     col_get_item_property(item, NULL),
+                     pe->error,
+                     pe->line,
+                     ini_get_error_str(pe->error,
+                                       INI_FAMILY_PARSING));
+
+            errlist[count] = line;
+            count++;
+        }
+
+    }
+
+    /* Do not forget to unbind iterator - otherwise there will be a leak */
+    col_unbind_iterator(iterator);
+
+    *errors = errlist;
+
+    TRACE_FLOW_EXIT();
+    return error;
+}
+
diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
index 5e8d95baa788d9615e901165cc7f35b760eb5997..16f6499862ddb5f933baaedce75b6baac32c3eaf 100644
--- a/ini/ini_configobj.h
+++ b/ini/ini_configobj.h
@@ -526,11 +526,11 @@ void ini_config_file_destroy(struct ini_cfgfile *file_ctx);
  * how many parsing errors were found during last
  * parsing operation.
  *
- * @param[in]  file_ctx         Configuration file object.
+ * @param[in]  ini_config       Configuration object.
  *
  * @return Number of errors.
  */
-unsigned ini_config_error_count(struct ini_cfgfile *file_ctx);
+unsigned ini_config_error_count(struct ini_cfgobj *ini_config);
 
 /**
  * @brief Get array of parsing errors
@@ -541,14 +541,14 @@ unsigned ini_config_error_count(struct ini_cfgfile *file_ctx);
  * Array can be referenced as a normal array of strings.
  * The NULL entry indicates the end of the array.
  *
- * @param[in]  file_ctx         Configuration file object.
+ * @param[in]  ini_config       Configuration object.
  * @param[out] errors           Array of error strings.
  *
  * @return 0 - Success.
  * @return EINVAL - Invalid parameter.
  * @return ENOMEM - No memory.
  */
-int ini_config_get_errors(struct ini_cfgfile *file_ctx,
+int ini_config_get_errors(struct ini_cfgobj *ini_config,
                           char ***errors);
 
 /**
@@ -693,6 +693,7 @@ int ini_config_parse(struct ini_cfgfile *file_ctx,
  * @brief Create a copy of the configuration object
  *
  * Function creates a deep copy of all the configuration data.
+ * Error list created during parsing is not copied over.
  *
  * @param[in]  ini_config       Original configuration object.
  * @param[out] ini_new          A new configuration object.
diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
index 67734653e5b9534a4f177a2030ba317bbde76d78..677383c9de12263cbf51fbace11ff7c878a175fc 100644
--- a/ini/ini_fileobj.c
+++ b/ini/ini_fileobj.c
@@ -27,9 +27,7 @@
 #include "ini_defines.h"
 #include "ini_configobj.h"
 #include "ini_config_priv.h"
-#include "collection.h"
 #include "path_utils.h"
-#include "collection_tools.h"
 
 
 /* Close file but not destroy the object */
@@ -54,7 +52,6 @@ void ini_config_file_destroy(struct ini_cfgfile *file_ctx)
 
     if(file_ctx) {
         free(file_ctx->filename);
-        col_destroy_collection(file_ctx->error_list);
         if(file_ctx->file) fclose(file_ctx->file);
         free(file_ctx);
     }
@@ -79,15 +76,6 @@ static int common_file_init(struct ini_cfgfile *file_ctx)
         return error;
     }
 
-    /* Create internal collections */
-    error = col_create_collection(&(file_ctx->error_list),
-                                  INI_ERROR,
-                                  COL_CLASS_INI_PERROR);
-    if (error) {
-        TRACE_ERROR_NUMBER("Failed to create error list", error);
-        return error;
-    }
-
     /* Collect stats */
     if (file_ctx->metadata_flags & INI_META_STATS) {
         errno = 0;
@@ -128,11 +116,9 @@ int ini_config_file_open(const char *filename,
 
     new_ctx->filename = NULL;
     new_ctx->file = NULL;
-    new_ctx->error_list = NULL;
 
     /* Store flags */
     new_ctx->metadata_flags = metadata_flags;
-    new_ctx->count = 0;
 
     /* Construct the full file path */
     new_ctx->filename = malloc(PATH_MAX + 1);
@@ -187,11 +173,9 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
     }
 
     new_ctx->file = NULL;
-    new_ctx->error_list = NULL;
 
     /* Store flags */
     new_ctx->metadata_flags = file_ctx_in->metadata_flags;
-    new_ctx->count = 0;
 
     /* Copy full file path */
     errno = 0;
@@ -216,122 +200,6 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
     return error;
 }
 
-/* How many errors do we have in the list ? */
-unsigned ini_config_error_count(struct ini_cfgfile *file_ctx)
-{
-    unsigned count = 0;
-
-    TRACE_FLOW_ENTRY();
-
-    count = file_ctx->count;
-
-    TRACE_FLOW_EXIT();
-    return count;
-
-}
-
-/* Free error strings */
-void ini_config_free_errors(char **errors)
-{
-    TRACE_FLOW_ENTRY();
-
-    col_free_property_list(errors);
-
-    TRACE_FLOW_EXIT();
-}
-
-/* Get the list of error strings */
-int ini_config_get_errors(struct ini_cfgfile *file_ctx,
-                          char ***errors)
-{
-    char **errlist = NULL;
-    struct collection_iterator *iterator = NULL;
-    int error;
-    struct collection_item *item = NULL;
-    struct ini_parse_error *pe;
-    unsigned int count = 0;
-    char *line;
-
-    TRACE_FLOW_ENTRY();
-
-    /* If we have something to print print it */
-    if ((!errors) || (!file_ctx)) {
-        TRACE_ERROR_NUMBER("Invalid parameter.", EINVAL);
-        return EINVAL;
-    }
-
-    errlist = calloc(file_ctx->count + 1, sizeof(char *));
-    if (!errlist) {
-        TRACE_ERROR_NUMBER("Failed to allocate memory for errors.", ENOMEM);
-        return ENOMEM;
-    }
-
-    /* Bind iterator */
-    error =  col_bind_iterator(&iterator,
-                               file_ctx->error_list,
-                               COL_TRAVERSE_DEFAULT);
-    if (error) {
-        TRACE_ERROR_NUMBER("Faile to bind iterator:", error);
-        ini_config_free_errors(errlist);
-        return error;
-    }
-
-    while(1) {
-        /* Loop through a collection */
-        error = col_iterate_collection(iterator, &item);
-        if (error) {
-            TRACE_ERROR_NUMBER("Error iterating collection", error);
-            col_unbind_iterator(iterator);
-            ini_config_free_errors(errlist);
-            return error;
-        }
-
-        /* Are we done ? */
-        if (item == NULL) break;
-
-        /* Process collection header */
-        if (col_get_item_type(item) == COL_TYPE_COLLECTION) {
-            continue;
-        }
-        else {
-            /* Put error into provided format */
-            pe = (struct ini_parse_error *)(col_get_item_data(item));
-
-            /* Would be nice to have asprintf function...
-             * ...but for now we know that all the errors
-             * are pretty short and will fir into the predefined
-             * error length buffer.
-             */
-            line = malloc(MAX_ERROR_LINE + 1);
-            if (!line) {
-                TRACE_ERROR_NUMBER("Failed to get memory for error.", ENOMEM);
-                col_unbind_iterator(iterator);
-                ini_config_free_errors(errlist);
-                return ENOMEM;
-            }
-
-            snprintf(line, MAX_ERROR_LINE, LINE_FORMAT,
-                     col_get_item_property(item, NULL),
-                     pe->error,
-                     pe->line,
-                     ini_get_error_str(pe->error,
-                                       INI_FAMILY_PARSING));
-
-            errlist[count] = line;
-            count++;
-        }
-
-    }
-
-    /* Do not forget to unbind iterator - otherwise there will be a leak */
-    col_unbind_iterator(iterator);
-
-    *errors = errlist;
-
-    TRACE_FLOW_EXIT();
-    return error;
-}
-
 /* Get the fully resolved file name */
 const char *ini_config_get_filename(struct ini_cfgfile *file_ctx)
 {
@@ -344,7 +212,6 @@ const char *ini_config_get_filename(struct ini_cfgfile *file_ctx)
     return ret;
 }
 
-
 /* Check access */
 int ini_config_access_check(struct ini_cfgfile *file_ctx,
                             uint32_t flags,
@@ -471,8 +338,6 @@ void ini_config_file_print(struct ini_cfgfile *file_ctx)
         printf("File name: %s\n", (file_ctx->filename) ? file_ctx->filename : "NULL");
         printf("File is %s\n", (file_ctx->file) ? "open" : "closed");
         printf("Metadata flags %u\n", file_ctx->metadata_flags);
-        if (file_ctx->error_list) col_print_collection(file_ctx->error_list);
-        else printf("Error list is empty.");
         printf("Stats flag st_dev %li\n", file_ctx->file_stats.st_dev);
         printf("Stats flag st_ino %li\n", file_ctx->file_stats.st_ino);
         printf("Stats flag st_mode %u\n", file_ctx->file_stats.st_mode);
@@ -485,7 +350,6 @@ void ini_config_file_print(struct ini_cfgfile *file_ctx)
         printf("Stats flag st_atime %ld\n", file_ctx->file_stats.st_atime);
         printf("Stats flag st_mtime %ld\n", file_ctx->file_stats.st_mtime);
         printf("Stats flag st_ctime %ld\n", file_ctx->file_stats.st_ctime);
-        printf("Count %u\n", file_ctx->count);
     }
     TRACE_FLOW_EXIT();
 }
diff --git a/ini/ini_parse.c b/ini/ini_parse.c
index d009914360858959cd3b48d0c087f919504ebe36..60ef1169986f2ea27596931ebf16fab166c71937 100644
--- a/ini/ini_parse.c
+++ b/ini/ini_parse.c
@@ -202,7 +202,6 @@ static int parser_create(struct ini_cfgobj *co,
                          int error_level,
                          uint32_t collision_flags,
                          uint32_t parse_flags,
-                         struct collection_item *error_list,
                          struct parser_obj **po)
 {
     int error = EOK;
@@ -216,15 +215,7 @@ static int parser_create(struct ini_cfgobj *co,
         (!co) ||
         (!(co->cfg)) ||
         (!file) ||
-        (!config_filename) ||
-        (!error_list)) {
-        TRACE_ERROR_NUMBER("Invalid argument", EINVAL);
-        return EINVAL;
-    }
-
-    if ((error_level != INI_STOP_ON_ANY) &&
-        (error_level != INI_STOP_ON_NONE) &&
-        (error_level != INI_STOP_ON_ERROR)) {
+        (!config_filename)) {
         TRACE_ERROR_NUMBER("Invalid argument", EINVAL);
         return EINVAL;
     }
@@ -248,7 +239,7 @@ static int parser_create(struct ini_cfgobj *co,
 
     /* Save external data */
     new_po->file = file;
-    new_po->el = error_list;
+    new_po->el = co->error_list;
     new_po->filename = config_filename;
     new_po->error_level = error_level;
     new_po->collision_flags = collision_flags;
@@ -1618,10 +1609,11 @@ int ini_config_parse(struct ini_cfgfile *file_ctx,
         return EINVAL;
     }
 
-    if ((error_level < INI_STOP_ON_ANY) ||
-        (error_level > INI_STOP_ON_ERROR)) {
-        /* Any other error mode is equivalent stopping on any error */
-        error_level = INI_STOP_ON_ANY;
+    if ((error_level != INI_STOP_ON_ANY) &&
+        (error_level != INI_STOP_ON_NONE) &&
+        (error_level != INI_STOP_ON_ERROR)) {
+        TRACE_ERROR_NUMBER("Invalid argument", EINVAL);
+        return EINVAL;
     }
 
     error = parser_create(ini_config,
@@ -1630,7 +1622,6 @@ int ini_config_parse(struct ini_cfgfile *file_ctx,
                           error_level,
                           collision_flags,
                           parse_flags,
-                          file_ctx->error_list,
                           &po);
     if (error) {
         TRACE_ERROR_NUMBER("Failed to perform an action", error);
@@ -1658,8 +1649,8 @@ int ini_config_parse(struct ini_cfgfile *file_ctx,
         else {
             TRACE_ERROR_NUMBER("Failed to parse file", error);
             TRACE_ERROR_NUMBER("Mode", collision_flags);
-            col_get_collection_count(file_ctx->error_list, &(file_ctx->count));
-            if(file_ctx->count) (file_ctx->count)--;
+            col_get_collection_count(ini_config->error_list, &(ini_config->count));
+            if(ini_config->count) (ini_config->count)--;
             parser_destroy(po);
             return error;
         }
diff --git a/ini/ini_parse_ut.c b/ini/ini_parse_ut.c
index 3526d488916cd00f1101f448a4c4a36b9c7f5adc..e31b356df4f46c5e9d2d7c2c716defb5b38b0d68 100644
--- a/ini/ini_parse_ut.c
+++ b/ini/ini_parse_ut.c
@@ -86,10 +86,10 @@ int test_one_file(const char *in_filename,
     if (error) {
         INIOUT(printf("Failed to parse configuration. Error %d.\n", error));
 
-        if (ini_config_error_count(file_ctx)) {
+        if (ini_config_error_count(ini_config)) {
             INIOUT(printf("Errors detected while parsing: %s\n",
                    ini_config_get_filename(file_ctx)));
-            ini_config_get_errors(file_ctx, &error_list);
+            ini_config_get_errors(ini_config, &error_list);
             INIOUT(ini_config_print_errors(stdout, error_list));
             ini_config_free_errors(error_list);
         }
@@ -389,10 +389,10 @@ int merge_values_test(void)
             INIOUT(printf("Failed to parse configuration. Error %d.\n",
                           error));
 
-            if (ini_config_error_count(file_ctx)) {
+            if (ini_config_error_count(ini_config)) {
                 INIOUT(printf("Errors detected while parsing: %s\n",
                        ini_config_get_filename(file_ctx)));
-                ini_config_get_errors(file_ctx, &error_list);
+                ini_config_get_errors(ini_config, &error_list);
                 INIOUT(ini_config_print_errors(stdout, error_list));
                 ini_config_free_errors(error_list);
             }
@@ -585,10 +585,10 @@ int merge_section_test(void)
                 INIOUT(printf("Failed to parse configuration. "
                               "Error %d.\n", error));
 
-                if (ini_config_error_count(file_ctx)) {
+                if (ini_config_error_count(ini_config)) {
                     INIOUT(printf("Errors detected while parsing: %s\n",
                            ini_config_get_filename(file_ctx)));
-                    ini_config_get_errors(file_ctx, &error_list);
+                    ini_config_get_errors(ini_config, &error_list);
                     INIOUT(ini_config_print_errors(stdout, error_list));
                     ini_config_free_errors(error_list);
                 }
@@ -707,10 +707,10 @@ int read_one_file(char *name,
         INIOUT(printf("Failed to parse configuration. "
                       "Error %d.\n", error));
 
-        if (ini_config_error_count(file_ctx)) {
+        if (ini_config_error_count(ini_config)) {
             INIOUT(printf("Errors detected while parsing: %s\n",
                    ini_config_get_filename(file_ctx)));
-            ini_config_get_errors(file_ctx, &error_list);
+            ini_config_get_errors(ini_config, &error_list);
             INIOUT(ini_config_print_errors(stdout, error_list));
             ini_config_free_errors(error_list);
         }
@@ -1146,10 +1146,10 @@ int startup_test(void)
     if (error) {
         INIOUT(printf("Failed to parse configuration. Error %d.\n", error));
 
-        if (ini_config_error_count(file_ctx)) {
+        if (ini_config_error_count(ini_config)) {
             INIOUT(printf("Errors detected while parsing: %s\n",
                    ini_config_get_filename(file_ctx)));
-            ini_config_get_errors(file_ctx, &error_list);
+            ini_config_get_errors(ini_config, &error_list);
             INIOUT(ini_config_print_errors(stdout, error_list));
             ini_config_free_errors(error_list);
         }
@@ -1430,10 +1430,10 @@ int get_test(void)
     if (error) {
         INIOUT(printf("Failed to parse configuration. Error %d.\n", error));
 
-        if (ini_config_error_count(file_ctx)) {
+        if (ini_config_error_count(ini_config)) {
             INIOUT(printf("Errors detected while parsing: %s\n",
                    ini_config_get_filename(file_ctx)));
-            ini_config_get_errors(file_ctx, &error_list);
+            ini_config_get_errors(ini_config, &error_list);
             INIOUT(ini_config_print_errors(stdout, error_list));
             ini_config_free_errors(error_list);
         }
-- 
1.7.1

