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

Dmitri Pal dpal at redhat.com
Tue Mar 18 18:08:05 UTC 2014


On 03/18/2014 02:02 AM, Lukas Slebodnik wrote:
> 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 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.
>>
> 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 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.
> 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 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.
>>
> 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?
www.redhat.com/carveoutcosts/






More information about the sssd-devel mailing list