[SSSD] [PATCH] Fix copying default dp_option values

Jakub Hrozek jhrozek at redhat.com
Mon Mar 10 16:22:27 UTC 2014


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 at 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.



More information about the sssd-devel mailing list