[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