Author neologix
Recipients nadeem.vawda, neologix, pitrou, vstinner
Date 2012-02-29.08:10:44
SpamBayes Score 1.24345e-14
Marked as misclassified No
Message-id <CAH_1eM3QjUpxpdUFjYNyVov75BmO4x6yugahOM9JtCoiYoVawQ@mail.gmail.com>
In-reply-to <1330469328.03.0.112488412954.issue14154@psf.upfronthosting.co.za>
Content
> + f = open(self.procfile, 'r')
>
> 'rb' mode is enough here, no need of Unicode ;-)

Why?
At least to me, 'r' stands for text I/O, whereas 'rb' stands for
binary I/O: here, I want to read /proc/<PID>/statm, which is a textual
representation of the memory usage (entries are separated by space
characters).

> + self.mem_watchdog = subprocess.Popen(..., stdin=f)
>
> Can't you open the /proc/pid/stat file in the child process? It might be an issue with SELinux or Grsecurity, but I don't expect that our buildbot use such security patch.

Yes, SELinux and grsecurity are the first reason I open it in the
parent. The second reason is that, if the parent crashes after the
fork(), but before the child starts, I don't want it to read another
process' /proc/<PID>/statm (I know this would require an immediate
recycling of the PID which is thus really unlikely, but hey).

> + statm = sys.stdin.read()
>
> file_watchdog() did read only 1024 bytes. I don't know if it would be better or not to use an arbitrary limit (to avoid filling the memory?).

"""
$ cat /proc/self/statm
954 106 84 5 0 67 0
"""

I think that should fit in memory ;-)

> You should catch OSError here.

Why? If we get an OSError, we can't do much except exiting anyway.

> You may also write the watchdog script in .py file instead of passing it on the command line, so it will be easier to maintain it later.

Yes, that would be better.
History
Date User Action Args
2012-02-29 08:10:46neologixsetrecipients: + neologix, pitrou, vstinner, nadeem.vawda
2012-02-29 08:10:45neologixlinkissue14154 messages
2012-02-29 08:10:44neologixcreate