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<d...@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<d...@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<d...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel