-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/01/2014 10:33 PM, Dmitri Pal wrote:
On 04/01/2014 03:06 AM, Lukas Slebodnik wrote:
> 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
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?
Just a couple minor edits to the language of the doxygen:
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.
@brief Create a configuration file object from a NULL-terminated buffer.
+ *
+ * Create a file object for parsing a configuration file.
+ * Configuration will use provided memory instead of the actual file.
Configuration will use the provided memory buffer instead of a real file.
+ *
+ * A "configuration file object" is different from
+ * a "configuration object". The former stores metadata
+ * about a file the configuration data is read from,
about a file from which the configuration data is read,
(this is a very esoteric bit of English)
+ * while the latter holds the configuration itself.
while the latter holds the configuration data itself.
+ *
+ * @param[in] data_buf In memory configuration data.
In-memory
+ * Needs to be NULL terminated.
NULL-terminated
+ * @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);
+
One additional question: is it expected that data_buf will contain NUL
values in it other than the terminator? If not, why does data_len exist?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird -
http://www.enigmail.net/
iEYEARECAAYFAlM8PioACgkQeiVVYja6o6PBMgCgiOhGQfO3wJgLhrhUo/88+s0v
+RoAn1inyIefVOAZbwMHIQnGT4TiQCme
=Z6Vs
-----END PGP SIGNATURE-----