-----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<d...@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----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel