On (26/02/14 14:12), Jakub Hrozek wrote:
On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
> On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
> >Hi,
> >
> >The attached two patches are related to:
> >https://fedorahosted.org/sssd/ticket/2257
> >
> >The first patch is included pretty much for completeness, as I noted
> >during development of the unit test, the blob type didn't handle any
> >default value.
> >
> >The second patch directly addresses #2257. The previous code didn't
> >handle copying options well if the option was set to zero, because the
> >code followed logic like:
> > if (oldval) {
> > newval = oldval;
> > else {
> > newval = defval;
> > }
> >
> >The patch implements Sumit's idea to provide a separate function for
> >copying default values and amend the generic copy function to only
> >create a copy using the current values.
>
> Nack.
>
> I think the approach is all right for this purpose, however have you
> considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit.
But then he came up with the argument (which I agree with) that _copy
function should only really copy assigned values, not do a half-copy of
defaults and half of values..hence the split of copy and copy_defaults.
>
> I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable
with GCC. Especially the first warning was bad and perhaps a compiler
bug (I suspect that clang optimizes the strlen for sizeof internally,
but I don't think it should do so with -O0)
From f6f735b44870c0c4895fa43a380da571dd2d0f82 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 24 Feb 2014 15:42:51 +0100
Subject: [PATCH 2/2] DP: Provide separate dp_copy_defaults function
---
Makefile.am | 12 ++
src/providers/ad/ad_common.c | 16 +-
src/providers/data_provider.h | 5 +
src/providers/data_provider_opts.c | 42 ++--
src/tests/cmocka/test_dp_opts.c | 421 +++++++++++++++++++++++++++++++++++++
src/tests/ipa_ldap_opt-tests.c | 2 +-
6 files changed, 476 insertions(+), 22 deletions(-)
create mode 100644 src/tests/cmocka/test_dp_opts.c
diff --git a/Makefile.am b/Makefile.am
index e51003c29dd673199949b60575cb8fb5f7eee8a0..11abaf9b81604fb08957a0c185e7bfe2e6c7a5f0
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -165,6 +165,7 @@ if HAVE_CMOCKA
test_utils \
ad_access_filter_tests \
ad_common_tests \
+ dp_opt_tests \
test_search_bases
endif
@@ -1590,6 +1591,17 @@ ad_common_tests_LDADD = \
libsss_krb5_common.la \
libsss_test_common.la
+dp_opt_tests_SOURCES = \
+ src/providers/data_provider_opts.c \
+ src/tests/cmocka/test_dp_opts.c
+dp_opt_tests_CFLAGS = \
+ $(AM_CFLAGS)
+dp_opt_tests_LDADD = \
+ $(CMOCKA_LIBS) \
+ $(TALLOC_LIBS) \
+ $(SSSD_INTERNAL_LTLIBS) \
+ libsss_test_common.la
+
endif
The test cannot be build with disabled link_all_deplibs (debian).
(master and sssd-1-11)
Simple patch is attached.
LS