Charles-François Natali <neolo...@free.fr> added the comment:

> + 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.

----------

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue14154>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to