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@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 might be useful in the future.
LS
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@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. We did so in commit 2b94ab415b30861f42b68725d9231905baf8c3bd We can always resurrect removed code if needed.
I'm not too passionate about this I just think we should be consistent.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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.
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.
Removing functions from other parts of code is something else.
LS
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@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 I just saw we did it repeatedly before and thought it's our general practice.
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.
Removing functions from other parts of code is something else.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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.
Here is an answer to your question about public libraries. We have few *public libraries* which have maintained version script libipa_hbac libsss_idmap libsss_nss_idmap libwbclient libsss_simpleifp
In this case, the rule is much more stricter. It's almost prohibited to remove functions from them or to bump a SONAME. It would cause many troubles with upgrade ...
LS
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@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?
Here is an answer to your question about public libraries. We have few *public libraries* which have maintained version script libipa_hbac libsss_idmap libsss_nss_idmap libwbclient libsss_simpleifp
In this case, the rule is much more stricter. It's almost prohibited to remove functions from them or to bump a SONAME. It would cause many troubles with upgrade ...
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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
LS
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@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'.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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.
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. I wouldn't go so far to say that "Unused code is broken code. Untested code is also broken code." because I agree with Lukas that libraries ofter have calls that are not used at a given time (nevertheless they should have tests :-) but as said in my opinion we do not have a library yet.
bye, Sumit
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
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@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
On 29/09/15 08:21, Lukas Slebodnik wrote:
[root@unused-4-233 ~]# objdump -T /usr/lib64/sssd/libsss_util.so | grep confdb
This library is considered (or has been till I was more involved with the code) private to SSSD, with no stability or ABI guarantees, and with updates that go in lockstep with the main binaries.
I do not think it would serve any useful purpose to make internal interfaces public, or commit to ABI stability for them, they are supposed to be easy to change to adapt to new needs and other internal changes.
Simo.
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@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? :-)
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@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
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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.pavel@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
On (30/09/15 10:06), Sumit Bose wrote:
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.pavel@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.
It should be done together with config validation. ATM it's hard to say which integer types we will need. But we might have separate tickets (divide&conquer)
LS
sssd-devel@lists.fedorahosted.org