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

[Hannah S. <han...@1u...>]

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?

>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.

>> 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.

Kind regards,

Hannah.