Hello,
this patch replaces strerror with sss_strerror on some places. I think it would be OK to use always sss_strerror, but to keep the patch relatively small I left strerror on places where we directly print value of errno or return value of some third party functions that do not (and never will) return our specific error codes.
Patch is attached. It may look big (111 files changed), but there are only few insertions and deletions in each.
Thanks, Michal
I haven't tested this patch to be honest, but then again, there's not so much to test except compiling and a bit of sanity testing..
I prefer Michal's version of using sss_strerror explicitly instead of the route Simo proposed to #define strerror as sss_strerror simply because I prefer to see what function gets called right away. The drawback of mass-converting to sss_strerror is that we lose git-blame history somewhat...but given that we have just recently converted the DEBUG messages and strerror is mostly used only around DEBUG messages, I don't think we'd lose much metadata.
But I'd like to hear other opinions as well.
On Thu, Apr 24, 2014 at 7:03 PM, Michal Židek mzidek@redhat.com wrote:
Hello,
this patch replaces strerror with sss_strerror on some places. I think it would be OK to use always sss_strerror, but to keep the patch relatively small I left strerror on places where we directly print value of errno or return value of some third party functions that do not (and never will) return our specific error codes.
Patch is attached. It may look big (111 files changed), but there are only few insertions and deletions in each.
Thanks, Michal
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/11/2014 11:10 PM, Jakub Hrozek wrote:
I haven't tested this patch to be honest, but then again, there's not so much to test except compiling and a bit of sanity testing..
I prefer Michal's version of using sss_strerror explicitly instead of the route Simo proposed to #define strerror as sss_strerror simply because I prefer to see what function gets called right away. The drawback of mass-converting to sss_strerror is that we lose git-blame history somewhat...but given that we have just recently converted the DEBUG messages and strerror is mostly used only around DEBUG messages, I don't think we'd lose much metadata.
But I'd like to hear other opinions as well.
On Thu, Apr 24, 2014 at 7:03 PM, Michal Židek mzidek@redhat.com wrote:
Hello,
this patch replaces strerror with sss_strerror on some places. I think it would be OK to use always sss_strerror, but to keep the patch relatively small I left strerror on places where we directly print value of errno or return value of some third party functions that do not (and never will) return our specific error codes.
Patch is attached. It may look big (111 files changed), but there are only few insertions and deletions in each.
Thanks, Michal
Rebased version attached.
Michal
On (16/07/14 16:37), Michal Židek wrote:
On 05/11/2014 11:10 PM, Jakub Hrozek wrote:
I haven't tested this patch to be honest, but then again, there's not so much to test except compiling and a bit of sanity testing..
I prefer Michal's version of using sss_strerror explicitly instead of the route Simo proposed to #define strerror as sss_strerror simply because I prefer to see what function gets called right away. The drawback of mass-converting to sss_strerror is that we lose git-blame history somewhat...but given that we have just recently converted the DEBUG messages and strerror is mostly used only around DEBUG messages, I don't think we'd lose much metadata.
But I'd like to hear other opinions as well.
On Thu, Apr 24, 2014 at 7:03 PM, Michal Židek mzidek@redhat.com wrote:
Hello,
this patch replaces strerror with sss_strerror on some places. I think it would be OK to use always sss_strerror, but to keep the patch relatively small I left strerror on places where we directly print value of errno or return value of some third party functions that do not (and never will) return our specific error codes.
Patch is attached. It may look big (111 files changed), but there are only few insertions and deletions in each.
Thanks, Michal
Rebased version attached.
Michal
From fce8ba028a10087c2e71781653b65cad733aef25 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Thu, 24 Apr 2014 17:26:37 +0200 Subject: [PATCH] Use sss_strerror instead of strerror.
Makefile.am | 9 +--
//snip
111 files changed, 479 insertions(+), 446 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 8d3d366..c9defa8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1290,7 +1290,7 @@ resolv_tests_LDADD = \ $(SSSD_LIBS) \ $(CHECK_LIBS) \ $(CARES_LIBS) \
- libsss_debug.la \
- $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
refcount_tests_SOURCES = \ @@ -2347,12 +2347,12 @@ krb5_child_CFLAGS = \ $(POPT_CFLAGS) \ $(KRB5_CFLAGS) krb5_child_LDADD = \
- libsss_debug.la \ $(TALLOC_LIBS) \ $(POPT_LIBS) \ $(DHASH_LIBS) \ $(KRB5_LIBS) \
- $(CLIENT_LIBS)
- $(CLIENT_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
On 07/16/2014 04:42 PM, Lukas Slebodnik wrote:
On (16/07/14 16:37), Michal Židek wrote:
On 05/11/2014 11:10 PM, Jakub Hrozek wrote:
I haven't tested this patch to be honest, but then again, there's not so much to test except compiling and a bit of sanity testing..
I prefer Michal's version of using sss_strerror explicitly instead of the route Simo proposed to #define strerror as sss_strerror simply because I prefer to see what function gets called right away. The drawback of mass-converting to sss_strerror is that we lose git-blame history somewhat...but given that we have just recently converted the DEBUG messages and strerror is mostly used only around DEBUG messages, I don't think we'd lose much metadata.
But I'd like to hear other opinions as well.
On Thu, Apr 24, 2014 at 7:03 PM, Michal Židek mzidek@redhat.com wrote:
Hello,
this patch replaces strerror with sss_strerror on some places. I think it would be OK to use always sss_strerror, but to keep the patch relatively small I left strerror on places where we directly print value of errno or return value of some third party functions that do not (and never will) return our specific error codes.
Patch is attached. It may look big (111 files changed), but there are only few insertions and deletions in each.
Thanks, Michal
Rebased version attached.
Michal
From fce8ba028a10087c2e71781653b65cad733aef25 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Thu, 24 Apr 2014 17:26:37 +0200 Subject: [PATCH] Use sss_strerror instead of strerror.
Makefile.am | 9 +--
//snip
111 files changed, 479 insertions(+), 446 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 8d3d366..c9defa8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1290,7 +1290,7 @@ resolv_tests_LDADD = \ $(SSSD_LIBS) \ $(CHECK_LIBS) \ $(CARES_LIBS) \
- libsss_debug.la \
- $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la
refcount_tests_SOURCES = \ @@ -2347,12 +2347,12 @@ krb5_child_CFLAGS = \ $(POPT_CFLAGS) \ $(KRB5_CFLAGS) krb5_child_LDADD = \
- libsss_debug.la \ $(TALLOC_LIBS) \ $(POPT_LIBS) \ $(DHASH_LIBS) \ $(KRB5_LIBS) \
- $(CLIENT_LIBS)
- $(CLIENT_LIBS) \
- $(SSSD_INTERNAL_LTLIBS)
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
On Wed, 2014-08-27 at 16:47 +0200, Jakub Hrozek wrote:
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
This is fine and reasonable.
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
I think I really only care that strerror is not used if we standardize on sss_strerror to avoid having "oops fis strerror" commits later, so if you prefer just adding the macro above, it is fine by me.
Simo.
On (27/08/14 16:47), Jakub Hrozek wrote:
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
and you may forget to include such header file and you will use real function from glibc.
LS
On Wed, 2014-08-27 at 17:00 +0200, Lukas Slebodnik wrote:
On (27/08/14 16:47), Jakub Hrozek wrote:
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
We decided to use $(NULL) at the end of lists in Makefile.am. So you will not change two lines with adding new library next time. see 1746e8b8399da2a7a8da4aace186f66055ccfec1
LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
and you may forget to include such header file and you will use real function from glibc.
Should be in a header file that is included everywhere.
Simo.
On (27/08/14 11:08), Simo Sorce wrote:
On Wed, 2014-08-27 at 17:00 +0200, Lukas Slebodnik wrote:
On (27/08/14 16:47), Jakub Hrozek wrote:
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote:
>We decided to use $(NULL) at the end of lists in Makefile.am. So you will not >change two lines with adding new library next time. >see 1746e8b8399da2a7a8da4aace186f66055ccfec1 > >LS
Ok. I added it where I modified the end of the lists.
Michal
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
and you may forget to include such header file and you will use real function from glibc.
Should be in a header file that is included everywhere.
You cannot be sure, because there can be another library with the same HACK in header file.
//from our header file #idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
//from another header file which is included indirectly. #idef strerror #undef strerror #endif #define strerror(err) strerror_l((err), locale)
I just want to say that this kind of hacks are dangerous.
the 2nd problem is that Michal didn't replace all occurences of strerror. He didn't replace strerror with sss_strerror after libc functions
For example: src/util/domain_info_utils.c:433: "fclose failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:508: "fclose failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:516: "rename failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:526: "fchmod failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:544: "fclose failed [%d][%s].\n", err, strerror(err));
LS
On Wed, 2014-08-27 at 17:25 +0200, Lukas Slebodnik wrote:
On (27/08/14 11:08), Simo Sorce wrote:
On Wed, 2014-08-27 at 17:00 +0200, Lukas Slebodnik wrote:
On (27/08/14 16:47), Jakub Hrozek wrote:
On Wed, Aug 27, 2014 at 08:48:14AM -0400, Simo Sorce wrote:
On Wed, 2014-08-27 at 14:30 +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:19:54PM +0200, Michal Židek wrote: > >We decided to use $(NULL) at the end of lists in Makefile.am. So you will not > >change two lines with adding new library next time. > >see 1746e8b8399da2a7a8da4aace186f66055ccfec1 > > > >LS > > Ok. I added it where I modified the end of the lists. > > Michal >
Simo,
I know you're normally against mass-converting anything in the code, but I think this conversion actually makes sense. We converted the DEBUG messages only recently with Nikolai's patches and strerror/sss_strerror is pretty much only used around DEBUG messages, so the context is already 'tainted'..
If you're OK with this mass-conversion, I'd like to push this patch to master and keep using sss_strerror only from now on.
You described my preferences quite well :)
My question is, given it seem we have to use sss_strerror() everywhere why don't we simply ban the use of strerror() by redefining it to sss_strerror() ? If that would be too confusing I would like at least this macro be added to the header file:
#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
Simo.
Simply because I don't like code obfuscation :-) If the code says it's calling strerror I'd like it to call strerror..
But I don't feel too strongly about this issue, as long as the code prefers sss_strerror, I'm fine.
and you may forget to include such header file and you will use real function from glibc.
Should be in a header file that is included everywhere.
You cannot be sure, because there can be another library with the same HACK in header file.
//from our header file#idef strerror #undef strerror #endif #define strerror(err) __ERROR__USE__SSS_STRERROR_INSTEAD__;
//from another header file which is included indirectly. #idef strerror #undef strerror #endif #define strerror(err) strerror_l((err), locale)
I just want to say that this kind of hacks are dangerous.
This "hack" is just a warning, it does not absolve the reviewer from nacking a patch that re-introduces strerror, it just helps the developer knowing he is not supposed to use it.
It is not perfect, but better than nothing IMO.
the 2nd problem is that Michal didn't replace all occurences of strerror. He didn't replace strerror with sss_strerror after libc functions
For example: src/util/domain_info_utils.c:433: "fclose failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:508: "fclose failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:516: "rename failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:526: "fchmod failed [%d][%s].\n", ret, strerror(ret)); src/util/domain_info_utils.c:544: "fclose failed [%d][%s].\n", err, strerror(err));
This should be fixed, indeed.
Simo.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Simo Sorce