[SSSD] [PATCH] Use sss_strerror instead of strerror

Simo Sorce simo at redhat.com
Wed Aug 27 15:53:32 UTC 2014


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.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list