On Wed, Sep 30, 2015 at 08:31:57AM +0200, Pavel Reichl wrote:
>
>
> On 09/30/2015 07:42 AM, Lukas Slebodnik wrote:
> >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.pa...@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 think this was rather question for Sumit AFAIK he was the author of quoted
> text.
>
> >I still do not think that removing function and adding it back after few
> >months
>
> The problem is - why do you think we will need the function in future? Using
> this logic we could never remove any function from private libs.
Lukas gave some justification in another part of this thread:
"""
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.
"""
Lukas, would you agree if we create a ticket like "Evaluate the usage of
confdb_get_int()" for the next release. Then you can replace
confdb_get_int() at places where you think more specific calls will fit
better. Maybe it would even make sense to add calls like
confdb_get_uint32() during this effort.
bye,
Sumit
>
> >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
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel@lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
