[K42-discussion] process linux getfirst/next pid patch
Patrick Bozeman
PEBozeman at lbl.gov
Thu Sep 28 06:29:04 EST 2006
You are proposing to pass a restart pointer into a collection across a
PPC call without locking the collection, right?
If the collection is resized, or worse, if the element corresponding to
the restart pointer is removed before the next call to the iterator,
restarting the iteration becomes impossible and will probably crash the
server. I basically copied structure of this code from K42's existing
HashSimple::getNextWithFF which appears to have been designed to avoid
this very issue.
It seems to me that the iteration should continue to be based on logical id.
I'm not so sure it is a good idea to pass pointers from clients to
servers in general. The server has no good way of knowing if the client
passed it a bogus pointer or not (unless it is a pointer into a fixed
array), and enables a rogue client to bring down a server.
As Muli pointed out earlier in the thread, the first part of the restart
block has a snippet that says,
if (p) {
while (p) {
which is silly. I must have refactored this at some point and left in a superfluous conditional. The 'if (p)' should be removed.
I'll resubmit to fix the if(p) and clean up codes to use the range that was allocated to me, but it seems that we should leave the restart argument alone.
Dilma DaSilva wrote:
> Hi Patrick,
>
> I would like to suggest another way of iterating on the pids;
> if you prefer to keep what you have, let me know and I will apply your
> patch.
>
> My suggestion is to follow the kind of enumeration that we have in
> data structures such as lib/libc/misc/ListSimple.H through the
> method next(), which takes a opaque pointer as an argument and returns
> the "next" element in the list.
>
> For you enumaration, this would mean to have a method
> SysStatus getNextPID(void* &curr, pid_t &pid).
> If the method is called with curr == NULL, the implementation
> will return the first pid, and it will also update curr to store
> the "restart node" that your implementation searches for.
> The client of getNextPID can't look at curr; it will just pass
> it along on its next call when it wants to advance in the enumeration.
> What do you think?
>
> Thanks,
>
> Dilma
>
>
> Patrick Bozeman writes:
> > Enable enumeration of ProcessLinux pids.
> > diff -ru kitchsrc.orig/lib/libc/sys/ProcessLinuxClient.C kitchsrc/lib/libc/sys/ProcessLinuxClient.C
> > --- kitchsrc.orig/lib/libc/sys/ProcessLinuxClient.C 2006-02-28 08:56:43.000000000 -0800
> > +++ kitchsrc/lib/libc/sys/ProcessLinuxClient.C 2006-09-15 14:16:10.000000000 -0700
> > @@ -1760,6 +1760,20 @@
> > return 0;
> > }
> >
> > +/* virtual */ SysStatus
> > +ProcessLinuxClient::getFirstPID(pid_t& pid)
> > +{
> > + // no need to lock
> > + return stub._getFirstPID(pid);
> > +}
> > +
> > +/* virtual */ SysStatus
> > +ProcessLinuxClient::getNextPID(pid_t& pid)
> > +{
> > + // no need to lock
> > + return stub._getNextPID(pid);
> > +}
> > +
> > SysStatus
> > ProcessLinuxClient::setTimeOfDay(uval sec, uval usec)
> > {
> > diff -ru kitchsrc.orig/lib/libc/sys/ProcessLinuxClient.H kitchsrc/lib/libc/sys/ProcessLinuxClient.H
> > --- kitchsrc.orig/lib/libc/sys/ProcessLinuxClient.H 2006-02-28 08:56:43.000000000 -0800
> > +++ kitchsrc/lib/libc/sys/ProcessLinuxClient.H 2006-09-15 14:29:35.000000000 -0700
> > @@ -281,6 +281,9 @@
> > virtual SysStatus releaseCredsPointer(
> > ProcessLinux::creds_t* linuxCredsPtr);
> >
> > + virtual SysStatus getFirstPID(pid_t& pid);
> > + virtual SysStatus getNextPID(pid_t& pid);
> > +
> > virtual SysStatus setTimeOfDay(uval sec, uval usec);
> >
> > /*
> > diff -ru kitchsrc.orig/lib/libc/sys/ProcessLinux.H kitchsrc/lib/libc/sys/ProcessLinux.H
> > --- kitchsrc.orig/lib/libc/sys/ProcessLinux.H 2006-02-28 08:56:42.000000000 -0800
> > +++ kitchsrc/lib/libc/sys/ProcessLinux.H 2006-09-15 14:12:41.000000000 -0700
> > @@ -261,6 +261,15 @@
> > virtual SysStatus releaseCredsPointer(
> > ProcessLinux::creds_t* linuxCredsPtr)=0;
> >
> > + /* getFirstPID/getNextPID used for information support - to list
> > + * information about all known linux processes. For performance reasons,
> > + * results are NOT sorted by pid number.
> > + *
> > + * Returns success if value found, ENOENT if no more
> > + */
> > + virtual SysStatus getFirstPID(pid_t& pid)=0;
> > + virtual SysStatus getNextPID(pid_t& pid)=0;
> > +
> > virtual SysStatus setTimeOfDay(uval sec, uval usec)=0;
> >
> > /*
> > diff -ru kitchsrc.orig/os/servers/baseServers/ProcessLinuxServer.C kitchsrc/os/servers/baseServers/ProcessLinuxServer.C
> > --- kitchsrc.orig/os/servers/baseServers/ProcessLinuxServer.C 2006-02-28 08:59:32.000000000 -0800
> > +++ kitchsrc/os/servers/baseServers/ProcessLinuxServer.C 2006-09-15 14:25:04.000000000 -0700
> > @@ -1164,6 +1164,91 @@
> > return 0;
> > }
> >
> > +/* virtual */ SysStatus
> > +ProcessLinuxServer::getFirstPID(pid_t& pid)
> > +{
> > + return _getFirstPID(pid);
> > +}
> > +
> > +/* virtual */ SysStatus
> > +ProcessLinuxServer::_getFirstPID(__inout pid_t& pid)
> > +{
> > + AutoLock<BLock> as(&taskLock);
> > + task_struct *linux_task;
> > +
> > + linux_task = locked_next_task(0);
> > + if (!linux_task) {
> > + pid = 0;
> > + // FIXME: ibm needs to allocate error code
> > + return _SERROR(9000, 0, ENOENT);
> > + }
> > +
> > + pid = linux_task->pid;
> > + return 0;
> > +}
> > +
> > +/* virtual */ SysStatus
> > +ProcessLinuxServer::getNextPID(pid_t& pid)
> > +{
> > + return _getNextPID(pid);
> > +}
> > +
> > +/* _getNextPID's Basic structure is copied from HashSimple::getNextWithFF
> > + * This function might skip entries that were inserted since the iteration
> > + * started, but won't reprocess any.
> > + */
> > +
> > +/* virtual */ SysStatus
> > +ProcessLinuxServer::_getNextPID(__inout pid_t& pid)
> > +{
> > + AutoLock<BLock> as(&taskLock);
> > + task_struct *p;
> > + uval index;
> > +
> > + // locate the chain on which the restart should run on.
> > + index = pid_hashfn(pid);
> > + p = pidhash[index];
> > +
> > + // Search hash chain that the pid should be on
> > + if (p) {
> > + while (p) {
> > + if (p->pid == pid) {
> > + /* if we find the restart node then the next node
> > + * is the one we want. If it is null we are at then
> > + * end the chain so we will have to continue the search
> > + * but this is taken care of below as node will be set to null.
> > + */
> > + p = p->pidhash_next;
> > + break;
> > + }
> > + p = p->pidhash_next;
> > + }
> > + }
> > +
> > + /* If we could not find the restart node or there were no nodes after it
> > + * on the chain then we look for the first non empty chain following
> > + * the restart node's chain and identify the head as the next node.
> > + */
> > + if (!p) {
> > + for (index++; index < pidhash_sz; index++) {
> > + p = pidhash[index];
> > + // Don't return zombies (for now, maybe add an optional param later)
> > + if (p && p->k42process) {
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (p) {
> > + pid = p->pid;
> > + return 0;
> > + }
> > +
> > + pid = 0;
> > + // FIXME: ibm needs to allocate error code
> > + return _SERROR(9001, 0, ENOENT);
> > +}
> > +
> > SysStatus
> > ProcessLinuxServer::setTimeOfDay(uval sec, uval usec)
> > {
> > diff -ru kitchsrc.orig/os/servers/baseServers/ProcessLinuxServer.H kitchsrc/os/servers/baseServers/ProcessLinuxServer.H
> > --- kitchsrc.orig/os/servers/baseServers/ProcessLinuxServer.H 2006-02-28 08:59:32.000000000 -0800
> > +++ kitchsrc/os/servers/baseServers/ProcessLinuxServer.H 2006-09-15 14:20:50.000000000 -0700
> > @@ -486,6 +486,9 @@
> > virtual SysStatus releaseCredsPointer(
> > ProcessLinux::creds_t* linuxCredsPtr);
> >
> > + virtual SysStatus getFirstPID(pid_t& pid);
> > + virtual SysStatus getNextPID(pid_t& pid);
> > +
> > __xprotected:
> >
> > static SysStatusProcessID _GetPID();
> > @@ -532,6 +535,9 @@
> > virtual SysStatus _getInfoNativePid(__in ProcessID about,
> > __out struct ProcessLinux::LinuxInfo& linuxInfo);
> >
> > + virtual SysStatus _getFirstPID(__inout pid_t& pid);
> > + virtual SysStatus _getNextPID(__inout pid_t& pid);
> > +
> > virtual SysStatus _kill(__in pid_t pid, __in sval sig, __XHANDLE xhandle);
> >
> > virtual SysStatus _setsid(__XHANDLE xhandle);
> > _______________________________________________
> > K42-discussion mailing list
> > K42-discussion at ozlabs.org
> > https://ozlabs.org/mailman/listinfo/k42-discussion
> _______________________________________________
> K42-discussion mailing list
> K42-discussion at ozlabs.org
> https://ozlabs.org/mailman/listinfo/k42-discussion
>
More information about the K42-discussion
mailing list