[SSSD] [ding-libs] Patches for #2267, build issue and tight loop

Dmitri Pal dpal at redhat.com
Mon Mar 17 22:05:54 UTC 2014


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 at 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.

>
>> + * @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.

>
>> +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.
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?

>
>> /* 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.

>
>> +        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.

>
>> +
>> +    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 at 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.
I do not see a reason to start using it now.

>
>
>> {
>>      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.


>
>> +                            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 at 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.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/






More information about the sssd-devel mailing list