[SSSD] [INI][PATCHES] All of them (C-Style Comments, Unicode conversion, Space processing, UNIT tests)

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 6 17:48:20 UTC 2013


On (19/10/13 16:44), Dmitri Pal wrote:
>Hello,
>
>This mail contains all the patches that I have so far in my git.
>
>--------------------------
>
>The following set of patches is related to comments and unchanged
>from the previous submission.
>I replied to the comments about them in another mail.
>I do not think there is anything to address.
>If there is please let me know.
>
>0001-INI-Do-not-check-validity-of-comments.patch
>0002-INI-Extend-error-set-and-add-parsing-error.patch
>0003-INI-Process-c-style-comments.patch
>0004-INI-Test-files-for-unit-test.patch
>0005-INI-Unit-test-for-c-style-comments.patch
>
>--------------------------
>
>The following set of patches addresses comments and fixes
>bugs in Unicode/UTF8 conversion
>
>0006-INI-Expose-buffer-context-as-void.patch
>0007-INI-Extend-internal-file-handle.patch
>0008-INI-Convert-files-to-UTF.patch
>0009-INI-Updated-unit-test-for-UTF8-conversion.patch
>
>Thanks for review Lukas. I reworked the code significantly.
>I think I addressed everything except the initialization comment.
>I agree with you that it would have been better to
>initialize things outside the loop but I need to read the first buffer
>to be able to determine the encoding. This is what causes this logic.
>I do not see a way to avoid it so left that part as is.
>
>----------------------------
>
>The following one liner fixes a trace text
>0010-INI-Fix-typo-in-comment.patch
>
>----------------------------
>
>The following patches are related to the problem I noticed earlier
>and recorded in the ticket https://fedorahosted.org/sssd/ticket/2119
>But in reality the cause was  bug in the Unicode conversion
>and a bug with space processing.
>
>0011-INI-Fix-processing-of-the-white-space-at-the-end-of-.patch
>0012-INI-Unit-test-for-space-trimming-in-multiline-values.patch
>
>----------------------------
>
>The following patch adds more unit test for both Unicode conversion
>and folding by combining the tests together. The unit tests written
>for each of them individually are not sufficient to catch corner cases.
>This test does. And I played with different constants before checking
>in to make sure that all sorts of combinations are caught.
>I decided to settle on these numbers because they provide
>sufficient amount of testing but do not extend the time too much.
>Right now all make check tests take about 7-8 seconds on my laptop.
>
>0013-INI-Adding-more-unit-tests.patch
>
>
>-- 
>Thank you,
>Dmitri Pal
>
>Sr. Engineering Manager for IdM portfolio
>Red Hat Inc.
>
>

I would like to appologize for late review.

0001-INI-Do-not-check-validity-of-comments.patch
    ACK
0002-INI-Extend-error-set-and-add-parsing-error.patch
    ACK
0003-INI-Process-c-style-comments.patch
    ACK
0004-INI-Test-files-for-unit-test.patch
    ACK
0005-INI-Unit-test-for-c-style-comments.patch
make distcheck failed. Please add new file into dist_noinst_DATA
into Makefile.am
dist_noinst_DATA +=
    ini/ini.d/comment.conf
0006-INI-Expose-buffer-context-as-void.patch
    ACK
0007-INI-Extend-internal-file-handle.patch
    ACK
0008-INI-Convert-files-to-UTF.patch
    comments are inlined for this patch.
0009-INI-Updated-unit-test-for-UTF8-conversion.patch
make distcheck failed. Please add new file into dist_noinst_DATA
into Makefile.am
dist_noinst_DATA +=
    ini/ini.d/real8.conf \
    ini/ini.d/real16be.conf \
    ini/ini.d/real16le.conf \
    ini/ini.d/real32be.conf \
    ini/ini.d/real32le.conf

