[SSSD] [PATCH] confdb: Remove unused function confdb_get_long

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 29 12:21:15 UTC 2015


On (29/09/15 11:38), Sumit Bose wrote:
>On Tue, Sep 29, 2015 at 10:50:06AM +0200, Pavel Reichl wrote:
>> 
>> 
>> On 09/29/2015 10:44 AM, Lukas Slebodnik wrote:
>> >On (29/09/15 10:42), Pavel Reichl wrote:
>> >>
>> >>
>> >>On 09/29/2015 10:35 AM, Lukas Slebodnik wrote:
>> >>>On (29/09/15 10:16), Pavel Reichl wrote:
>> >>>>On 09/29/2015 10:08 AM, Lukas Slebodnik wrote:
>> >>>>>On (29/09/15 08:45), Pavel Reichl wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>>On 09/29/2015 08:31 AM, Lukas Slebodnik wrote:
>> >>>>>>>On (27/09/15 12:49), Pavel Reichl wrote:
>> >>>>>>>>Hello, please see trivial patch attached.
>> >>>>>>>
>> >>>>>>>>From b9f938087973444f0ec26fc24ad68dca7ac63034 Mon Sep 17 00:00:00 2001
>> >>>>>>>>From: Pavel Reichl <reichl.pavel at gmail.com>
>> >>>>>>>>Date: Sun, 27 Sep 2015 12:34:20 +0200
>> >>>>>>>>Subject: [PATCH] confdb: Remove unused function confdb_get_long
>> >>>>>>>>
>> >>>>>>>>---
>> >>>>>>>>src/confdb/confdb.c | 51 ---------------------------------------------------
>> >>>>>>>>1 file changed, 51 deletions(-)
>> >>>>>>>>
>> >>>>>>>>diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
>> >>>>>>>>index c097aad7745eda4fff051c7da027776f95db0f03..eebd478f74041d2050df9edc283df43a65462340 100644
>> >>>>>>>>--- a/src/confdb/confdb.c
>> >>>>>>>>+++ b/src/confdb/confdb.c
>> >>>>>>>>@@ -475,57 +475,6 @@ failed:
>> >>>>>>>>     return ret;
>> >>>>>>>>}
>> >>>>>>>>
>> >>>>>>>>-long confdb_get_long(struct confdb_ctx *cdb,
>> >>>>>>>>-                     const char *section, const char *attribute,
>> >>>>>>>>-                     long defval, long *result)
>> >>>>>>>>-{
>> >>>>>>>Would it be better to consider this function as confdb API
>> >>>>>>>and add to src/confdb/confdb.h?
>> >>>>>>
>> >>>>>>It could be.
>> >>>>>>
>> >>>>>>>It might be useful in the future.
>> >>>>>>
>> >>>>>>I thought that out policy towards unused functions were to remove them.
>> >>>>>Could you point me to the description of such policy?
>> >>>>>I'm not aware of it.
>> >>>>
>> >>>>I don't think it's written anywhere
>> >>>Good to know. I thought I missed something
>> >>>
>> >>>>I just saw we did it repeatedly before and thought it's our general practice.
>> >>>>
>> >>>One more time:
>> >>>If we consider confdb as library than we should never remove functions.
>> >>>
>> >>>Removing functions from other parts of code is something
>> >>>else.
>> >>>
>> >>>>>
>> >>>>>>We did so in commit 2b94ab415b30861f42b68725d9231905baf8c3bd
>> >>>>>I didn't notice that patch. I'm sorry I do not have a time
>> >>>>>to follow each patchset.
>> >>>>>
>> >>>>>>We can always resurrect removed code if needed.
>> >>>>>>
>> >>>>>If we consider confdb as library than
>> >>>>>we should never remove functions.
>> >>>>
>> >>>>Then we should decide which parts of SSSD are public libraries and which are not, then we could avoid this kind of discussions.
>> >>>>
>> >>>I wrote "consider confdb as library" and not as a public library with
>> >>>stable API. And libraries many times contains function which are not used.
>> >>>So we should not remove them. Even though it would be still in git history.
>> >>
>> >>Why? If we don't use them and nobody else does why do we have to keep them?
>> >>
>> >libraries many times contains function which are not used
>> 
>> Well, but if they are not public we can change that, right? We can change them as we need and removing unused code is one of those things. I don't see the difference between private library and 'other parts of code'.
>
>I think we should be explicit here. If we want to consider confdb as a
>library we should make it a public library with a stable API with all
The library is already public, but does not have public header file.
[root at unused-4-233 ~]# objdump -T /usr/lib64/sssd/libsss_util.so | grep confdb
00000000000138a0 g    DF .text  00000000000002a1  Base        confdb_set_string
0000000000013b50 g    DF .text  0000000000000187  Base        confdb_get_string
0000000000016200 g    DF .text  00000000000001e1  Base        confdb_list_all_domain_names
0000000000048880 g    DF .text  00000000000005f7  Base        sss_confdb_create_ldif
0000000000014500 g    DF .text  0000000000001c63  Base        confdb_get_domains
00000000000141f0 g    DF .text  0000000000000155  Base        confdb_get_string_as_list
0000000000016170 g    DF .text  000000000000008c  Base        confdb_get_domain
0000000000013650 g    DF .text  000000000000024b  Base        confdb_get_param
0000000000014010 g    DF .text  00000000000001da  Base        confdb_get_bool
0000000000014350 g    DF .text  00000000000001ab  Base        confdb_init
00000000000132f0 g    DF .text  0000000000000358  Base        confdb_add_param
0000000000013ce0 g    DF .text  000000000000019f  Base        confdb_get_int
0000000000013e80 g    DF .text  000000000000018d  Base        confdb_get_long

>the consequences. In general I think this is not a bad idea e.g. with
>respect to all the config related tickets we have planned for 1.14. But
>we should define the initial version of a stable API, based on the
>result and requirements from the 1.14 tickets.
>

>Having private/internal libraries with a more relaxed policy might turn
>out to be problematic as e.g. can be seen by some internal samba
>libraries which are used by us, OpenChange and maybe other projects
>where upstream changes might force changes in the other projects as
>well. Ok, the argument here is that they shouldn't have been used in the
>first place, but what's the point in re-implementing existing
>functionality.
>
>Currently I wouldn't see libsss_util where confdb.c is included as a
>library at all, but just as a mean to make the build process easier and
>faster.
>
It is a library but with terrible interface and problematic dependency.
libsss_util.so requires libsss_child and libsss_child requires functions from
libsss_util.so.

Another problem with libsss_util.so is that it's a mix of many "libraries"
sysdb, confdb ...

Another problem is tat we use confdb_get_int on many places even though
that there should be confdb_get_uint or confdb_get_long.
We have few tickets which was testing -1 as unallowed value and it passed.

So we can remove confdb_get_long atm and we might want to introduce
later after implementation of config validation. Decide what you want to
do. I will rather spend time with something more productive:
improving CI script, writing unit tests ...

LS


More information about the sssd-devel mailing list