On Wed, Jun 22, 2016 at 10:52:34AM +0200, Lukas Slebodnik wrote:
> On (22/06/16 10:39), Jakub Hrozek wrote:
> >On Wed, Jun 22, 2016 at 09:28:26AM +0200, Lukas Slebodnik wrote:
> >> ehlo,
> >>
> >> The first 4 patches are slightly modified version
> >> of Michal's patches. Mostly coding style issues, fixed reports from static
> >> analyzers + fixed small issues which would be fixed as part of review
> >> process.
> >>
> >> Last two patches are optional improvements which changes ofiginal API.
> >> It was not discussed as part of desing review
> >> but these ideas come to my mind as a part or reading code.
> >>
> >> Please review patches.
> >>
> >> LS
> >
> >Can you rebase them atop Michal's and Dmitri's patches I just pushed?
> Sure,
>
> + attached are patches which bump version info and version.
>
> Changes for version-info will be the same even if we remove
> optional patches (7th and 8th patch)
>
> LS
I only spot-checked Michal's patches, because you did the most there, I
will also add your RB.
I also tested this code with SSSD, also seems to work well. The SONAME
of libini_config remains at .5 (.5.1 -> 5.2) and the API changes were
only additions.
> From 0f36ea26b3fc9f77d90dff9d8ea4a7f5d4bb3388 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
> Date: Wed, 3 Feb 2016 18:51:49 +0100
> Subject: [PATCH 01/10] ini: Add infrastructure for validators
LGTM
> From b8ae1b0b0da342587713fae8f0813fac1c1462a6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
> Date: Tue, 8 Mar 2016 17:15:54 +0100
> Subject: [PATCH 02/10] ini: Add internal validator ini_allowed_options
LGTM
> From 3730861426299e7443aa5f950002056d1b3b020e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
> Date: Tue, 15 Mar 2016 17:35:05 +0100
> Subject: [PATCH 03/10] tests: Tests for rules/validators infrastructure
LGTM + tested with valgrind
> From b61ecc0b47529f9bb285b2b7560d6681c19bca3d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzi...@redhat.com>
> Date: Fri, 15 Apr 2016 12:13:58 +0200
> Subject: [PATCH 04/10] ini: Add internal validator allowed_sections
LGTM, except one comment..
> +static int is_allowed_section(const char *tested_section,
> + char **allowed_sections,
> + size_t num_sec,
> + regex_t *allowed_sections_re,
> + size_t num_sec_re,
> + int case_insensitive)
> +{
> + int ret;
> + int i;
> +
> + if (case_insensitive) {
> + for (i = 0; i < num_sec; i++) {
> + if (strcasecmp(tested_section, allowed_sections[i]) == 0) {
> + return 1;
> + }
> + }
> + } else { /* case insensitive */
This comment should say case sensitive, I can fix this before pushing.
> From 5602bcb89c7c29e843a93a9ac45b384098df5a9e Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Tue, 21 Jun 2016 22:32:33 +0200
> Subject: [PATCH 05/10] INI: Enable string format check for ini_errobj_add_msg
ACK
> From ecfee264ec5736348aa045e4d68ac0e211327a61 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Tue, 21 Jun 2016 23:01:25 +0200
> Subject: [PATCH 06/10] INI: Extend validator unit test for corner cases
ACK + tested with valgrind
> From 069d613ee210b7f1631b9f0bd15e52f270c72078 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Wed, 22 Jun 2016 00:50:50 +0200
> Subject: [PATCH 07/10] INI: Reduce count of argumets for ini_rules_check
>
> We can use NULL terminated array of pointers instead of
> two argumets: array + length of array.
OK, this is a cleaner API, so ACK
> From 374c6d0bca88360a2a1baae023b89de19f685736 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Wed, 22 Jun 2016 09:13:04 +0200
> Subject: [PATCH 08/10] INI: Prepare for schema validation
>
> Pointer to function ini_schema_validator_func will be an optional
> and can be used to schema validation. It shoul also prepare data
> for validator function if last output argument is not NULL.
> These prepared data will be passed to validator function as
> a 5th argument of ini_validator_func. These two functions
> are responsible for memory management of passed additional data.
>
> It isn't an API/ABI change because lib_iniconfig has not been released yet.
...
> @@ -2173,6 +2179,7 @@ typedef int (ini_validator_func)(const char *rule_name,
> struct ini_validator {
> const char *name;
> ini_validator_func *func;
I would prefer to add a comment explaining that this attribute is
currently unused here, but otherwise ACK. I can do that before pushing
as well if you agree.
> + ini_schema_validator_func *schema_validator;
> From b0af6b0e2da7fe929d7651a313cd59cd86b83195 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Wed, 22 Jun 2016 10:45:09 +0200
> Subject: [PATCH 09/10] Bump version-info
ACK
> From 313a037671b146102276ef65ad32e8743841058b Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lsleb...@redhat.com>
> Date: Wed, 22 Jun 2016 10:46:44 +0200
> Subject: [PATCH 10/10] Update versions before 0.6.0 release
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