{./bom3 ./bom4le ./bom4be ./bom2le ./bom2be}
"bom*" files are created in testini_parse_ut (function create_bom)
Those files should be removed in test (remove_bom_files???)
or in Makefile.am

 clean-local-ini_config:
        rm -f ./*.out
        rm -f test.ini
-       rm -f ./foo.conf #From ini_parse_ut
+       rm -f ./foo.conf ./bom* #From ini_parse_ut

0010-INI-Fix-typo-in-comment.patch
    little nitpick, please change commit message
    s/comment/log message/
0011-INI-Fix-processing-of-the-white-space-at-the-end-of-.patch
    ACK
0012-INI-Unit-test-for-space-trimming-in-multiline-values.patch
    ACK
0013-INI-Adding-more-unit-tests.patch
make distcheck failed. Please add new file into dist_noinst_DATA
into Makefile.am
dist_noinst_DATA +=
    ini/ini.d/symbols.conf

>From 76ab33c0462cb01de700e40b9cc86fa46269fa85 Mon Sep 17 00:00:00 2001
>From: Dmitri Pal <dpal at redhat.com>
>Date: Thu, 10 Oct 2013 22:47:23 -0400
>Subject: [PATCH 08/13] [INI] Convert files to UTF
>
>Patch adds functionality that detects BOM
>at the beginning of the file and uses
>it to convert to UTF8.
>If no BOM is detected file is assumed to be UTF8
>encoded.
>---
> ini/ini_fileobj.c |  368 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 356 insertions(+), 12 deletions(-)
>
>diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
>index 7ac9dc6..d7a42ed 100644
>--- a/ini/ini_fileobj.c
>+++ b/ini/ini_fileobj.c
>@@ -18,17 +18,33 @@
>     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/>.
> */
>-
> #include "config.h"
> #include <errno.h>
>+#include <sys/types.h>
>+#include <sys/stat.h>
>+#include <fcntl.h>
> #include <string.h>
> #include <stdlib.h>
>+#include <iconv.h>
> #include "trace.h"
> #include "ini_defines.h"
> #include "ini_configobj.h"
> #include "ini_config_priv.h"
> #include "path_utils.h"
> 
>+#define ICONV_BUFFER    5000
>+
>+#define BOM4_SIZE 4
>+#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
>+};
> 
> /* Close file but not destroy the object */
> void ini_config_file_close(struct ini_cfgfile *file_ctx)
>@@ -52,6 +68,7 @@ void ini_config_file_destroy(struct ini_cfgfile *file_ctx)
> 
>     if(file_ctx) {
>         free(file_ctx->filename);
>+        simplebuffer_free(file_ctx->file_data);
>         if(file_ctx->file) fclose(file_ctx->file);
>         free(file_ctx);
>     }
>@@ -59,37 +76,345 @@ void ini_config_file_destroy(struct ini_cfgfile *file_ctx)
>     TRACE_FLOW_EXIT();
> }
> 
>-/* Internal common initialization part */
>-static int common_file_init(struct ini_cfgfile *file_ctx)
>+/* How much I plan to read? */
>+size_t how_much_to_read(size_t left, size_t increment)
         ^^^^^^^^^^^^^^^^
Internal function without "static modifier"
>+{
>+    if(left > increment) return increment;
>+    else return left;
>+}
>+
>+enum index_utf_t check_bom(enum index_utf_t ind,
                   ^^^^^^^^^
                 Internal function without "static modifier"
>+                           unsigned char *buffer,
>+                           size_t len,
>+                           size_t *bom_shift)
                                    ^^^
I am not sure about code style, but I think output name of parameters should
start with underscore "_bom_shift"

