[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