[SSSD] [ding-libs] Patches for #2267, build issue and tight loop

Lukas Slebodnik lslebodn at redhat.com
Tue Mar 18 06:02:21 UTC 2014


On (17/03/14 18:05), Dmitri Pal wrote:
>On 03/17/2014 05:00 PM, Lukas Slebodnik wrote:
>>On (08/03/14 19:11), Dmitri Pal wrote:
>>>Hello,
>>>
>>>The following patches address the request to be able to read config
>>>data from a memory buffer rather than a file.
>>>Ticket: https://fedorahosted.org/sssd/ticket/2267
>>>
>>>It also addresses a build problem that I found on my system and fixes
>>>a tight loop in parsing when we ignore errors and file is empty.
>>>
>>>
>>>-- 
>>>Thank you,
>>>Dmitri Pal
>>>
>>>Sr. Engineering Manager for IdM portfolio
>>>Red Hat Inc.
>>>
>>>
>>>-------------------------------
>>>Looking to carve out IT costs?
>>>www.redhat.com/carveoutcosts/
>>>
>>>
>>>
>>> From 30a92ababaa9148a7354ebf52bf318d37b5451d2 Mon Sep 17 00:00:00 2001
>>>From: Dmitri Pal<dpal at redhat.com>
>>>Date: Sat, 8 Mar 2014 19:00:57 -0500
>>>Subject: [PATCH 2/4] [INI] New entry to read data from mem
>>>
>>>The commit adds a new entry point to read configuration
>>>data from a memory buffer instead of file.
>>>---
>>>ini/ini_configobj.h   |   25 ++++++
>>>ini/ini_fileobj.c     |  204 +++++++++++++++++++++++++++++++++++++------------
>>>ini/libini_config.sym |    1 +
>>>3 files changed, 182 insertions(+), 48 deletions(-)
>>>
>>>diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
>>>index 9a4eb58..4ef98dc 100644
>>>--- a/ini/ini_configobj.h
>>>+++ b/ini/ini_configobj.h
>>>@@ -483,6 +483,31 @@ int ini_config_file_open(const char *filename,
>>>                          struct ini_cfgfile **file_ctx);
>>>
>>>/**
>>>+ * @brief Create a configuration file object using memory buffer.
>>>+ *
>>>+ * Create a file object for parsing a configuration file.
>>>+ * Configuration will use provided memory instead of the actual file.
>>>+ *
>>>+ * A "configuration file object" is different from
>>>+ * a "configuration object". The former stores metadata
>>>+ * about a file the configuration data is read from,
>>>+ * while the latter holds the configuration itself.
>>>+ *
>>>+ * @param[in]  data_buf         In memory configuration data.
>>>+ *                              Needs to be NULL terminated.
>>                                               ^^^^^^^^^^^^^^^^
>>                             In my opinion, it is little bit confusing.
>>                             Because argument data_buf has type "void *".
>>                             What about: byte array null byte ('\0')
>>
>>Do we really need null byte terminated string?
>>I think fmemopen will need null byte terminated string only if buffer is opened
>>in append mode.
>
>I tired without extra byte. Tried twice.
>I does not work.
>I did not like it in the first implementation so after refactoring I
>decided to try again without extra byte and it failed for me.
>
OK.
Please update doxygen comment. NULL evoked to me NULL pointer ((void *)0)

>>
>>>+ * @param[in]  data_len         Length of memory data
>>>+ *                              not including terminating NULL.
>>>+ * @param[out] file_ctx         Configuration file object.
>>>+ *
>>>+ * @return 0 - Success.
>>>+ * @return EINVAL - Invalid parameter.
>>>+ * @return ENOMEM - No memory.
>>>+ */
>>>+int ini_config_file_from_mem(void *data_buf,
>>>+                             uint32_t data_len,
>>>+                             struct ini_cfgfile **file_ctx);
>>>+
>>>+/**
>>>  * @brief Close configuration file after parsing
>>>  *
>>>  * Closes file but keeps the context. File can be reopened
>>>diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
>>>index d87956a..fbb1a81 100644
>>>--- a/ini/ini_fileobj.c
>>>+++ b/ini/ini_fileobj.c
>>>@@ -46,6 +46,12 @@ enum index_utf_t {
>>>     INDEX_UTF8 = 4
>>>};
>>>
>>>+/* Trick to work around the fact that
>>>+ * fmemopen failes if buffer is 0 length
>>>+ */
>>>+unsigned char alt_buffer[2] = {0, 0};
>>                                 {'\n', 0};
>>It would be better to have string with length 1.
>>(gdb) print strlen("\n")
>>$1 = 1
>
>I do not like this approach "\n" is not one symbol on other platforms.
>I do not see a reason why this should be changed.
>
The idea was tu use "string" with length 1. But I realised it is not necessary.