>+{
>+    TRACE_FLOW_ENTRY();
>+
>+    if (len > BOM4_SIZE) {
             ^^
             ">="
>+        if ((buffer[0] == 0x00) &&
>+            (buffer[1] == 0x00) &&
>+            (buffer[2] == 0xFE) &&
>+            (buffer[3] == 0xFF)) {
>+                TRACE_FLOW_RETURN(INDEX_UTF32BE);
>+                *bom_shift = BOM4_SIZE;
>+                return INDEX_UTF32BE;
>+        }
>+        else if ((buffer[0] == 0xFF) &&
>+                 (buffer[1] == 0xFE) &&
>+                 (buffer[2] == 0x00) &&
>+                 (buffer[3] == 0x00)) {
>+                TRACE_FLOW_RETURN(INDEX_UTF32LE);
>+                *bom_shift = BOM4_SIZE;
>+                return INDEX_UTF32LE;
>+        }
>+    }
>+
>+    if (len > BOM3_SIZE) {
             ^^
             ">="
>+        if ((buffer[0] == 0xEF) &&
>+            (buffer[1] == 0xBB) &&
>+            (buffer[2] == 0xBF)) {
>+                TRACE_FLOW_RETURN(INDEX_UTF8);
>+                *bom_shift = BOM3_SIZE;
>+                return INDEX_UTF8;
>+        }
>+    }
>+
>+    if (len > BOM2_SIZE) {
             ^^
             ">="
>+        if ((buffer[0] == 0xFE) &&
>+            (buffer[1] == 0xFF)) {
>+                TRACE_FLOW_RETURN(INDEX_UTF16BE);
>+                *bom_shift = BOM2_SIZE;
>+                return INDEX_UTF16BE;
>+        }
>+        else if ((buffer[0] == 0xFF) &&
>+                 (buffer[1] == 0xFE)) {
>+                TRACE_FLOW_RETURN(INDEX_UTF16LE);
>+                *bom_shift = BOM2_SIZE;
>+                return INDEX_UTF16LE;
>+        }
>+    }
>+
>+    TRACE_FLOW_RETURN(ind);
>+    return ind;
>+}
>+
>+static int read_chunk(int raw_file, size_t left, size_t increment,
>+                      char *position, size_t *read_num)
> {
>     int error = EOK;
>+    size_t to_read = 0;
>+    size_t read_cnt = 0;
> 
>     TRACE_FLOW_ENTRY();
> 
>+    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;
>+    }
>+
>+    if (read_cnt != to_read) {
>+        error = EIO;
>+        TRACE_ERROR_NUMBER("Read less than required", error);
>+        return error;
>+    }
>+
>+    *read_num = read_cnt;
>+
>+    TRACE_FLOW_EXIT();
>+    return error;
>+}
>+
>+/* Function useful for debugging */
>+/*
>+void print_buffer(char *read_buffer, int len)
>+{
>+    int i;
>+    for (i=0; i < len; i++) {
>+        printf("%02X ", (unsigned char)read_buffer[i]);
>+    }
>+    printf("\n");
>+}
>+*/
>+
>+/* Internal conversion part */
>+static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>+{
>+    int error = EOK;
>+    size_t read_cnt = 0;
>+    size_t total_read = 0;
>+    size_t in_buffer = 0;
>+    enum index_utf_t ind = INDEX_UTF8;
>+    iconv_t conv = (iconv_t)-1;
>+    size_t conv_res = 0;
>+    char read_buf[ICONV_BUFFER+1];
>+    char result_buf[ICONV_BUFFER];
>+    char *src, *dest;
>+    size_t to_convert = 0;
>+    size_t room_left = 0;
>+    size_t bom_shift = 0;
>+    const char *encodings[] = {  "UTF-32BE",
>+                                 "UTF-32LE",
>+                                 "UTF-16BE",
>+                                 "UTF-16LE",
>+                                 "UTF-8" };
>+
>+    TRACE_FLOW_ENTRY();
>+
>+    do {
>+        /* print_buffer(read_buf, ICONV_BUFFER); */
>+        error = read_chunk(raw_file,
>+                           file_ctx->file_stats.st_size - total_read,
>+                           ICONV_BUFFER - in_buffer,
>+                           read_buf + in_buffer,
>+                           &read_cnt);
>+        /* print_buffer(read_buf, ICONV_BUFFER); */
>+        if (error) {
>+            if (conv != (iconv_t) -1) iconv_close(conv);
>+            TRACE_ERROR_NUMBER("Failed to read chunk", error);
>+            return error;
>+        }
>+
>+        /* Prepare source buffer for conversion */
>+        src = read_buf;
>+        to_convert = read_cnt + in_buffer;
>+        in_buffer = 0;
>+
>+        /* First time do some initialization */
>+        if (total_read == 0) {
>+            TRACE_INFO_STRING("Reading first time.","Checking BOM");
>+
>+            ind = check_bom(ind,
>+                            (unsigned char *)read_buf,
>+                            read_cnt,
>+                            &bom_shift);
>+
>+            /* Skip BOM the first time */
>+            src += bom_shift;
>+            to_convert -= bom_shift;
>+
>+            TRACE_INFO_STRING("Converting to", encodings[INDEX_UTF8]);
>+            TRACE_INFO_STRING("Converting from", encodings[ind]);
>+
>+            errno = 0;
>+            conv = iconv_open(encodings[INDEX_UTF8], encodings[ind]);
              ^^^^^^^^
           Coverity warning RESOURCE_LEAK.
But I think it is a false positive, because total_read cannot be 0 on second
pass of loop. But I prepared patch with fix.
UTF-fix.patch is attached in this mail and can be squashed to this patch.
>+            if (conv == (iconv_t) -1) {
>+                error = errno;
>+                TRACE_ERROR_NUMBER("Failed to create converter", error);
>+                return error;
>+            }
>+        }
>+
>+        total_read += read_cnt;
>+        TRACE_INFO_NUMBER("Total read", total_read);
>+
>+        do {
>+            /* Do conversion */
>+            dest = result_buf;
>+            room_left = ICONV_BUFFER;
>+
>+            TRACE_INFO_NUMBER("To convert", to_convert);
>+            TRACE_INFO_NUMBER("Room left", room_left);
>+            TRACE_INFO_NUMBER("Total read", total_read);
>+
>+            errno = 0;
>+            conv_res = iconv(conv, &src, &to_convert, &dest, &room_left);
>+            if (conv_res == (size_t) -1) {
>+                error = errno;
>+                switch(error) {
>+                case EILSEQ:
>+                    TRACE_ERROR_NUMBER("Invalid multibyte encoding", error);
>+                    iconv_close(conv);
>+                    return error;
>+                case EINVAL:
>+                    /* 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) {
>+                        /* Or return error if we can't */
>+                        TRACE_ERROR_NUMBER("Incomplete sequence", error);
>+                        iconv_close(conv);
>+                        return error;
>+                    }
>+                    memmove(read_buf, src, to_convert);
>+                    in_buffer = to_convert;
>+                    break;
>+
>+                case E2BIG:
>+                    TRACE_INFO_STRING("No room in the output buffer.", "");
>+                    error = simplebuffer_add_raw(file_ctx->file_data,
>+                                                 result_buf,
>+                                                 ICONV_BUFFER - room_left,
>+                                                 ICONV_BUFFER);
>+                    if (error) {
>+                        TRACE_ERROR_NUMBER("Failed to store converted bytes",
>+                                            error);
>+                        iconv_close(conv);
>+                        return error;
>+                    }
>+                    continue;
>+                default:
>+                    TRACE_ERROR_NUMBER("Unexpected internal error",
>+                                        error);
>+                    iconv_close(conv);
>+                    return ENOTSUP;
>+                }
>+            }
>+            /* The whole buffer was sucessfully converted */
>+            error = simplebuffer_add_raw(file_ctx->file_data,
>+                                         result_buf,
>+                                         ICONV_BUFFER - room_left,
>+                                         ICONV_BUFFER);
>+            if (error) {
>+                TRACE_ERROR_NUMBER("Failed to store converted bytes",
>+                                    error);
>+                iconv_close(conv);
>+                return error;
>+            }
>+/*
>+            TRACE_INFO_STRING("Saved procesed portion.",
>+                        (char *)simplebuffer_get_vbuf(file_ctx->file_data));
>+*/
>+            break;
>+        }
>+        while (1);
>+    }
>+    while (total_read < file_ctx->file_stats.st_size);
>+
>+    iconv_close(conv);
>+
>     /* Open file */
>-    TRACE_INFO_STRING("File", file_ctx->filename);
>+    TRACE_INFO_STRING("File data",
>+                      (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);
>     errno = 0;
>-    file_ctx->file = fopen(file_ctx->filename, "r");
>+    file_ctx->file = fmemopen(simplebuffer_get_vbuf(file_ctx->file_data),
>+                              simplebuffer_get_len(file_ctx->file_data),
>+                              "r");
>     if (!(file_ctx->file)) {
>         error = errno;
>         TRACE_ERROR_NUMBER("Failed to open file", error);
>         return error;
>     }
> 
>-    file_ctx->stats_read = 0;
>+    TRACE_FLOW_EXIT();
>+    return EOK;
>+}
>+
> 
>-    /* Collect stats */
>-    if (file_ctx->metadata_flags & INI_META_STATS) {
>+/* Internal common initialization part */
>+static int common_file_init(struct ini_cfgfile *file_ctx)
>+{
>+    int error = EOK;
>+    int raw_file = 0;
>+    int stat_ret = 0;
>+
>+    TRACE_FLOW_ENTRY();
>+
>+    TRACE_INFO_STRING("File", file_ctx->filename);
>+
>+    /* 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;
>+    }
>+
>+    /* 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;
>+    }
>+
>+    /* 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);
File descriptor raw_file will not be used by any stream (FILE*),
because file_ctx->file is pointer to stream created from memory (fmemopen)

We should close file descriptor raw_file every time.

It was the second coverity warning "RESOURCE_LEAK"
>+        if (error) {
>+            TRACE_ERROR_NUMBER("Failed to convert file",
>+                                error);
>+            close(raw_file);
>+            return error;
>+        }
>+    }
>+    else {
>         errno = 0;
>-        if (fstat(fileno(file_ctx->file),
>-                  &(file_ctx->file_stats)) < 0) {
>+        file_ctx->file = fdopen(raw_file, "r");
>+        if (!(file_ctx->file)) {
>             error = errno;
>-            TRACE_ERROR_NUMBER("Failed to get file stats.", error);
>+            close(raw_file);
>+            TRACE_ERROR_NUMBER("Failed to fdopen file", error);
>             return error;
>         }
>+
>+    }
>+
>+    /* Collect stats */
>+    if (file_ctx->metadata_flags & INI_META_STATS) {
>         file_ctx->stats_read = 1;
>     }
>-    else memset(&(file_ctx->file_stats), 0, sizeof(struct stat));
>+    else {
>+        memset(&(file_ctx->file_stats), 0, sizeof(struct stat));
>+        file_ctx->stats_read = 0;
>+    }
> 
>     TRACE_FLOW_EXIT();
>     return EOK;
>@@ -119,6 +444,15 @@ int ini_config_file_open(const char *filename,
> 
>     new_ctx->filename = NULL;
>     new_ctx->file = NULL;
>+    new_ctx->file_data = NULL;
>+
>+    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;
>+
>+    }
> 
>     /* Store flags */
>     new_ctx->metadata_flags = metadata_flags;
>@@ -176,6 +510,16 @@ int ini_config_file_reopen(struct ini_cfgfile *file_ctx_in,
>     }
> 
>     new_ctx->file = NULL;
>+    new_ctx->file_data = NULL;
>+    new_ctx->filename = NULL;
>+
>+    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;
>+
>+    }
> 
>     /* Store flags */
>     new_ctx->metadata_flags = file_ctx_in->metadata_flags;
>-- 
>1.7.1
>

LS
-------------- next part --------------
>From fdd615e6e48ce09e9b9c6c025a2d8e3ba0ba39f8 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 6 Nov 2013 14:56:10 +0100
Subject: [PATCH] UTF fix

---
 ini/ini_fileobj.c | 135 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 88 insertions(+), 47 deletions(-)

diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
index d7a42ed7627db12694434076f3e00b9439af9f71..036c5f7d3b1726b4a51c18df28429fd629447fd4 100644
--- a/ini/ini_fileobj.c
+++ b/ini/ini_fileobj.c
@@ -77,26 +77,26 @@ void ini_config_file_destroy(struct ini_cfgfile *file_ctx)
 }
 
 /* How much I plan to read? */
