2016-05-03 1:19 GMT+03:00 Mark Kettenis <mark.kette...@xs4all.nl>:
>> Date: Tue, 3 May 2016 00:04:13 +0200
>> From: Alexander Bluhm <alexander.bl...@gmx.net>
>>
>> On Sun, May 01, 2016 at 01:24:19AM +0300, Vadim Zhukov wrote:
>> > When matching by PID, we'd better return ESRCH expicitly rather
>> > than zero entries. This makes, for example, fstat(1) behaviour
>> > more consistent and makes it easier to replace "if lsof -p ..."
>> > checks in third-party code with "if fstat -p ...".
>> >
>> > For libkvm, I explicitly test for ESRCH to avoid going inside
>> > kmem without real need.
>> >
>> > Tested on amd64.
>> >
>> > Comments? Okays?
>>
>> Your use case makes sense. So I think this should go in.
>
> The code change is abit convoluted though though. It's enough to
> initialize matched to 0 before the LIST_FOREACH and set it
> unconditionally to after the
>
> if (arg > 0 && pr->ps_pid != (pid_t)arg)
>
> check. So:
>
> matched = 0
> LIST_FOREACH(pr, &allprocess, ps_list) {
> /*
> * skip system, exiting, embryonic and undead
> * processes
> */
> if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO |
> PS_EXITING))
> continue;
> if (arg > 0 && pr->ps_pid != (pid_t)arg) {
> /* not the pid we are looking for */
> continue;
> }
> matched = 1;
I think you're right. I've modelled the kernel code after libkvm, and
your suggestion makes it much easier to read, thanks!
I'll send updated diff a bit later, have to close lid right now. Thanks!
>> > Index: sys/kern/kern_sysctl.c
>> > ===================================================================
>> > RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
>> > retrieving revision 1.300
>> > diff -u -p -r1.300 kern_sysctl.c
>> > --- sys/kern/kern_sysctl.c 29 Feb 2016 19:44:07 -0000 1.300
>> > +++ sys/kern/kern_sysctl.c 30 Apr 2016 22:26:06 -0000
>> > @@ -1225,7 +1225,7 @@ sysctl_file(int *name, u_int namelen, ch
>> > struct process *pr;
>> > size_t buflen, elem_size, elem_count, outsize;
>> > char *dp = where;
>> > - int arg, i, error = 0, needed = 0;
>> > + int arg, i, error = 0, needed = 0, matched;
>> > u_int op;
>> > int show_pointers;
>> >
>> > @@ -1325,6 +1325,7 @@ sysctl_file(int *name, u_int namelen, ch
>> > error = EINVAL;
>> > break;
>> > }
>> > + matched = (arg <= 0);
>> > LIST_FOREACH(pr, &allprocess, ps_list) {
>> > /*
>> > * skip system, exiting, embryonic and undead
>> > @@ -1332,9 +1333,12 @@ sysctl_file(int *name, u_int namelen, ch
>> > */
>> > if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO |
>> > PS_EXITING))
>> > continue;
>> > - if (arg > 0 && pr->ps_pid != (pid_t)arg) {
>> > - /* not the pid we are looking for */
>> > - continue;
>> > + if (arg > 0) {
>> > + /* is this the pid we are looking for? */
>> > + if (pr->ps_pid == (pid_t)arg)
>> > + matched = 1;
>> > + else
>> > + continue;
>> > }
>> > fdp = pr->ps_fd;
>> > if (pr->ps_textvp)
>> > @@ -1353,6 +1357,8 @@ sysctl_file(int *name, u_int namelen, ch
>> > FILLIT(fp, fdp, i, NULL, pr);
>> > }
>> > }
>> > + if (!matched)
>> > + error = ESRCH;
>> > break;
>> > case KERN_FILE_BYUID:
>> > LIST_FOREACH(pr, &allprocess, ps_list) {
>> > Index: lib/libkvm/kvm_file2.c
>> > ===================================================================
>> > RCS file: /cvs/src/lib/libkvm/kvm_file2.c,v
>> > retrieving revision 1.47
>> > diff -u -p -r1.47 kvm_file2.c
>> > --- lib/libkvm/kvm_file2.c 4 Sep 2015 02:58:14 -0000 1.47
>> > +++ lib/libkvm/kvm_file2.c 30 Apr 2016 22:26:06 -0000
>> > @@ -149,7 +149,7 @@ kvm_getfiles(kvm_t *kd, int op, int arg,
>> > /* find size and alloc buffer */
>> > rv = sysctl(mib, 6, NULL, &size, NULL, 0);
>> > if (rv == -1) {
>> > - if (kd->vmfd != -1)
>> > + if (errno != ESRCH && kd->vmfd != -1)
>> > goto deadway;
>> > _kvm_syserr(kd, kd->program, "kvm_getfiles");
>> > return (NULL);
>> > @@ -266,7 +266,7 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
>> > {
>> > size_t buflen;
>> > struct nlist nl[4], *np;
>> > - int n = 0;
>> > + int n = 0, matched = 0;
>> > char *where;
>> > struct kinfo_file kf;
>> > struct file *fp, file;
>> > @@ -312,6 +312,9 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
>> > kd->filebase = (void *)where;
>> > buflen = (nfiles + 10) * esize;
>> >
>> > + if (op != KERN_FILE_BYPID || arg <= 0)
>> > + matched = 1;
>> > +
>> > for (pr = LIST_FIRST(&allprocess);
>> > pr != NULL;
>> > pr = LIST_NEXT(&process, ps_list)) {
>> > @@ -333,10 +336,12 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
>> > goto cleanup;
>> > }
>> >
>> > - if (op == KERN_FILE_BYPID && arg > 0 &&
>> > - proc.p_pid != (pid_t)arg) {
>> > - /* not the pid we are looking for */
>> > + if (op == KERN_FILE_BYPID) {
>> > + /* check if this is the pid we are looking for */
>> > + if (arg > 0 && proc.p_pid != (pid_t)arg)
>> > continue;
>> > + else
>> > + matched = 1;
>> > }
>> >
>> > if (KREAD(kd, (u_long)process.ps_ucred, &ucred)) {
>> > @@ -457,6 +462,10 @@ kvm_deadfile_byid(kvm_t *kd, int op, int
>> > buflen -= esize;
>> > n++;
>> > }
>> > + }
>> > + if (!matched) {
>> > + errno = ESRCH;
>> > + goto cleanup;
>> > }
>> > done:
>> > *cnt = n;
>>
>>
--
WBR,
Vadim Zhukov
