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

Lukas Slebodnik lslebodn at redhat.com
Tue Apr 1 07:06:52 UTC 2014


On (31/03/14 18:57), Dmitri Pal wrote:
>On 03/31/2014 02:22 PM, Lukas Slebodnik wrote:
>>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.
>
>I really do not see a difference, sorry.
>
>>
>>>>>>>>+ * @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 />
>
>
>As I explained earlier this use case is checked implicitly.
>There are files with 0 length in the test. If you check the verbose
>output of the unit test you will see that case when the data is 0 is
>executed and the line
>
>+        INIOUT(printf("Data len %u\n", stream_len));
>
>"Data len 0"
>
>so this is covered.
You are right. It is covered.
It was not shown on the test coverage page due to optimisation.
http://lslebodn.fedorapeople.org/ding-libs-coverage-O2/ini/ini_fileobj.c.gcov.html

I generated coverage without optimisation "-O0" and now it shown as covered
http://lslebodn.fedorapeople.org/ding-libs-coverage-O2/ini/ini_fileobj.c.gcov.html

Both files: lines 413-419

>
>>
>>>>>>>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|
>
>Do we have any requirement on the level?
>I see two places that are outliers:
>
>/dev/shm/ding-libs/collection/collection_cnv.c     |48.8    201|51.0    51|
>
>This file has a lot of convenience wrappers. I do not see a reason to add tests to those a bit too much.
>
>/dev/shm/ding-libs/ini/ini_print.c                 |20.9    115|20.0    10|
>
>
>This one has some future looking functions that are not used yet and
>some functions that are used in the older version of the API.
>
>Seems like we are well covered.
Yes it is well covered.
You can see results on page
http://lslebodn.fedorapeople.org/ding-libs-coverage/


LS



More information about the sssd-devel mailing list