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

Lukas Slebodnik lslebodn at redhat.com
Mon Mar 31 18:22:08 UTC 2014


On (30/03/14 22:31), Dmitri Pal wrote:
>On 03/18/2014 02:08 PM, Dmitri Pal wrote:
>>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/
>
>I dropped the first patch as it is not relevant to the tree and only
>local to my build environment.
>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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)
>
>"NULL terminated" is a standard term.
>http://en.wikipedia.org/wiki/Null-terminated_string
>I did not change this
>
It isn't "NULL terminated" but "null-terminated"
It is not big difference but it would be great to have a review of doxygen
comments from native speaker.

>>>
>>>>>>+ * @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};

<snip />

>>>>>>+/* 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)
>
>
>I added new_line.conf.
>
The project ding-libs has very good test coverage.
(78.1% of LOC and 88.6% of functions). Good job.
I cheched test coverage of new functions
added since release of ding_libs-0_3_0_1; almost everything if covered.

error = ini_config_file_from_mem(stream_data, 0, &file_ctx);
                                             ^^^
                                 but this corner case is not covered.
                                 It was mentioned in my previous mail.

Tests help us to catch some problems with refactoring (or adding new code)
It would be great to keep test coverage as high as possible.

<snip />

>>>>>
>>>>>LS
>>>>Thanks for the review.
>>>>
>>>LS
>>OK, I will produce a new set of patches then.
>>
>
>I ran the tests again, did distcheck and verified valgrind.
>Everything seems OK.
>
>Refined patches attached.
>

Patches work fine and previous two comments are not show stopper.

LS

Table of test coverage for ding-libs.

sh-4.2$ lcov --list coverage.info --list-full-path
Reading tracefile coverage.info
                                                   |Lines      |Functions |
Filename                                           |Rate    Num|Rate   Num|
============================================================================
/dev/shm/ding-libs/basicobjects/simplebuffer.c     |96.6     58| 100    10|
/dev/shm/ding-libs/collection/collection.c         |91.2    808|98.0    51|
/dev/shm/ding-libs/collection/collection_cmp.c     |66.9    133| 100     2|
/dev/shm/ding-libs/collection/collection_cnv.c     |48.8    201|51.0    51|
/dev/shm/ding-libs/collection/collection_iter.c    |92.5    134| 100     9|
/dev/shm/ding-libs/collection/collection_queue.c   |91.8     73|92.3    13|
/dev/shm/ding-libs/collection/collection_stack.c   |87.7     73|84.6    13|
/dev/shm/ding-libs/collection/collection_tools.c   |10.9    267|15.4    13|
/dev/shm/ding-libs/dhash/dhash.c                   |83.4    368|92.6    27|
/dev/shm/ding-libs/ini/ini_comment.c               |77.0    226|95.0    20|
/dev/shm/ding-libs/ini/ini_config.c                |75.5    290| 100    11|
/dev/shm/ding-libs/ini/ini_configobj.c             |91.5    328| 100    19|
/dev/shm/ding-libs/ini/ini_fileobj.c               |75.7    296|86.7    15|
/dev/shm/ding-libs/ini/ini_get_array.c             |87.0    131| 100     8|
/dev/shm/ding-libs/ini/ini_get_array_valueobj.c    |86.5    141| 100     8|
/dev/shm/ding-libs/ini/ini_get_value.c             |82.2    185| 100    17|
/dev/shm/ding-libs/ini/ini_get_valueobj.c          |84.0    243| 100    19|
/dev/shm/ding-libs/ini/ini_list.c                  |84.0     25| 100     4|
/dev/shm/ding-libs/ini/ini_list_valueobj.c         |87.1     31| 100     4|
/dev/shm/ding-libs/ini/ini_metadata.c              |67.8    143|85.7     7|
/dev/shm/ding-libs/ini/ini_parse.c                 |88.1    586| 100    20|
/dev/shm/ding-libs/ini/ini_print.c                 |20.9    115|20.0    10|
/dev/shm/ding-libs/ini/ini_serialize.c             |94.7     19| 100     2|
/dev/shm/ding-libs/ini/ini_valueobj.c              |86.7    369|95.8    24|
/dev/shm/ding-libs/path_utils/path_utils.c         |86.3    270|93.3    15|
/dev/shm/ding-libs/refarray/ref_array.c            |85.9    170|93.3    15|
===========================================================================
                                             Total:|78.1   5683|88.6   407|



More information about the sssd-devel mailing list