>>
>>>+uint32_t alt_buffer_len = 1;
>>Could you use static modifier (both variable)?
>>
>>sh-4.2$ nm ini/ini_fileobj.o | grep " D "
>>0000000000000004 D alt_buffer
>>0000000000000000 D alt_buffer_len
>>
>>It is not big problem because symbols will be hidden by version-script file.
>>But It it will be better to hide them explicitly.
>
>I am concerned that if I will declare them as such they will go out
>of scope under the fmemopen handle.
They won't be out of scope. These variables are used in internal (static)
function common_file_init. It is implementation detail and it should be hidden.

>I do not trust this function much. I am not sure what it does under
>the hood so I would rather leave the buffer potentially accessible
>from other modules.
>But may be I am missing something?
>
Yhe only difference is in visibility of variable between modules
and there is no reason for this.

>>
>>>/* Close file but not destroy the object */
>>>void ini_config_file_close(struct ini_cfgfile *file_ctx)
>>>{
>>>@@ -138,7 +144,7 @@ static enum index_utf_t check_bom(enum index_utf_t ind,
>>>     return ind;
>>>}
>>>
>>>-static int read_chunk(int raw_file, size_t left, size_t increment,
>>>+static int read_chunk(FILE *file, size_t left, size_t increment,
>>>                       char *position, size_t *read_num)
>>>{
>>>     int error = EOK;
>>>@@ -149,22 +155,28 @@ static int read_chunk(int raw_file, size_t left, size_t increment,
>>>
>>>     to_read = how_much_to_read(left, increment);
>>>
>>>-    TRACE_INFO_NUMBER("About to read", to_read);
>>>-    errno = 0;
>>>-    read_cnt = read(raw_file, position, to_read);
>>>-    if (read_cnt == -1) {
>>>-        error = errno;
>>>-        TRACE_ERROR_NUMBER("Failed to read data from file", error);
>>>-        return error;
>>>-    }
>>>+    TRACE_INFO_NUMBER("About to read", (unsigned int)to_read);
>>                                          ^^^^^^^^^^^^^^
>>                                 casting is not necessary in this case.
>>                         because in macro TRACE_NUMBER variable is casted to
>>                                       ((unsigned long int))
>
>OK, I will fix that
>
>
>>>-    if (read_cnt != to_read) {
>>>-        error = EIO;
>>>-        TRACE_ERROR_NUMBER("Read less than required", error);
>>>-        return error;
>>>+    Read_cnt = fread(position, to_read, 1, file);
>>>+
>>>+    TRACE_INFO_NUMBER("Read", (unsigned int)(read_cnt * to_read));
>>                                 ^^^^^^^^^^^^^^
>>                                 the same here.
>
>And that.
>
>>>+
>>>+    if (read_cnt == 0) {
>>>+        error = ferror(file);
>>>+        if (error) {
>>>+            TRACE_ERROR_NUMBER("Failed to read data from file", error);
>>>+            return error;
>>>+        }
>>>+        error = feof(file);
>>>+        if(error) {
>>>+            TRACE_FLOW_EXIT();
>>>+            return EOK;
>>>+        }
>>It would be better to call feof before ferror.
>
>
>Why?
>If there is an error why would we first check if we reached the end of file?
>What you suggest is against the pattern: if not clear whether it is
>an error or not first check error and only if no error then check
>other things.
>I do not see a reason to change this.
>
I thought about situation when ferrror returns non-zero value and in the same
time feof returns end of file. I could not find anything in manual pages. if it
is possible.

>>
>>>+        TRACE_ERROR_NUMBER("Failed to read data from file", EIO);
>>>+        return EIO;
>>>     }
>>>
>>>-    *read_num = read_cnt;
>>>+    *read_num = to_read;
>>>
>>>     TRACE_FLOW_EXIT();
>>>     return error;
>>>@@ -229,7 +241,9 @@ static int initialize_conv(unsigned char *read_buf,
>>>}
>>>
>>>/* Internal conversion part */
>>>-static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>+static int common_file_convert(FILE *file,
>>>+                               struct ini_cfgfile *file_ctx,
>>>+                               uint32_t size)
>>>{
>>>     int error = EOK;
>>>     size_t read_cnt = 0;
>>>@@ -249,8 +263,8 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>
>>>     do {
>>>         /* print_buffer(read_buf, ICONV_BUFFER); */
>>>-        error = read_chunk(raw_file,
>>>-                           file_ctx->file_stats.st_size - total_read,
>>>+        error = read_chunk(file,
>>>+                           size - total_read,
>>>                            ICONV_BUFFER - in_buffer,
>>>                            read_buf + in_buffer,
>>>                            &read_cnt);
>>>@@ -305,9 +319,8 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>                     /* We need to just read more if we can */
>>>                     TRACE_INFO_NUMBER("Incomplete sequence len",
>>>                                       src - read_buf);
>>>-                    TRACE_INFO_NUMBER("File size.",
>>>-                                        file_ctx->file_stats.st_size);
>>>-                    if (total_read == file_ctx->file_stats.st_size) {
>>>+                    TRACE_INFO_NUMBER("File size.", size);
>>>+                    if (total_read == size) {
>>>                         /* Or return error if we can't */
>>>                         TRACE_ERROR_NUMBER("Incomplete sequence", error);
>>>                         iconv_close(conv);
>>>@@ -356,7 +369,7 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>         }
>>>         while (1);
>>>     }
>>>-    while (total_read<  file_ctx->file_stats.st_size);
>>>+    while (total_read<  size);
>>>
>>>     iconv_close(conv);
>>>
>>>@@ -365,8 +378,7 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>                       (char *)simplebuffer_get_vbuf(file_ctx->file_data));
>>>     TRACE_INFO_NUMBER("File len",
>>>                       simplebuffer_get_len(file_ctx->file_data));
>>>-    TRACE_INFO_NUMBER("Size",
>>>-                      file_ctx->file_data->size);
>>>+    TRACE_INFO_NUMBER("Size", size);
>>>     errno = 0;
>>>     file_ctx->file = fmemopen(simplebuffer_get_vbuf(file_ctx->file_data),
>>>                               simplebuffer_get_len(file_ctx->file_data),
>>>@@ -383,59 +395,98 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>>>
>>>
>>>/* Internal common initialization part */
>>>-static int common_file_init(struct ini_cfgfile *file_ctx)
>>>+static int common_file_init(struct ini_cfgfile *file_ctx,
>>>+                            void *data_buf,
>>>+                            uint32_t data_len)
>>>{
>>>     int error = EOK;
>>>-    int raw_file = 0;
>>>+    FILE *file = NULL;
>>>     int stat_ret = 0;
>>>+    uint32_t size = 0;
>>>+    void *internal_data = NULL;
>>>+    uint32_t internal_len = 0;
>>>
>>>     TRACE_FLOW_ENTRY();
>>>
>>>-    TRACE_INFO_STRING("File", file_ctx->filename);
>>>+    if (data_buf) {
>>>
>>>-    /* Open file in binary mode first */
>>>-    errno = 0;
>>>-    raw_file = open(file_ctx->filename, O_RDONLY);
>>>-    if (raw_file == -1) {
>>>-        error = errno;
>>>-        TRACE_ERROR_NUMBER("Failed to open file in binary mode", error);
>>>-        return error;
>>>+        if(data_len) {
>>>+            internal_data = data_buf;
>>>+            internal_len = data_len;
>>>+        }
>>>+        else {
>>>+            /* If buffer is empty fmemopen will return an error.
>>>+             * This will prevent creation of adefault config object.
>>>+             * Instead we will use buffer that has at least one character. */
>>>+            internal_data = (void *)alt_buffer;
>>                               ^^^^^^^^
>>                        Any pointer can be stored to variable with type "void *"
>>                        without explicit casting.
>
>OK, I will change that.
>
>>
>>>+            internal_len = alt_buffer_len;
>>>+        }
>>>+
>>>+        TRACE_INFO_NUMBER("Inside file_init len", internal_len);
>>>+        TRACE_INFO_STRING("Inside file_init data:", (char *)internal_data);
>>>+
>>>+        file = fmemopen(internal_data, internal_len, "r");
>>>+        if (!file) {
>>>+            error = errno;
>>>+            TRACE_ERROR_NUMBER("Failed to memmap file", error);
>>>+            return error;
>>>+        }
>>>+        size = internal_len;
>>>     }
>>>+    else {
>>>
>>>-    /* Get the size of the file */
>>>-    errno = 0;
>>>-    stat_ret = fstat(raw_file,&(file_ctx->file_stats));
>>>-    if (stat_ret == -1) {
>>>-        error = errno;
>>>-        close(raw_file);
>>>-        TRACE_ERROR_NUMBER("Failed to get file stats", error);
>>>-        return error;
>>>+        TRACE_INFO_STRING("File", file_ctx->filename);
>>>+
>>>+        /* Open file to get its size */
>>>+        errno = 0;
>>>+        file = fopen(file_ctx->filename, "r");
>>>+        if (!file) {
>>>+            error = errno;
>>>+            TRACE_ERROR_NUMBER("Failed to open file", error);
>>>+            return error;
>>>+        }
>>>+
>>>+        /* Get the size of the file */
>>>+        errno = 0;
>>>+        stat_ret = fstat(fileno(file),&(file_ctx->file_stats));
>>>+        if (stat_ret == -1) {
>>>+            error = errno;
>>>+            fclose(file);
>>>+            TRACE_ERROR_NUMBER("Failed to get file stats", error);
>>>+            return error;
>>>+        }
>>>+        size = file_ctx->file_stats.st_size;
>>>     }
>>>
>>>     /* Trick to overcome the fact that
>>>      * fopen and fmemopen behave differently when file
>>>      * is 0 length
>>>      */
>>>-    if (file_ctx->file_stats.st_size) {
>>>-        error = common_file_convert(raw_file, file_ctx);
>>>-        close(raw_file);
>>>+    if (size) {
>>>+        error = common_file_convert(file, file_ctx, size);
>>>         if (error) {
>>>             TRACE_ERROR_NUMBER("Failed to convert file",
>>>                                 error);
>>>+            fclose(file);
>>>             return error;
>>>         }
>>>     }
>>>     else {
>>>+
>>>+        TRACE_INFO_STRING("File is 0 length","");
>>>         errno = 0;
>>>-        file_ctx->file = fdopen(raw_file, "r");
>>>+
>>>+        file_ctx->file = fdopen(fileno(file), "r");
>>>         if (!(file_ctx->file)) {
>>>             error = errno;
>>>-            close(raw_file);
>>>+            fclose(file);
>>>             TRACE_ERROR_NUMBER("Failed to fdopen file", error);
>>>             return error;
>>>         }
>>>     }
>>>
>>>+    fclose(file);
>>>+
>>>     /* Collect stats */
>>>     if (file_ctx->metadata_flags&  INI_META_STATS) {
>>>         file_ctx->stats_read = 1;
>>>@@ -505,7 +556,7 @@ int ini_config_file_open(const char *filename,
>>>     }
>>>
>>>     /* Do common init */
>>>-    error = common_file_init(new_ctx);
>>>+    error = common_file_init(new_ctx, NULL, 0);
>>>     if(error) {
>>>         TRACE_ERROR_NUMBER("Failed to do common init", error);
>>>         ini_config_file_destroy(new_ctx);
>>>@@ -517,6 +568,63 @@ int ini_config_file_open(const char *filename,
>>>     return error;
>>>}
>>>
>>>+/* Create a file object from a memory buffer */
>>>+int ini_config_file_from_mem(void *data_buf,
>>>+                             uint32_t data_len,
>>>+                             struct ini_cfgfile **file_ctx)
>>>+{
>>>+    int error = EOK;
>>>+    struct ini_cfgfile *new_ctx = NULL;
>>>+
>>>+    TRACE_FLOW_ENTRY();
>>>+
>>>+    if ((!data_buf) || (!file_ctx)) {
>>>+        TRACE_ERROR_NUMBER("Invalid parameter.", EINVAL);
>>>+        return EINVAL;
>>>+    }
>>>+
>>>+    /* Allocate structure */
>>>+    new_ctx = malloc(sizeof(struct ini_cfgfile));
>>>+    if (!new_ctx) {
>>>+        TRACE_ERROR_NUMBER("Failed to allocate file ctx.", ENOMEM);
>>>+        return ENOMEM;
>>>+    }
>>>+
>>>+    new_ctx->filename = NULL;
>>>+    new_ctx->file = NULL;
>>>+    new_ctx->file_data = NULL;
>>>+    new_ctx->metadata_flags = 0;
>>Could we use calloc instead of malloc.
>
>No. :-)
>
>>Memory will be set to zero autmatically. We could remove these lines in this
>>case.
>
>I know. We had this conversation at the beginning of ding-libs. I use
>malloc and initialize variables one by one and check with Valgrind
>that everything is initialized.
>This is a preference and style.
>
good to know.

>>
>>>+
>>>+    error = simplebuffer_alloc(&(new_ctx->file_data));
>>>+    if (error) {
>>>+        TRACE_ERROR_NUMBER("Failed to allocate buffer ctx.", error);
>>>+        ini_config_file_destroy(new_ctx);
>>>+        return error;
>>>+    }
>>>+
>>>+    /* Put an empty string into the file name */
>>>+    new_ctx->filename = strdup("");
>>>+    if (!(new_ctx->filename)) {
>>>+        ini_config_file_destroy(new_ctx);
>>>+        TRACE_ERROR_NUMBER("Failed to put empty string into filename.", ENOMEM);
>>>+        return ENOMEM;
>>>+    }
>>>+
>>>+    /* Do common init */
>>>+    error = common_file_init(new_ctx, data_buf, data_len);
>>>+    if(error) {
>>>+        TRACE_ERROR_NUMBER("Failed to do common init", error);
>>>+        ini_config_file_destroy(new_ctx);
>>>+        return error;
>>>+    }
>>>+
>>>+    *file_ctx = new_ctx;
>>>+    TRACE_FLOW_EXIT();
>>>+    return error;
>>>+}
>>>+
>>>+
>>>+
>>>/* Create a file object from existing one */
>>>int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
>>>                            struct ini_cfgfile **file_ctx_out)
>>>@@ -564,7 +672,7 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
>>>     }
>>>
>>>     /* Do common init */
>>>-    error = common_file_init(new_ctx);
>>>+    error = common_file_init(new_ctx, NULL, 0);
>>>     if(error) {
>>>         TRACE_ERROR_NUMBER("Failed to do common init", error);
>>>         ini_config_file_destroy(new_ctx);
>>>diff --git a/ini/libini_config.sym b/ini/libini_config.sym
>>>index d38b17b..d0035f2 100644
>>>--- a/ini/libini_config.sym
>>>+++ b/ini/libini_config.sym
>>>@@ -46,6 +46,7 @@ global:
>>>     ini_config_destroy;
>>>     ini_config_clean_state;
>>>     ini_config_file_open;
>>>+    ini_config_file_from_mem;
>>>     ini_config_file_close;
>>>     ini_config_file_reopen;
>>>     ini_config_file_destroy;
>>>-- 
>>>1.7.1
>>>
>>> From 16c9140ada8461250d2aa8ac484e29c5237fdeaa Mon Sep 17 00:00:00 2001
>>>From: Dmitri Pal<dpal at redhat.com>
>>>Date: Sat, 8 Mar 2014 19:04:02 -0500
>>>Subject: [PATCH 4/4] [INI] Unit test for the new interface
>>>
>>>This patch adds unit test for the interface that allows
>>>reading configuration data from memory.
>>>---
>>>ini/ini_parse_ut.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>>1 files changed, 140 insertions(+), 14 deletions(-)
>>>
>>>diff --git a/ini/ini_parse_ut.c b/ini/ini_parse_ut.c
>>>index 1fa2624..721ee4a 100644
>>>--- a/ini/ini_parse_ut.c
>>>+++ b/ini/ini_parse_ut.c
>>>@@ -50,7 +50,8 @@ typedef int (*test_fn)(void);
>>>
>>>static int test_one_file(const char *in_filename,
>>>                          const char *out_filename,
>>>-                         int edge)
>>>+                         int edge,
>>>+                         int mem)
>>     I think type bool would be better. For example: bool from_memory
>>     In my opinion, it is easier to read code: "if (from_memory)"
>>     type bool is just a cosmetic change. (it needn't be changed)
>
>I do not use "bools" anywhere in the code. Just a tradition from the
>pre bool time.
On the other hand, we use uin32_t type in code and it was defined in the
standard c99 (bool as well)

>I do not see a reason to start using it now.
>
At least use different name because "if (mem)" looks like varaible mem is some
kind of buffer (void* mem)


>>
>>
>>>{
>>>     int error = EOK;
>>>     struct ini_cfgfile *file_ctx = NULL;
>>>@@ -60,6 +61,11 @@ static int test_one_file(const char *in_filename,
>>>     char **error_list = NULL;
>>>     struct simplebuffer *sbobj = NULL;
>>>     uint32_t left = 0;
>>>+    uint32_t stream_len = 0;
>>>+    void *stream_data = NULL;
>>>+    struct stat file_stats;
>>>+    int stat_ret = 0;
>>>+    FILE *file = NULL;
>>>
>>>     INIOUT(printf("<==== Testing file %s ====>\n", in_filename));
>>>
>>>@@ -70,14 +76,73 @@ static int test_one_file(const char *in_filename,
>>>         return error;
>>>     }
>>>
>>>-    error = ini_config_file_open(in_filename,
>>>-                                 0, /* TBD */
>>>-&file_ctx);
>>>-    if (error) {
>>>-        printf("Failed to open file %s for reading. Error %d.\n",
>>>-               in_filename, error);
>>>-        ini_config_destroy(ini_config);
>>>-        return error;
>>>+    if(mem) {
>>       ^^^
>>You forget to add space between "if" and the bracket.
>
>Will fix
>
>>
>>>+        /* Get file stats */
>>>+        errno = 0;
>>>+        stat_ret = stat(in_filename,&file_stats);
>>>+        if (stat_ret == -1) {
>>           ^^^^
>>         with space
>>>+            error = errno;
>>>+            printf("Failed to get file stats. Error %d.\n", error);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>+        /* Allocate memory to store file */
>>>+        errno = 0;
>>>+        stream_len = file_stats.st_size;
>>>+        stream_data = malloc(file_stats.st_size + 1);
>>>+        if(!stream_data) {
>>           ^^^^
>>         without space
>
>Will fix
>
>>>+            error = errno;
>>>+            printf("Failed to allocate memory for stream. Error %d.\n", error);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>+        *((unsigned char *)(stream_data) + stream_len) = '\0';
>>>+        /* Open file */
>>>+        errno = 0;
>>>+        file = fopen(in_filename, "r");
>>>+        if(!stream_data) {
>>           ^^^^
>>         without space
>
>
>Will fix
>
>>>+            error = errno;
>>>+            printf("Failed to open file to prepare stream. Error %d.\n", error);
>>>+            free(stream_data);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>+        /* Read file into memory */
>>>+        errno = 0;
>>>+        fread(stream_data, stream_len, 1, file);
>>>+        error = ferror(file);
>>>+        if(error) {
>>           ^^^^
>>         without space
>
>Will fix
>
>>>+            printf("Failed to read stream data. Error %d.\n", error);
>>>+            free(stream_data);
>>>+            fclose(file);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>+        fclose(file);
>>>+
>>>+        INIOUT(printf("Data len %u\n", stream_len));
>>>+        INIOUT(printf("Data:\n%s\n", (char *)stream_data));
>>>+
>>>+        error = ini_config_file_from_mem(stream_data,
>>>+                                         stream_len,
>>>+&file_ctx);
>>>+        if (error) {
>>>+            printf("Failed to open from memory. Error %d.\n", error);
>>>+            free(stream_data);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>+    }
>>>+    else {
>>>+        error = ini_config_file_open(in_filename,
>>>+                                     0, /* TBD */
>>>+&file_ctx);
>>>+        if (error) {
>>>+            printf("Failed to open file %s for reading. Error %d.\n",
>>>+                   in_filename, error);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>     }
>>>
>>>     error = ini_config_parse(file_ctx,
>>>@@ -89,15 +154,29 @@ static int test_one_file(const char *in_filename,
>>>         INIOUT(printf("Failed to parse configuration. Error %d.\n", error));
>>>
>>>         if (ini_config_error_count(ini_config)) {
>>>-            INIOUT(printf("Errors detected while parsing: %s\n",
>>>-                   ini_config_get_filename(file_ctx)));
>>>+            if (mem) {
>>>+                INIOUT(printf("Errors detected while parsing"
>>>+                              " configuration stored in memory: %s\n",
>>>+                              in_filename));
>>>+            }
>>>+            else {
>>>+                INIOUT(printf("Errors detected while parsing: %s\n",
>>>+                        ini_config_get_filename(file_ctx)));
>>>+            }
>>>             ini_config_get_errors(ini_config,&error_list);
>>>             INIOUT(ini_config_print_errors(stdout, error_list));
>>>             ini_config_free_errors(error_list);
>>>         }
>>>-        /* We do not return here intentionally */
>>>+        /* If we are testing memory mapped, return error */
>>>+        if (mem) {
>>>+            free(stream_data);
>>>+            ini_config_file_destroy(file_ctx);
>>>+            ini_config_destroy(ini_config);
>>>+            return error;
>>>+        }
>>>     }
>>>
>>>+    if (mem) free(stream_data);
>>>     ini_config_file_destroy(file_ctx);
>>>
>>>     INIOUT(col_debug_collection(ini_config->cfg, COL_TRAVERSE_DEFAULT));
>>>@@ -200,8 +279,9 @@ static int read_save_test(void)
>>>             snprintf(infile, PATH_MAX, "%s/ini/ini.d/%s.conf",
>>>                     (srcdir == NULL) ? "." : srcdir, files[i]);
>>>             snprintf(outfile, PATH_MAX, "./%s_%d.conf.out", files[i], edge);
>>>-            error = test_one_file(infile, outfile, edge);
>>>+            error = test_one_file(infile, outfile, edge, 0);
>>>             INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>>>+            if (error) return error;
>>>         }
>>>         i++;
>>>     }
>>>@@ -211,6 +291,51 @@ static int read_save_test(void)
>>>     return EOK;
>>>}
>>>
>>>+
>>>+/* Run tests for multiple files */
>>>+static int read_mem_test(void)
>>>+{
>>>+    int error = EOK;
>>>+    int i = 0;
>>>+    int edge = 5;
>>>+    char infile[PATH_MAX];
>>>+    char outfile[PATH_MAX];
>>>+    char *srcdir = NULL;
>>>+    const char *files[] = { "real",
>>>+                            "mysssd",
>>>+                            "ipa",
>>>+                            "test",
>>>+                            "smerge",
>>>+                            "real8",
>>>+                            "real16be",
>>>+                            "real16le",
>>>+                            "real32be",
>>>+                            "real32le",
>>>+                            "symbols",
>>I am attaching two files: empty.conf (size is zero)
>>                           new_line.conf (size is one and contains only '\n'
>>Could you add them to tests?
>
>The empty file is generated on the fly as a result of one of the
>read-write-read-write operation becuase first read fails and there is
>an emty config that ends up with a file of 0 size. So that is
>covered.
>I can add new_line.conf.
>
The intention was to test the corner case.
    error = ini_config_file_from_mem(stream_data,
                                     0,
                                     &file_ctx);
And it is not tested. Testing empty file would be the easiest way.
(I tried it and test passed)

>>
>>>+                            NULL };
>>>+
>>>+    INIOUT(printf("<==== Read mem test ====>\n"));
>>>+
>>>+    srcdir = getenv("srcdir");
>>>+
>>>+    while(files[i]) {
>>>+        for ( edge = 15; edge<  100; edge +=25) {
>>>+            snprintf(infile, PATH_MAX, "%s/ini/ini.d/%s.conf",
>>>+                    (srcdir == NULL) ? "." : srcdir, files[i]);
>>>+            snprintf(outfile, PATH_MAX, "./%s_%d.conf.mem.out", files[i], edge);
>>>+            error = test_one_file(infile, outfile, edge, 1);
>>>+            INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>>>+            if ((error)&&  (strncmp(files[i], "test", 4) != 0)) return error;
>>>+        }
>>>+        i++;
>>>+    }
>>>+
>>>+    INIOUT(printf("<==== Read mem test end ====>\n"));
>>>+
>>>+    return EOK;
>>>+}
>>>+
>>>+
>>>/* Run tests for multiple files */
>>>static int read_again_test(void)
>>>{
>>>@@ -241,7 +366,7 @@ static int read_again_test(void)
>>>             snprintf(infile, PATH_MAX, "./%s_%d.conf.out", files[i], edge);
>>>             snprintf(outfile, PATH_MAX, "./%s_%d.conf.2.out", files[i], edge);
>>>
>>>-            error = test_one_file(infile, outfile, edge);
>>>+            error = test_one_file(infile, outfile, edge, 0);
>>>             INIOUT(printf("Test for file: %s returned %d\n", files[i], error));
>>>             if (error) break;
>>>             snprintf(command, PATH_MAX * 3, "diff -q %s %s", infile, outfile);
>>>@@ -3058,6 +3183,7 @@ int main(int argc, char *argv[])
>>>     int error = 0;
>>>     test_fn tests[] = { read_save_test,
>>>                         read_again_test,
>>>+                        read_mem_test,
>>>                         merge_values_test,
>>>                         merge_section_test,
>>>                         merge_file_test,
>>>-- 
>>>1.7.1
>>>
>>>From: Dmitri Pal<dpal at redhat.com>
>>>Date: Sat, 8 Mar 2014 19:02:29 -0500
>>>Subject: [PATCH 3/4] [INI] Prevent tight loop
>>>
>>>New functionality added revealed a potential tight loop
>>>with files of the 0 length. This patch addresses this issue.
>>>---
>>>ini/ini_parse.c |   24 +++++++++++++++---------
>>>1 files changed, 15 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/ini/ini_parse.c b/ini/ini_parse.c
>>>index 7792324..0bab822 100644
>>>--- a/ini/ini_parse.c
>>>+++ b/ini/ini_parse.c
>>>@@ -1534,16 +1534,22 @@ static int parser_error(struct parser_obj *po)
>>>         else po->ret = EIO;
>>>     }
>>>     else if (po->error_level == INI_STOP_ON_NONE) {
>>>-        action = PARSE_READ;
>>>-        if (po->ret == 0) {
>>>-            if (po->last_error&  INI_WARNING) po->ret = EILSEQ;
>>>-            else po->ret = EIO;
>>>+        if (po->last_error != ERR_READ) {
>>>+            action = PARSE_READ;
>>>+            if (po->ret == 0) {
>>>+                if (po->last_error&  INI_WARNING) po->ret = EILSEQ;
>>>+                else po->ret = EIO;
>>>+            }
>>>+            /* It it was warning but now if it is an error
>>>+             * bump to return code to indicate error. */
>>>+            else if((po->ret == EILSEQ)&&
>>>+                    (!(po->last_error&  INI_WARNING))) po->ret = EIO;
>>>+        }
>>>+        else {
>>>+            /* Avoid endless loop */
>>>+            action = PARSE_DONE;
>>>+            po->ret = EIO;
>>>         }
>>>-        /* It it was warning but now if it is an error
>>>-         * bump to return code to indicate error. */
>>>-        else if((po->ret == EILSEQ)&&
>>>-                (!(po->last_error&  INI_WARNING))) po->ret = EIO;
>>>-
>>>     }
>>>     else { /* Stop on error */
>>>         if (po->last_error&  INI_WARNING) {
>>ACK to this patch.
>>
>>LS
>
>Thanks for the review.
>

LS



More information about the sssd-devel mailing list