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