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.
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.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
