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