On Mon, Mar 10, 2014 at 05:22:27PM +0100, Jakub Hrozek wrote:
On Mon, Mar 10, 2014 at 07:43:24AM +0100, Lukas Slebodnik wrote:
> 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
ACK, builds fine.
Pushed to master.