On (29/09/15 17:45), Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 11:38:58AM +0200, 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 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.
I'm more with Simo here that we shouldn't consider confdb and sysdb as
stable ABIs, at least not yet. I think we need the flexibility of a private,
unstable library.
At the same time, I think we should try harder to avoid polluting the
sysdb/confdb APIs with new calls and in general strive to make even the
internal APIs easily usable. Here I agree with Lukas that the
current interface is not very nice -- and the sysdb interface would be
something that any new external contributor would be confronted with.
Maybe when we review (hint, nudge, nudge) Michal's sysdb patches, we can
take a look at the API and massage it a bit, move related functions
together in the header files and merge or remove extra functions? (Yes,
we can also file a ticket, but I don't think it would land in a
milestone where we would touch it soon...)
>
> 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.
>
> Coming to the original topic of the discussion, I would agree to remove
> confdb_get_long() because it is nowhere used (and it looks like it
> never was) and it does not even have a test.
Is that an ack? :-)
No,
I still do not think that removing function and adding it back after few months
is a good idea. Better would be to include to confdb.h and write tests.
But it seems that I'm a single person in opposition.
So someone else need to ACK it.
LS