On (17/10/13 12:56), Lukas Slebodnik wrote:
>On (10/10/13 23:03), Dmitri Pal wrote:
>>Hello,
>>
>>The attached patches address ticket
>>https://fedorahosted.org/sssd/ticket/2106
>>The patches do not define new flags as suggested in the ticket.
>>As we discussed with Nalin we try to detect the encoding based on the
>>BOM and do the right thing without flags.
>>If we see that this is not enough we will add flags but that would add a
>>new function so such change would be destined for a bigger release.
>>For now I think the use case is addressed. If we see the need to other
>>use cases we will open other tickets.
>>
>>The patch depends on the previous set of patches that deal with C-Style
>>comments.
>>
>>I also found some other unrelated issue that I logged as a separate ticket.
>>https://fedorahosted.org/sssd/ticket/2119
>>
>>--
>>Thank you,
>>Dmitri Pal
>>
>>Sr. Engineering Manager for IdM portfolio
>>Red Hat Inc.
>>
>>
>>-------------------------------
>>Looking to carve out IT costs?
>>www.redhat.com/carveoutcosts/
>>
>>
>>
>
>>From ee1988301ba164a12263b7faf8c9bf686174deab Mon Sep 17 00:00:00 2001
>>From: Dmitri Pal <d...@redhat.com>
>>Date: Thu, 10 Oct 2013 22:47:23 -0400
>>Subject: [PATCH 3/4] [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 | 317
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 306 insertions(+), 11 deletions(-)
>>
>>diff --git a/ini/ini_fileobj.c b/ini/ini_fileobj.c
>>index 7ac9dc6..000d36e 100644
>>--- a/ini/ini_fileobj.c
>>+++ b/ini/ini_fileobj.c
>>@@ -21,14 +21,29 @@
>>
>> #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 100
>>+
>>+#define BOM4_SIZE 4
>>+#define BOM3_SIZE 3
>>+#define BOM2_SIZE 2
>>+
>>+#define INDEX_UTF32BE 0
>>+#define INDEX_UTF32LE 1
>>+#define INDEX_UTF16BE 2
>>+#define INDEX_UTF16LE 3
>>+#define INDEX_UTF8 4
I forgot to write one comment in the previous mail.
Could we use enum for INDEX_* instead of constants?
>>
>> /* Close file but not destroy the object */
>> void ini_config_file_close(struct ini_cfgfile *file_ctx)
>>@@ -52,6 +67,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 +75,298 @@ 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)
>>+{
>>+ if(left > increment) return increment;
>>+ else return left;
>>+}
>>+
>>+int check_bom(int ind, unsigned char *buffer, size_t len, size_t *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;
>>+}
>>+
>>+/* Internal conversion part */
>>+static int common_file_convert(int raw_file, struct ini_cfgfile *file_ctx)
>> {
>This function is quite long (174 LOC).
>It would be better to split it into smaller functions.
>
>> int error = EOK;
>>+ size_t read_cnt = 0;
>>+ size_t total_read = 0;
>>+ size_t to_read = 0;
>>+ size_t in_buffer = 0;
>>+ int ind = INDEX_UTF8;
>>+ iconv_t conv = 0;
>>+ 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 {
>>+ to_read = how_much_to_read(file_ctx->file_stats.st_size - total_read,
>>+ ICONV_BUFFER - in_buffer);
>>+
>>+ TRACE_INFO_NUMBER("About to read", to_read);
>>+ errno = 0;
>>+ read_cnt = read(raw_file, read_buf + in_buffer, to_read);
>>+ if (read_cnt == -1) {
>>+ error = errno;
>>+ close(raw_file);
>read can fail on second round of loop do-while
>and conv handle will not be closed.
>
>>+ TRACE_ERROR_NUMBER("Failed to read data from file", error);
>>+ return error;
>>+ }
>>+
>>+ if (read_cnt != to_read) {
>>+ error = EIO;
>>+ close(raw_file);
>The same situation is here.
>
>>+ TRACE_ERROR_NUMBER("Read less than required", error);
>>+ return error;
>>+ }
>>+
>>+ /* First time do some initialization */
>>+ if(total_read == 0) {
> ^^^
> missing space
>>+ TRACE_INFO_STRING("Reading first time.","Checking BOM");
>>+
>>+ ind = check_bom(ind, (unsigned char *)read_buf, read_cnt,
>>&bom_shift);
>
> ^^^^^^^^^^^
> line length > 80
>>+
>>+ 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;
>>+ close(raw_file);
>>+ TRACE_ERROR_NUMBER("Failed to create converter", error);
>>+ return error;
>>+ }
>>+ }
> ^^^^^
> I would prefer to have initialisation >>> if(total_read == 0) <<<
> before the outermost do while, because it generate coverity warning.
> Overwriting "conv" in "conv = iconv_open(encodings[4],
> encodings[ind])" leaks the storage that "conv" points to.
> I know that it is a fall positive, but code will be simpler.
>
>>+ total_read += read_cnt;
>>+ TRACE_INFO_NUMBER("Total read", total_read);
>>+
>>+ do {
>>+ /* Do conversion */
>>+ errno = 0;
>>+ src = read_buf + bom_shift;
>>+ dest = result_buf;
>>+ to_convert = read_cnt + in_buffer - bom_shift;
>>+ bom_shift = 0;
>>+ room_left = ICONV_BUFFER;
>>+ conv_res = iconv(conv, &src, &to_convert, &dest, &room_left);
>>+ if (conv == (iconv_t) -1) {
> ^^^^
> conv_res should be checked and it has type size_t
>>+ error = errno;
>>+ switch(error) {
>>+ case EILSEQ:
>>+ TRACE_ERROR_NUMBER("Invalid multibyte encoding", error);
>>+ close(raw_file);
>>+ iconv_close(conv);
>>+ return error;
>>+ case EINVAL:
>>+ /* We need to just read more if we can */
>>+ if (total_read != file_ctx->file_stats.st_size) {
>>+ TRACE_INFO_STRING("Incomplete sequence.", "");
>>+ TRACE_INFO_NUMBER("File size.",
>>+ file_ctx->file_stats.st_size);
>>+ memmove(read_buf, src, to_convert);
>>+ in_buffer = to_convert;
>>+ }
>>+ else {
>>+ /* Or return error if we can't */
>>+ TRACE_ERROR_NUMBER("Incomplete sequence", error);
>>+ close(raw_file);
>>+ iconv_close(conv);
>>+ return error;
>>+ }
> ^^^^^^^^^
> missing break.
> Is it intentional?
>http://www.freeipa.org/page/Coding_Style#Switch
>/* FALLTHROUGH */
>>+ 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);
>>+ close(raw_file);
>>+ iconv_close(conv);
>>+ return error;
>>+ }
>>+
>>+ room_left = ICONV_BUFFER;
>>+ dest = result_buf;
>>+ continue;
>>+ default:
>>+ TRACE_ERROR_NUMBER("Unexpected internal error",
>>+ error);
>>+ close(raw_file);
>>+ 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);
>>+ close(raw_file);
>>+ iconv_close(conv);
>>+ return error;
>>+ }
>>+ TRACE_INFO_STRING("Saved procesed portion.",
>>+ (char
>>*)simplebuffer_get_vbuf(file_ctx->file_data));
>>+
>>+ in_buffer = 0;
>>+ break;
>>+ }
>>+ while (1);
>>+ }
>>+ while(total_read < file_ctx->file_stats.st_size);
>>+
>>+ close(raw_file);
>>+ 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;
>>+}
>>+
>>+
>>+/* 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;
>>+ TRACE_ERROR_NUMBER("Failed to get file stats", error);
>Resource leak
>Handle variable "raw_file" going out of scope leaks the handle.
>>+ return error;
>>+ }
>>
>>- /* Collect stats */
>>- if (file_ctx->metadata_flags & INI_META_STATS) {
>>+ /* 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);
>>+ 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);
>>+ 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 +396,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 +462,15 @@ int ini_config_file_reopen(struct ini_cfgfile
>>*file_ctx_in,
>> }
>>
>> 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);
>It is better to initialize new_ctx with memset or initialize
>new_ctx->filename to NULL, because ini_config_file_destroy can access to
>uninitialized variable.
>
>>+ return error;
>>+
>>+ }
>>
>> /* Store flags */
>> new_ctx->metadata_flags = file_ctx_in->metadata_flags;
>>--
>>1.7.1
>>
>
>>From e4fcd1ba0f86675e6fe6455c80d5215b821af973 Mon Sep 17 00:00:00 2001
>>From: Dmitri Pal <d...@redhat.com>
>>Date: Wed, 9 Oct 2013 23:29:22 -0400
>>Subject: [PATCH 1/4] [INI] Expose buffer context as void
>>
>>In some cases the intenal buffer needs to be accessed as void.
>>Add a function that would do that instead of type casting
>>and dealing with 'const'.
>>---
>> basicobjects/simplebuffer.c | 7 +++++++
>> basicobjects/simplebuffer.h | 4 ++++
>> 2 files changed, 11 insertions(+), 0 deletions(-)
>>
>ACK
>
>>From b48891b2753bd8d26f71f6cbb18b28bd22ece9cc Mon Sep 17 00:00:00 2001
>>From: Dmitri Pal <d...@redhat.com>
>>Date: Thu, 10 Oct 2013 22:51:22 -0400
>>Subject: [PATCH 4/4] [INI] Updated unit test for UTF8 conversion
>>
>>Patch adds new test files and bom creation function.
>>---
>> ini/ini.d/real16be.conf | Bin 0 -> 3288 bytes
>> ini/ini.d/real16le.conf | Bin 0 -> 3288 bytes
>> ini/ini.d/real32be.conf | Bin 0 -> 6576 bytes
>> ini/ini.d/real32le.conf | Bin 0 -> 6576 bytes
>> ini/ini.d/real8.conf | 70
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> ini/ini_parse_ut.c | 35 +++++++++++++++++++++++
>> 6 files changed, 105 insertions(+), 0 deletions(-)
>> create mode 100644 ini/ini.d/real16be.conf
>> create mode 100644 ini/ini.d/real16le.conf
>> create mode 100644 ini/ini.d/real32be.conf
>> create mode 100644 ini/ini.d/real32le.conf
>> create mode 100644 ini/ini.d/real8.conf
>>
>
>The are some trailing whitespaces in real8.conf, but it seems like intentional.
>
>ACK
>
>>From f0c79c9091d9165d60aaa030d09fe63450215c5d Mon Sep 17 00:00:00 2001
>>From: Dmitri Pal <d...@redhat.com>
>>Date: Wed, 9 Oct 2013 23:31:49 -0400
>>Subject: [PATCH 2/4] [INI] Extend internal file handle
>>
>>When we need to decode the file we need to keep it
>>in memory so we need to have a buffer.
>>Adding simple buffer into the internal structure.
>>---
>> ini/ini_config_priv.h | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>>diff --git a/ini/ini_config_priv.h b/ini/ini_config_priv.h
>>index 89ad8a9..e1292f7 100644
>>--- a/ini/ini_config_priv.h
>>+++ b/ini/ini_config_priv.h
>>@@ -26,6 +26,7 @@
>> #include <sys/stat.h>
>> #include <unistd.h>
>> #include "collection.h"
>>+#include "simplebuffer.h"
>> #include "ini_comment.h"
>>
>> /* Configuration object */
>>@@ -71,6 +72,8 @@ struct ini_cfgfile {
>> struct stat file_stats;
>> /* Were stats read ? */
>> int stats_read;
>>+ /* Internal buffer */
>>+ struct simplebuffer *file_data;
>> };
>>
>> /* Parsing error */
>
>ACK
>
>LS
>_______________________________________________
>sssd-devel mailing list
>sssd-devel@lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
