[SSSD] [ding-libs] Patches for #2267, build issue and tight loop
Stephen Gallagher
sgallagh at redhat.com
Wed Apr 2 16:43:23 UTC 2014
-----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 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
> 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-----
More information about the sssd-devel
mailing list