| msg94502 - (view) |
Author: Michael Broghton (mbroughton) |
Date: 2009-10-26 19:54 |
| On FreeBSD and MacOS 64-bit systems the ident field of a kevent is big enough to hold a 64-bit integer (uintptr_t). Looks like Python is casting it to an unsigned 32-bit integer. This is inconvenient for implementing kqueue timers, where id(timer_obj) is a natural choice for an ident. |
|
|
| msg94504 - (view) |
Author: Martin v. Löwis (loewis) *  |
Date: 2009-10-26 20:28 |
| Would you like to propose a patch? |
|
|
| msg94529 - (view) |
Author: Michael Broghton (mbroughton) |
Date: 2009-10-26 22:34 |
| I'm not sure how to patch this so that it will work on both 32 and 64 bit systems. Issues: 1. What would be an appropriate member type for ident in kqueue_event_members? It seems like T_PYSSIZET might work. Otherwise, I am guessing that this will involve some #if's. 2. I think the format spec in kqueue_event_repr needs to change. It seems like this will also require some #if's. 3. kqueue_event_init uses PyObject_AsFileDescriptor to set the ident field. This should be doing a PyLong_Check first to see if PyLong_AsSomething would be more appropriate. 4. I think the type of the result variable in kqueue_event_richcompare needs to be changed to long long int. |
|
|
| msg94534 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2009-10-26 22:48 |
| > 1. What would be an appropriate member type for ident in > kqueue_event_members? It seems like T_PYSSIZET might work. Otherwise, I > am guessing that this will involve some #if's. IIUC, it needs to match *exactly* the field size inside struct kevent. So you'll have to use #ifs, possible along with autoconf tests (to find out the sizes of the fields that typically vary across implementations). > 2. I think the format spec in kqueue_event_repr needs to change. It > seems like this will also require some #if's. Here, I would widen the fields to size_t, and use the size_t formatter (assuming that all systems supporting kqueue also know how to print size_t, or know to print long long). > 3. kqueue_event_init uses PyObject_AsFileDescriptor to set the ident > field. This should be doing a PyLong_Check first to see if > PyLong_AsSomething would be more appropriate. Hmm. I think I need to understand the use case better. Can you post some sample code where this all matters? If this *is* a regular file-like object, then surely int is enough, no? > 4. I think the type of the result variable in kqueue_event_richcompare > needs to be changed to long long int. Fine with me. The question then is whether long long is available on all systems that support kqueue. |
|
|
| msg94544 - (view) |
Author: Michael Broghton (mbroughton) |
Date: 2009-10-27 00:52 |
| Martin, thanks for your responses. In regards to point three: Kqueue's are not just used for file descriptors. I believe this is the reason why the ident field is a uintptr_t and not an int. The example I gave was for kqueue timers. Since the operating system does not allocate 'timer descriptors' for you, I decided to use the return value from the id function. Here is some code that demonstrates: import select a = 1 b = id(a) c = select.kevent(a) d = select.kevent(b) assert a == c.ident assert b == d.ident assert b & (1<<32) - 1 == d.ident The second assert will fail on 64 bit systems if 'b' is too big. Anyway, I will try to come up with a patch for this. |
|
|
| msg94562 - (view) |
Author: Michael Broghton (mbroughton) |
Date: 2009-10-27 15:35 |
| This is against release31-maint, if it matters. |
|
|
| msg94666 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-10-29 13:29 |
| According to man pages on the Web, the kevent structure is: struct kevent { uintptr_t ident; /* identifier for this event */ short filter; /* filter for event */ u_short flags; /* action flags for kqueue */ u_int fflags; /* filter flag value */ intptr_t data; /* filter data value */ void *udata; /* opaque user data identifier */ }; So the `ident` field is indeed an uintptr_t. However the patch is slightly wrong IMO: - you should not blindly use `long long`. Python already defines a "Py_uintptr_t" type. If you need more information you should conditional compilation such as in (for the off_t type) http://svn.python.org/view/python/trunk/Modules/_io/_iomodule.h?view=markup - in tests, you should use sys.maxsize (which gives you the max value of a ssize_t integer) rather than choosing based on platform.architecture(): >>> sys.maxsize 9223372036854775807 >>> 2**64*1 18446744073709551616L >>> sys.maxsize*2 + 1 18446744073709551615L PS : a patch against trunk or py3k is preferred, but in this case it would probably apply cleanly anyway |
|
|
| msg94667 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-10-29 13:31 |
| Apparently Roundup borked the URL. Let's try another one (or look into Modules/_io/_iomodule.h): http://code.python.org/hg/trunk/file/b9bc35171668/Modules/_io/_iomodule.h#l88 |
|
|
| msg94671 - (view) |
Author: Michael Broghton (mbroughton) |
Date: 2009-10-29 14:59 |
| Antoine, thanks for the tips and the example. I have updated the patch. I checked and this does apply cleanly to py3k. |
|
|
| msg94676 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2009-10-29 15:58 |
| The patch (http://bugs.python.org/file15228/kevent.patch) works for me under OS X 10.5.8 Intel. All tests pass, except test_telnetlib which fails intermittently with and without the patch. Python 3.2a0 (py3k:75951, Oct 29 2009, 11:38:58) [GCC 4.0.1 (Apple Inc. build 5465)] on darwin |
|
|
| msg94896 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-11-04 18:15 |
| For me, the patch is worth trying out on the buildbots - to see if there are any configuration problems we might have overlooked. |
|
|
| msg94904 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-11-04 21:57 |
| Patch was committed in r76108 (trunk) and r76111 (py3k) and didn't introduce any regression on the FreeBSD and OS X buildbots. Thanks! |
|
|
| msg95852 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2009-12-01 09:08 |
| This patch causes build errors on 32-bit/64-bit multi-architecture builds on OS X. See Issue7416 for more info and a patch. |
|
|