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(a)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(a)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(a)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
OK, I will produce a new set of patches then.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?