Re: [Log4cplus-devel] Rest of the patches (was Re: log4cplus 1.0.4-RC8 released)

[Václav H. <v.h...@sh...>]

On Mon, 7 Jun 2010 12:57:59 +0200, Hannah Schroeter wrote:
> Hi!
> 
> On Thu, Jun 03, 2010 at 11:16:30AM +0200, Václav Haisman wrote:
> 
>>On Wed, 2 Jun 2010 16:06:21 +0200, Hannah Schroeter wrote:
> 
>>> On Wed, Jun 02, 2010 at 12:01:33AM +0200, Václav Haisman wrote:
>>>>[...]
> 
>>>>Yes, I will consider any patches you send.
> 
>>>>[...]
> 
>>>>The best for me is if you base the patches on the HEAD revision of
e.g.
>>>>PRODUCTION_1_0_x branch. But it does not matter much, I can deal with
>>>>patches
>>>>against release source as well.
> 
>>> No problem, rebasing them wasn't difficult.
> 
>>>>> Based on your feedback, I could send you the other patches which
would
>>>>> encompass another fix for a possible problem we noticed here in our
>>>>> company, and a few feature additions we've come to depend upon here.
> 
>>>>Please send the patches. I will consider each one of them separately.
>>Use
>>>>either the mailing list or the patches tracker on SF, whichever is
more
>>>>comfortable.
> 
>>> Sending them here.
> 
>>> Little explanations.
> 
>>> 0001 seems to fix a bug for the syslog appender in some cases (if
either
>>> log4cplus uses wstrings and thus LOG4CPLUS_TSTRING_TO_STRING returns a
>>> temporary, or the identifier is derived from a property, so the
>>> identifier is always a temporary). At least glibc always stores the
>>> char* you pass it, instead of copying it by itself.
> 
>>Applied with minor changes.
> 
> Thanks.
> 
>>> 0002 is a feature addition we needed in house. log4cplus already
>>> supports logging of the thread ID returned by pthread_self, however,
for
>>> us that wasn't as useful as the gettid() id, as they're different and
ps
>>> shows only the latter (Linux uses 1:1 threading).
> 
>>I am rejecting this patch. I am not against the idea of this but the
>>implementation needs some polishing first.
> 
>>The thread ID needs to be part of the InternalLoggingEvent instance
>>because the events can be passed around between threads and the thread
>>that
>>is formatting the output does not have to be the same thread in which
the
>>event has occurred.
> 
> Our patch seemed to work for us. So in which cases is the event passed
> to different threads before finally being logged?
There is not a place in 1.0.x source that passes events between threads.
There is such place in 1.1.x (AsyncAppender). And I know there are people
with who are using their own appenders in which case there is a possibility
they pass events between threads.

> 
>>The thread ID is also not passed through SocketAppender
>>but you can ignore that, it is a similar situation as with %h/%H format
>>specifier for host name which does not make it through either.
> 
>>I think that %T should expand to the same string as %t does when the
given
>>OS does not have any meaningful gettid()-like function.
> 
> That's a valid concern indeed (as is probably your above one), so I
> think your decision to appreciate the idea but to reject the particular
> implementation in favor of a better one is valid.
> 
>>> 0003 is a feature addition analogous to acces the behavior of
>>> openlog itself, namely being able to pass the NULL pointer to openlog
to
>>> access a default.
> 
>>Applied with minor changes.
> 
>>> 0004 makes the syslog *facility* configurable, instead of only being
>>> able to use the default (LOG_USER).
> 
>>Applied with minor changes.
> 
> These two commits (r1352, r1353) introduce a useless use of "static" in
> the anonymous namespace. See the patch attached.
I agree that at first glance they are useless and go contrary to what the
C++ standard suggests. But they have a meaning. Symbols in anonymous
namespace have external linkage. In older Cygwin versions and on systems
that do not have GCC's -fvisibility=hidden support such symbols get
exported in shared objects/DLLs. Adding the static keyword removes them
from the exports.

> 
>>> 0005 makes it possible to limit the depth of the NDC in the pattern
>>> layout.
> 
>>I have not had the time to apply the patch, yet, but I think is it looks
>>ok and I will accept it.
> 
> Seems you've committed it in-between, at least on the PRODUCTION_1_0_x
> branch.
> 
>>I am going to work on the thread ID patch myself, using yours as a
>>starting point, but I am going release RC9 first with only the rest of
the
>>patches applied.
> 
>>Thanks.
> 
> Thank you too.

--
VH