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(a)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@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