classification
Title: Return from fork() is pid_t, not int
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.0, Python 3.1, Python 2.7, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ajaksu2, christian.heimes, loewis, pitrou, rstutsman, stutsman
Priority: normal Keywords: patch

Created on 2008-01-31 20:07 by stutsman, last changed 2009-05-23 16:22 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue1983.patch pitrou, 2009-05-19 07:36
Messages (17)
msg61926 - (view) Author: Ryan Stutsman (stutsman) Date: 2008-01-31 20:07
In current trunk (60097).  Return from fork is not int but pid_t. 
Treating this as an int causes total breakage on systems with 64-bit pids.
msg61927 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-01-31 20:26
Can you provide a patch? It would be good if it still worked on systems
that don't have pid_t (although we could also require pid_t and then see
what systems that breaks for).

Also, what systems use 64-bit pids?
msg61928 - (view) Author: Ryan Stutsman (stutsman) Date: 2008-01-31 20:51
Yeah; I shuold be able to provide one.  I just hacked 2.4.4 to work so I
think I could provide a fix easily.  The version I put together here is
rough, so I'll try to create a cleaner solution tonight or this weekend.

HiStar does (http://www.scs.stanford.edu/histar/).

Not sure if it's better to test for pid_t (in configure) or just use a
long long value all around a punt the problem?  I'll probably just go
with the former unless there are objections.
msg61931 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-31 21:30
It's probably better to use pid_t through the code and add a typedef int
pid_t for system's without pid_t. Or better typedef long pid_t?

On my Linux, pid_t is defined as __PID_T_TYPE, which is defines as
__S32_TYPE which ends up as int.
msg61932 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-01-31 21:32
Don't use long long literally; not all systems have such a type, and
even if they do, they might not call it "long long".
msg61939 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-01-31 23:09
I fixed the bug in r60484. Python was already using pid_t on several
occasions like getpid(). I added a test to verify that PyInt_FromLong()
can always handle pid_t.

Do you want to port the fix to 2.5?
msg61947 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-01 05:04
> Do you want to port the fix to 2.5?

I'm not quite sure that the patch actually fixes the
problem. IIUC, HiStar is available in a 32-bit version,
too, yet it may still use a 64-bit pid_t (Ryan, can
you confirm whether that's the case?).

If so, Python would now fail to compile under that
patch. Backporting a change that causes Python to fail
to compile on some systems is not a good idea.

If that aspect was fixed also (e.g. by always returning
long ints on systems where sizeof(pid_t)>sizeof(long)),
a backport would be ok. For a perfect backport, that
change might still cause a behavior change:  on
a system where sizeof(pid_t)>sizeof(long), yet the
system only ever uses pid_t values < INT_MAX, people
would see that the fork return type changes unreasonably;
a perfect backport would only return longs if the values
are out of range. This is probably over-cautious, as
it's fairly unlikely that such systems actually exist.
msg61948 - (view) Author: Ryan Stutsman (stutsman) Date: 2008-02-01 06:06
> IIUC, HiStar is available in a 32-bit version,
> too, yet it may still use a 64-bit pid_t (Ryan, can
> you confirm whether that's the case?).

Great point.  pid_t is always 64-bit on HiStar.
msg61965 - (view) Author: Ryan Stutsman (stutsman) Date: 2008-02-01 16:32
Actually the current trunk of as of this morning (60484) is still broken
in a couple of ways.  First, converting the pid_t using PyInt is a
problem  and second the waitpids aren't corrected.  This would cause
waits on invalid pids.
msg61966 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-01 16:54
Ryan Stutsman wrote:
> Actually the current trunk of as of this morning (60484) is still broken
> in a couple of ways.  First, converting the pid_t using PyInt is a
> problem  and second the waitpids aren't corrected.  This would cause
> waits on invalid pids.

I'll see what I kind do about it.

Christian
msg61972 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-02-01 19:18
Martin v. Löwis wrote:
> If so, Python would now fail to compile under that
> patch. Backporting a change that causes Python to fail
> to compile on some systems is not a good idea.

I added the size comparison to identify systems with sizeof(pid_t) >
sizeof(long).

> If that aspect was fixed also (e.g. by always returning
> long ints on systems where sizeof(pid_t)>sizeof(long)),
> a backport would be ok. For a perfect backport, that
> change might still cause a behavior change:  on
> a system where sizeof(pid_t)>sizeof(long), yet the
> system only ever uses pid_t values < INT_MAX, people
> would see that the fork return type changes unreasonably;
> a perfect backport would only return longs if the values
> are out of range. This is probably over-cautious, as
> it's fairly unlikely that such systems actually exist.

Your proposal looks sound and good to me, but it involves some work. The
chance would require a new format operator 'p' for argument parsing and
new functions like PyInt_FromPid_t() and PyInt_AsPid_t().

In r60504 I've changed the type for the remaining functions like waitpit
 and getsid to pid_t. It should make it easy to spot the lines that have
to be changed.

Christian
msg61974 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-01 19:39
> Your proposal looks sound and good to me, but it involves some work. The
> chance would require a new format operator 'p' for argument parsing and
> new functions like PyInt_FromPid_t() and PyInt_AsPid_t().

No, it doesn't require that. You could use conditional compilation
throughout to achieve this, and the function might be more list
int_from_pid_t (i.e. static).
msg87717 - (view) Author: Daniel Diniz (ajaksu2) (Python triager) Date: 2009-05-13 22:02
Is this pending for trunk and py3k too or just for 2.5 (and hence can be
closed)?
msg88058 - (view) Author: Ryan Stutsman (rstutsman) Date: 2009-05-18 23:17
No, I don't think this is actually fixed in any version of Python at the
moment.  The title may be a bit misleading, because all the versions now
store the result of fork in a pid_t and return it as a PyLong.  However,
posix_waitpid is still pulling pid's as a PyInt.  Changing this to
PyLong would probably work for our purposes, but I guess the hangup was
that in reality pid_t is supposed to be an opaque datatype and
implementing it in CPython is non-trivial and has little benefit.

Perhaps we close this as won't fix or I can create a patch to at least
give a hack for 64-bit pid's but still treated as a long.
msg88073 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-19 07:36
The following patch should do the trick.
msg88228 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-05-23 11:11
The patch looks fine to me, please apply.
msg88241 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-05-23 16:22
Thanks, the patch was committed in r72852, r72853, r72854.
I then realized a couple of functions had been looked over (including
getpid() and getppid()), and committed a complement in r72855, r72856
and r72858.
History
Date User Action Args
2009-05-23 16:22:03pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg88241
2009-05-23 11:11:47loewissetresolution: accepted
messages: + msg88228
2009-05-19 07:37:10pitrousetstage: patch review
resolution: fixed -> (no value)
versions: + Python 2.6, Python 3.0, Python 3.1, Python 2.7, - Python 2.5
2009-05-19 07:36:48pitrousetfiles: + issue1983.patch

nosy: + pitrou
messages: + msg88073

keywords: + patch
2009-05-18 23:17:46rstutsmansetnosy: + rstutsman
messages: + msg88058
2009-05-13 22:02:56ajaksu2setstatus: pending -> open
nosy: + ajaksu2
messages: + msg87717

2009-05-13 22:02:51ajaksu2linkissue2005 dependencies
2008-02-01 19:39:56loewissetmessages: + msg61974
2008-02-01 19:18:27christian.heimessetmessages: + msg61972
2008-02-01 16:54:47christian.heimessetmessages: + msg61966
2008-02-01 16:32:16stutsmansetmessages: + msg61965
2008-02-01 06:07:00stutsmansetmessages: + msg61948
2008-02-01 05:04:53loewissetmessages: + msg61947
2008-01-31 23:09:38christian.heimessetstatus: open -> pending
resolution: fixed
messages: + msg61939
versions: + Python 2.5, - Python 2.6, Python 3.0
2008-01-31 21:32:58loewissetmessages: + msg61932
2008-01-31 21:30:02christian.heimessetpriority: normal
nosy: + christian.heimes
type: behavior
messages: + msg61931
2008-01-31 20:51:28stutsmansetmessages: + msg61928
2008-01-31 20:26:58loewissetnosy: + loewis
messages: + msg61927
2008-01-31 20:07:27stutsmancreate