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.
+ * @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};
+uint32_t alt_buffer_len = 1;
+
/* 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);
- 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));
+
+ 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;
+ }
+ 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;
+ 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;
+
+ 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)
{
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) {
+ /* Get file stats */
+ errno = 0;
+ stat_ret = stat(in_filename, &file_stats);
+ if (stat_ret == -1) {
+ 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) {
+ 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) {
+ 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) {
+ 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",
+ 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 05229bcdedf90e81991c9f907c6acca93292a93b Mon Sep 17 00:00:00 2001
From: Dmitri Pal <d...@redhat.com>
Date: Sat, 8 Mar 2014 18:59:20 -0500
Subject: [PATCH 1/4] [BUILD] Fixing build
The EXTRA_ prefix is not recognized by RHEL6 autotools.
---
Makefile.am | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 44a9dff..cb9008f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -256,7 +256,7 @@ libini_config_la_SOURCES = \
ini/ini_get_array_valueobj.c \
ini/ini_list_valueobj.c \
trace/trace.h
-EXTRA_libini_config_la_DEPENDENCIES = ini/libini_config.sym
+libini_config_la_DEPENDENCIES = ini/libini_config.sym
libini_config_la_LIBADD = \
libcollection.la \
libpath_utils.la \
--
1.7.1
From 257d51c0d24f36853c463897d3b84b7db1c9da7f Mon Sep 17 00:00:00 2001
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) {
--
1.7.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
