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

Pavel Reichl preichl at redhat.com
Wed Sep 30 06:31:57 UTC 2015



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

> 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>


More information about the sssd-devel mailing list