[syslog-ng] Re: syslog-ng Digest, Vol 6, Issue 10

[Roberto Nibali]

> This was done to remain consistent with the existing code.  Do you want
> me to enlarge the scope of the patch to change the existing instances of
> this as well?

Actually, come to think of it, you have done a patch against 1.6.x 
sources, which I suppose are in maintenance mode only, so your patch has 
a higher chance of being integrated in the 1.9.x code. Regarding the 
isdigit() conversion, a sudden portability issue stroke me but opon 
reading the C standard I find that it's ANSI/ISO C C89,C99 and POSIX 
1003.2-1992; 1003.1-2001 compatible.

>>> +                        while (left && *src >= '0' && *src <= '9') {
>> Consider isdigit(3) or parse up to *src == ']'
> 
> I avoided parsing up to ']' to avoid the unlikely situation where someone
> sends us a mal formatted log entry.  If for some reason I get a message
> like "foo[135xyz]", I don't want the xyz part, even though the 135 part
> is probably garbage anyway.

Maybe I needed sleep more than you did ;). But maybe one improvement 
could be you honoring the fact that PIDs are 16bit. So parsing 5 chars 
should be enough as an early continue out of the loop.

>>> 	self->msg = c_format_cstring("syslog-ng[%i]: %s", getpid(), >length,
>> data);
>>> 	self->program = c_format_cstring("syslog-ng");
>>> +	self->pid = c_format_cstring("%i", getpid());
>> This is wrong! You most certainly don't want to add the pid of the
>> receiving syslog-ng process.
> 
> seen out of context, that part of the patch is deceptive and misleading
> at first glance, but I am pretty certain it is correct.  This is part of
> the make_internal_message() function.  Notice the existing call to
> getpid() a few lines up where self->msg is being constructed.

Correct again. Sorry for not checking more closely; you're right, the 
patchlet was misleading, not enough context. However, I think the 
getpid() call could be cached, as it's "quite" slow on a lot of systems 
and calling it for each internal queued message is unnecessary.

>> Whitespace/tab damage.
> 
> oops!  I probably have broken .vimrc settings.

Hmm, but then it should affect all the lines and from what I could spot 
only the line quoted was damaged.

> Also, is macros.gperf an automatically generated file?  I wasn't sure if
> I needed to patch that part.

IIRC you should also recreate macros.gperf, but I'm not sure.

Bazsi, do you fancy this patch? I personally have no use for a PID macro 
extension because it's contained in the MSG macro but it seems a couple 
of people would really love this. And it doesn't hurt anyone.

Jason, would you be willing to also patch the 1.9.x code based on what 
I've sent in earlier?

Best regards and thanks for the discussion,
Roberto Nibali, ratz
-- 
echo '[q]sa[ln0=aln256%Pln256/snlbx]sb3135071790101768542287578439snlbxq'|dc