-size_t how_much_to_read(size_t left, size_t increment)
+static size_t how_much_to_read(size_t left, size_t increment)
 {
     if(left > increment) return increment;
     else return left;
 }
 
-enum index_utf_t check_bom(enum index_utf_t ind,
-                           unsigned char *buffer,
-                           size_t len,
-                           size_t *bom_shift)
+static enum index_utf_t check_bom(enum index_utf_t ind,
+                                  unsigned char *buffer,
+                                  size_t len,
+                                  size_t *_bom_shift)
 {
     TRACE_FLOW_ENTRY();
 
-    if (len > BOM4_SIZE) {
+    if (len >= BOM4_SIZE) {
         if ((buffer[0] == 0x00) &&
             (buffer[1] == 0x00) &&
             (buffer[2] == 0xFE) &&
             (buffer[3] == 0xFF)) {
                 TRACE_FLOW_RETURN(INDEX_UTF32BE);
-                *bom_shift = BOM4_SIZE;
+                *_bom_shift = BOM4_SIZE;
                 return INDEX_UTF32BE;
         }
         else if ((buffer[0] == 0xFF) &&
@@ -104,32 +104,32 @@ enum index_utf_t check_bom(enum index_utf_t ind,
                  (buffer[2] == 0x00) &&
                  (buffer[3] == 0x00)) {
                 TRACE_FLOW_RETURN(INDEX_UTF32LE);
-                *bom_shift = BOM4_SIZE;
+                *_bom_shift = BOM4_SIZE;
                 return INDEX_UTF32LE;
         }
     }
 
-    if (len > BOM3_SIZE) {
+    if (len >= BOM3_SIZE) {
         if ((buffer[0] == 0xEF) &&
             (buffer[1] == 0xBB) &&
             (buffer[2] == 0xBF)) {
                 TRACE_FLOW_RETURN(INDEX_UTF8);
-                *bom_shift = BOM3_SIZE;
+                *_bom_shift = BOM3_SIZE;
                 return INDEX_UTF8;
         }
     }
 
-    if (len > BOM2_SIZE) {
+    if (len >= BOM2_SIZE) {
         if ((buffer[0] == 0xFE) &&
             (buffer[1] == 0xFF)) {
                 TRACE_FLOW_RETURN(INDEX_UTF16BE);
-                *bom_shift = BOM2_SIZE;
+                *_bom_shift = BOM2_SIZE;
                 return INDEX_UTF16BE;
         }
         else if ((buffer[0] == 0xFF) &&
                  (buffer[1] == 0xFE)) {
                 TRACE_FLOW_RETURN(INDEX_UTF16LE);
-                *bom_shift = BOM2_SIZE;
+                *_bom_shift = BOM2_SIZE;
                 return INDEX_UTF16LE;
         }
     }
@@ -139,7 +139,7 @@ enum index_utf_t check_bom(enum index_utf_t ind,
 }
 
 static int read_chunk(int raw_file, size_t left, size_t increment,
-                      char *position, size_t *read_num)
+                      char *position, size_t *_read_num)
 {
     int error = EOK;
     size_t to_read = 0;
@@ -164,7 +164,7 @@ static int read_chunk(int raw_file, size_t left, size_t increment,
         return error;
     }
 
-    *read_num = read_cnt;
+    *_read_num = read_cnt;
 
     TRACE_FLOW_EXIT();
     return error;
@@ -172,7 +172,7 @@ static int read_chunk(int raw_file, size_t left, size_t increment,
 
 /* Function useful for debugging */
 /*
-void print_buffer(char *read_buffer, int len)
+static void print_buffer(char *read_buffer, int len)
 {
     int i;
     for (i=0; i < len; i++) {
@@ -182,6 +182,68 @@ void print_buffer(char *read_buffer, int len)
 }
 */
 
+static int read_file_header(int raw_file,
+                            struct ini_cfgfile *file_ctx,
+                            char *read_buf,
+                            size_t *_read_cnt,
+                            size_t *_left_in_buffer,
+                            iconv_t *_conv)
+{
+    int error;
+    enum index_utf_t ind = INDEX_UTF8;
+    size_t read_cnt = 0;
+    size_t bom_shift = 0;
+    size_t left_bytes = 0;
+    iconv_t conv = (iconv_t)-1;
+    const char *encodings[] = {  "UTF-32BE",
+                                 "UTF-32LE",
+                                 "UTF-16BE",
+                                 "UTF-16LE",
+                                 "UTF-8" };
+
+    /* First time do some initialization */
+    TRACE_INFO_STRING("Reading first time.","Checking BOM");
+
+    error = read_chunk(raw_file,
+                       file_ctx->file_stats.st_size,
+                       4, /* 4 bytes to check bom */
+                       read_buf,
+                       &read_cnt);
+    if (error) {
+        TRACE_ERROR_NUMBER("Failed to read initial chunk", error);
+        return error;
+    }
+
+    ind = check_bom(ind,
+                    (unsigned char *)read_buf,
+                    read_cnt,
+                    &bom_shift);
+
+    TRACE_INFO_STRING("Converting to", encodings[INDEX_UTF8]);
+    TRACE_INFO_STRING("Converting from", encodings[ind]);
+
+    errno = 0;
+    conv = iconv_open(encodings[INDEX_UTF8], encodings[ind]);
+    if (conv == (iconv_t) -1) {
+        error = errno;
+        TRACE_ERROR_NUMBER("Failed to create converter", error);
+        return error;
+    }
+
+    /* move left bytes to beginning of buffer*/
+    if (read_cnt - bom_shift > 0) {
+        left_bytes = read_cnt - bom_shift;
+        memmove(read_buf, read_buf + bom_shift, left_bytes);
+    }
+
+    *_read_cnt = read_cnt;
+    *_left_in_buffer = left_bytes;
+    *_conv = conv;
+
+    return EOK;
+}
+
+
 /* Internal conversion part */
 static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
 {
@@ -189,7 +251,6 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
     size_t read_cnt = 0;
     size_t total_read = 0;
     size_t in_buffer = 0;
-    enum index_utf_t ind = INDEX_UTF8;
     iconv_t conv = (iconv_t)-1;
     size_t conv_res = 0;
     char read_buf[ICONV_BUFFER+1];
@@ -197,15 +258,18 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
     char *src, *dest;
     size_t to_convert = 0;
     size_t room_left = 0;
-    size_t bom_shift = 0;
-    const char *encodings[] = {  "UTF-32BE",
-                                 "UTF-32LE",
-                                 "UTF-16BE",
-                                 "UTF-16LE",
-                                 "UTF-8" };
 
     TRACE_FLOW_ENTRY();
 
+    error = read_file_header(raw_file, file_ctx, read_buf,
+                             &read_cnt, &in_buffer, &conv);
+    if (error) {
+        TRACE_ERROR_NUMBER("Failed to read file header", error);
+        return error;
+    }
+
+    total_read += read_cnt;
+
     do {
         /* print_buffer(read_buf, ICONV_BUFFER); */
         error = read_chunk(raw_file,
@@ -226,29 +290,6 @@ static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
         in_buffer = 0;
 
         /* First time do some initialization */
-        if (total_read == 0) {
-            TRACE_INFO_STRING("Reading first time.","Checking BOM");
-
-            ind = check_bom(ind,
-                            (unsigned char *)read_buf,
-                            read_cnt,
-                            &bom_shift);
-
-            /* Skip BOM the first time */
-            src += bom_shift;
-            to_convert -= bom_shift;
-
-            TRACE_INFO_STRING("Converting to", encodings[INDEX_UTF8]);
-            TRACE_INFO_STRING("Converting from", encodings[ind]);
-
-            errno = 0;
-            conv = iconv_open(encodings[INDEX_UTF8], encodings[ind]);
-            if (conv == (iconv_t) -1) {
-                error = errno;
-                TRACE_ERROR_NUMBER("Failed to create converter", error);
-                return error;
-            }
-        }
 
         total_read += read_cnt;
         TRACE_INFO_NUMBER("Total read", total_read);
@@ -388,10 +429,10 @@ static int common_file_init(struct ini_cfgfile *file_ctx)
      */
     if (file_ctx->file_stats.st_size) {
         error = common_file_convert(raw_file, file_ctx);
+        close(raw_file);
         if (error) {
             TRACE_ERROR_NUMBER("Failed to convert file",
                                 error);
-            close(raw_file);
             return error;
         }
     }
-- 
1.8.3.1



More information about the sssd-devel mailing list