Date: Sat, 15 Dec 2012 18:28:04 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org>, John Baldwin <jhb@freebsd.org> Subject: Re: svn commit: r244154 - head/bin/ps Message-ID: <20121215171150.U1295@besplex.bde.org> In-Reply-To: <20121215011300.GN71906@kib.kiev.ua> References: <201212121545.qBCFj4Hl086444@svn.freebsd.org> <20121212210652.GO3013@kib.kiev.ua> <20121213111240.GB1381@garage.freebsd.pl> <201212141152.15567.jhb@freebsd.org> <20121214214246.GB1411@garage.freebsd.pl> <20121215011300.GN71906@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 15 Dec 2012, Konstantin Belousov wrote: > On Fri, Dec 14, 2012 at 10:42:47PM +0100, Pawel Jakub Dawidek wrote: >> On Fri, Dec 14, 2012 at 11:52:15AM -0500, John Baldwin wrote: >>> On Thursday, December 13, 2012 6:12:44 am Pawel Jakub Dawidek wrote: >>>> On Wed, Dec 12, 2012 at 11:06:52PM +0200, Konstantin Belousov wrote: >>>>> On Wed, Dec 12, 2012 at 03:45:04PM +0000, Pawel Jakub Dawidek wrote: >>>>>> Author: pjd >>>>>> Date: Wed Dec 12 15:45:03 2012 >>>>>> New Revision: 244154 >>>>>> URL: http://svnweb.freebsd.org/changeset/base/244154 >>>>>> >>>>>> Log: >>>>>> Use kern.max_pid sysctl to obtain maximum PID number instead of using local >>>>>> define. >>>>> It is pid_max, not max_pid. >>>>> >>>>> But the change is wrong. The kern.pid_max only limits newly allocated pids, >>>>> it does not magically moves existing pids, which are out of range, to the >>>>> limited region. See the corresponding commit log for the description. >>>>> It was added to make it easier to run FreeBSD 1.x binaries on the modern >>>>> kernels. >>>> >>>> I saw CTLFLAG_TUN on the sysctl and assumed it is read-only... >>>> How about defining BSD_PID_MAX in sys/proc.h, which would be visible by >>>> userland as well and setting PID_MAX to BSD_PID_MAX? >>>> >>>> This would also help bsnmpd. >>>> >>>> http://people.freebsd.org/~pjd/patches/PID_MAX.patch >>> >>> This doesn't help your actual use case though where you want to boot a kernel >>> with a different PID_MAX. I would much rather our tools learn such constants >>> from the kernel via sysctl than have them compiled in. So, I would add a new >>> sysctl which exports the true PID_MAX constant (and is read-only and never >>> changes) and use that in ps, etc. >> >> In that case I'd prefer to make existing kern.pid_max sysctl read-only >> and make it loader tunable. I don't expect there are many users of this >> sysctl... >> > No, I described you the purpose of the sysctl. Requiring reboot just for > running the old binaries is not useful. Please do not break it. ps doesn't make any useful use of either its BSD_PID_MAX or the kernel's PID_MAX anyway. Its only use is to break cases where these BSD_PID_MAX is smaller than the largest active pid. There are some useful uses of a limit: - to "optimize" the parsing function by not supporting pids larger than necessary - for formatting pids. The 99999 limit is useful for restricting the width of pid fields. With a dynamic limit, you need complications for dynamic widths. With a larger fixed limit, you need complications for dyamic widths anyway, since after expanding the pid fields there is no longer enough space for fixed widths in other fields. BTW, someone broke the formatting for ruptime(1) in -current. It now uses a fixed and _far too large_ width for the host name, so that most lines have length precisely 80 and get double-spaced by auto=linefeed on 80-column terminals. "ps l" still uses < 80 columns, with 2 5-digit fields for pids. Wide fields still eat into the field width for the COMMAND field. Interestingly, someone improved the formatting for the UID field -- it now seems to be dynamic and usually takes 3-4 columns, where it used to have the fixed width 5. ps of course doesn't use the limit for these useful uses: % #define BSD_PID_MAX 99999 /* Copy of PID_MAX from sys/proc.h. */ % static int % addelem_pid(struct listinfo *inf, const char *elem) % { % char *endp; % long tempid; % % if (*elem == '\0') { % warnx("Invalid (zero-length) process id"); % optfatal = 1; % return (0); /* Do not add this value. */ % } % % errno = 0; % tempid = strtol(elem, &endp, 10); Here ps assumes that the limit is < LONG_MAX. A reasonable assumption. % if (*endp != '\0' || tempid < 0 || elem == endp) { % warnx("Invalid %s: %s", inf->lname, elem); % errno = ERANGE; % } else if (errno != 0 || tempid > BSD_PID_MAX) { % warnx("%s too large: %s", inf->lname, elem); % errno = ERANGE; % } All ps does with BSD_PID_MAX is to give an up-front error for values that are so large that they can't match any pids. It would be harmless for it to simply accept all representable values and let them not match any actual pid later. Example uses: - ps -p 0,745,756: all 3 pids are accepted - ps -p -1: -1 is rejected because it gives tempid < -1 - ps -p 99999: 99999 is accepted because it is at the limit - ps -p 999999: 999999 is rejected because it is above the limit % if (errno == ERANGE) { % optfatal = 1; % return (0); % } % if (inf->count >= inf->maxcount) % expand_list(inf); % inf->l.pids[(inf->count)++] = tempid; l.pids[N] has type pid_t. We assume that the long tempid is representable as a pid. This is a valid assumption due to our earlier checks. 99999 is smaller than 2**31, so it fits in pid_t because pid_t happens to be int32_t. This would not necessarily be true of the limit were dynamic. The kern.pid_max sysctl actually returns an int. It makes sort of the opposite assumption -- that the max pid (which could reasonably be the max value representable by pid_t) in an int, instead of that the max value that we allow fits in a pid_t. Everything more or less assumes 32 bit ints and pid_t's. Anyway, a better value for BSD_PID_MAX is the maximum value representable by a pid_t. This has little to do with PID_MAX. We should use it just to ensure that tempid is representable in l.pids[]. % return (1); % } % #undef BSD_PID_MAX ps has not-quite-similar checks for UID_MAX and GID_MAX. I happen to have grepped ~100 utilities recently for their uid/gid parsing. I didn't find a single case of fully correct id parsing, but found many variations in the bugs. Their parsing in ps is better than in most places. UID_MAX and GID_MAX are now on the full max's for the types, so using them corresponds to the representability checks that I suggested above. Except UID_MAX and GID_MAX are not documented in any man page, so they shouldn't be assumed to have these values. And since uid_t and gid_t are unsigned types, it is very easy to spell their values portably as ((uid_t)-1) and ((gid_t)-1), respectively, so there is even less need for UID_MAX and GID_MAX than for PID_MAX. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121215171150.U1295>