Re: changes to fhandler_process.cc from 02/06/2002 should be reverted

["Chris January" <chris at atomice dot net>]

> >> >I've just seen this ChangeLog entry, Chris:
> >> >
> >> >2002-06-02  Christopher Faylor  <cgf@redhat.com>
> >> >
> >> > Remove unneeded sigproc.h includes throughout.
> >> > * fhandler.h (fhandler_proc::fill_filebuf): Take a pinfo argument.
> >> > * fhandler_proc.cc (fhandler_proc::get_proc_fhandler): Simplify
search
> >> > for given pid.
> >> > (fhandler_proc::readdir): Assume that pid exists if it shows up in
the
> >> > winpid list.
> >> > * fhandler_process.cc (fhandler_process::open): Simplify search for
> >> > given pid.  Call fill_filebuf with pinfo argument.
> >> > (fhandler_process::fill_filebuf): Pass pinfo here and assume that it
> >> > exists.
> >> > * pinfo.h (pinfo::remember): Define differently if sigproc.h is not
> >> > included.
> >> >
> >> >IMHO, these changes need to be reverted. fhandler_base::fill_filebuf
is
> >> >virtual. If you add the pinfo parameter to
> >fhandler_process::fill_filebuf,
> >> >then you are defining a new function, not overriding the one in
> >> >fhandler_base. Hence, /proc semantics whereby the file contents are
> >> >refreshed on an lseek are broken.
> >>
> >> I'll certainly consider changes, but your previous method of searching
> >> the whole process table for a given pid when there already is a method
> >> available for directly getting to the pid itself was flawed.  You used
> >> this technique throughout your proc code and I thought it demonstrated
> >> an unfamiliarity with the way that the pinfo class was supposed to
work,
> >> so I fixed it.
> >>
> >> I will put back the pinfo pointer in the fhandler_process class but I
> >> don't think that the entire checkin evidenced by the ChangeLog above
> >> needs to be reverted.
> >I took a look at your changes and this still won't work. Look at where
> >fill_filebuf is called in fhandler_virtual::lseek. The p member of
> >fhandler_process must be valid at this point, but it is not because the
code
> >in fhandler_process::open sets it back to NULL after it has called
> >fill_filebuf. The reason for calling fill_filebuf in lseek is that this
is
> >how the Linux proc utilities work - they open the file and then call seek
> >(fd, 0, SEEK_SET) when they want the file contents updated.
>
> Ok, but you can't keep the shared memory for every process open for the
> duration of the life of a fhandler_process.  I don't know how to deal with
> this but using up lots of resources isn't the way to do it.
Agreed, but the current code in CVS will actually crash when lseek is
called. Incidentally, I believe that line 158 in fhandler_process.cc can be
removed.
My preferred solution to this would be to save the pid as the original code
did and add pinfo p (pid) in fill_filebuf.

Chris