On Thu, Oct 06, 2005 at 09:10:41PM +0200, syslog-ng-request@lists.balabit.hu wrote:
+ if (left && *src >= '0' && *src <= '9') {
Consider isdigit(3)
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?
+ 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.
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.
Whitespace/tab damage.
oops! I probably have broken .vimrc settings. Also, is macros.gperf an automatically generated file? I wasn't sure if I needed to patch that part. -jason pepas
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
ERRATUM:
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.
And I'm wrong again, your patch does everything in a perfect way. It (the initial self pointer struct) is only filled up once at invocation. It wouldn't hurt my judgment to check the source from time to time ... My apologies, Roberto Nibali, ratz -- echo '[q]sa[ln0=aln256%Pln256/snlbx]sb3135071790101768542287578439snlbxq'|dc
On Fri, 07 Oct 2005 23:59:27 +0200, Roberto Nibali said:
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.
Erm. No. On a Linux 2.6 kernel, in include/linux/threads.h: /* * This controls the default maximum pid allocated to a process */ #define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) /* * A maximum of 4 million PIDs should be enough for a while: */ #define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT)) On 32-bit kernels, there's some code in the /proc that borks up if the PID is over 64K (it invents an inode number based on the pid, and the left-shift overflows). Running on a 64 bit system, the default is 4M, and can be raised *much* higher if needed. And it isn't just Linux either - seen on an AIX box: # cut -f2 -d'[' /var/adm/seclog | cut -f1 -d']' | sort -nr | uniq | head 191446 191208 191188 191186 191184 191170 191124 191122 191108 191104 Yep, PIDs pushing the 200K mark (AIX plays some funky games when generating the 'next PID' to reduce cache-line aliasing issues when dereferencing entries in some structures indexed by PID).
Erm. No.
On a Linux 2.6 kernel, in include/linux/threads.h:
Well, not all 2.6 kernels. Although 2.6.5 SuSE spits following: /* * This controls the default maximum pid allocated to a process */ #define PID_MAX_DEFAULT 0x8000 /* * A maximum of 4 million PIDs should be enough for a while: */ #define PID_MAX_LIMIT (4*1024*1024) A simple stupid test would wrap the pids at 32k: #include <stdio.h> #include <stdlib.h> #include <sys/types.h> #include <unistd.h> #include <errno.h> #define SECS_TO_SLEEP 10 int main(int argc, char *argv[]) { unsigned long i; unsigned long loop; pid_t pid; loop = 0; if (argc > 1) { loop = strtoul(argv[1], NULL, 10); } else { printf("%s NUM_OF_FORKS\n", argv[0]); } for (i = 0; i < loop; i++) { pid = fork(); if (pid < 0) { perror("fork"); exit(EXIT_FAILURE); } else if (pid > 0) { /* child */ fprintf(stderr, "forked pid: %d\n", pid); usleep(SECS_TO_SLEEP * 1024 * 1024); exit(EXIT_SUCCESS); } i++; } exit(EXIT_SUCCESS); }
On 32-bit kernels, there's some code in the /proc that borks up if the PID is over 64K (it invents an inode number based on the pid, and the left-shift overflows). Running on a 64 bit system, the default is 4M, and can be raised *much* higher if needed.
I knew that you could have 4Mio threads but I somehow wasn't aware that you can have 4Mio forked processes. Since the Linux world changed to NPTL we do not necessarily call clone() anymore when doing a pthread_create() and thus we don't really fork() anymore, as with linux_threads-0.10. On my system, despite starting threaded applications, I get the tgrp == pid (ps -AL for example), since my glibc uses linux_threads and thus LWPs: ratz@webphish:~> /lib/libc.so.6 GNU C Library stable release version 2.3.3 (20040325), by Roland McGrath et al. Copyright (C) 2004 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Configured for i586-suse-linux. Compiled by GNU CC version 3.3.3 (SuSE Linux). Compiled on a Linux 2.6.4 system on 2004-04-05. Available extensions: GNU libio by Per Bothner crypt add-on version 2.1 by Michael Glad and others linuxthreads-0.10 by Xavier Leroy GNU Libidn by Simon Josefsson NoVersion patch for broken glibc 2.0 binaries BIND-8.2.3-T5B libthread_db work sponsored by Alpha Processor Inc NIS(YP)/NIS+ NSS modules 0.19 by Thorsten Kukuk Report bugs using the `glibcbug' script to <bugs@gnu.org>.
And it isn't just Linux either - seen on an AIX box:
# cut -f2 -d'[' /var/adm/seclog | cut -f1 -d']' | sort -nr | uniq | head 191446 191208 191188 191186 191184 191170 191124 191122 191108 191104
Mhh, these must be PIDs I reckon, since IIRC AIX can only handle 512 threads per process, right?
Yep, PIDs pushing the 200K mark (AIX plays some funky games when generating the 'next PID' to reduce cache-line aliasing issues when dereferencing entries in some structures indexed by PID).
Thank you, Valdis, of course. Since I haven't actively remarked high PIDs in my current development field I was still under the distinct impression that PIDs are 16bit; I probably just overlooked the number of digits in the logfiles all those years. That about concludes my ability to comment on Jason's patch ;). Best regards, Roberto Nibali, ratz -- echo '[q]sa[ln0=aln256%Pln256/snlbx]sb3135071790101768542287578439snlbxq' | dc
On Sat, 08 Oct 2005 11:02:08 +0200, Roberto Nibali said:
A simple stupid test would wrap the pids at 32k:
Right. The fun starts when you do 'echo NNN > /proc/sys/kernel/pid_max' for some NNN > 32k. On 32bit boxes 64K is the limit due to the /proc inode wonkiness, but on 64bit kernels it's just a matter of how much RAM you have. ;)
I knew that you could have 4Mio threads but I somehow wasn't aware that you can have 4Mio forked processes. Since the Linux world changed to NPTL we do not necessarily call clone() anymore when doing a pthread_create() and thus we don't really fork() anymore, as with linux_threads-0.10.
The numbers returned by getpid() and gettid() come out of the same PID space, and the vast majority of programs aren't thread-aware, and get their PID via fork() or vfork() anyhow...
# cut -f2 -d'[' /var/adm/seclog | cut -f1 -d']' | sort -nr | uniq | head 191446 191208 191188 191186 191184 191170 191124 191122 191108 191104
Mhh, these must be PIDs I reckon, since IIRC AIX can only handle 512 threads per process, right?
Yep. Those are indeed process IDs, coming out of binaries that I *know* aren't multi-threaded...
Thank you, Valdis, of course. Since I haven't actively remarked high PIDs in my current development field I was still under the distinct impression that PIDs are 16bit; I probably just overlooked the number of digits in the logfiles all those years.
It's effectively an 'int' - any code that assumes a smaller range will fail in the most interesting ways in the near future... :)
Right. The fun starts when you do 'echo NNN > /proc/sys/kernel/pid_max' for some NNN > 32k. On 32bit boxes 64K is the limit due to the /proc inode wonkiness, but on 64bit kernels it's just a matter of how much RAM you have. ;)
And I guess this is depending on the selected frame size per process.
The numbers returned by getpid() and gettid() come out of the same PID space, and the vast majority of programs aren't thread-aware, and get their PID via fork() or vfork() anyhow...
True, to get the thread_id you either need to extract it via the POSIX pthread_* calls or, in case of Linux, you can use the non-portable gettid().
It's effectively an 'int' - any code that assumes a smaller range will fail in the most interesting ways in the near future... :)
Indeed. Thank you for your explanations. Best regards, Roberto Nibali, ratz -- echo '[q]sa[ln0=aln256%Pln256/snlbx]sb3135071790101768542287578439snlbxq'|dc
participants (3)
-
Jason Pepas
-
Roberto Nibali
-
Valdis.Kletnieks＠vt.edu