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(a)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....
I generated coverage without optimisation "-O0" and now it shown as covered
http://lslebodn.fedorapeople.org/ding-libs-coverage-O2/ini/ini_fileobj.c....
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
Does this mean the final ack?
--
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IdM portfolio
Red Hat, Inc.