Twisted: Ticket #5147: _inotify should specify result type of all ctypes function callshttps://twistedmatrix.com/trac/ticket/5147
<p>
PyPy emit RuntimeWarning for this case and not specifying it is very fragile (it might work now, but the general idea seems to be very fragile).
</p>
en-usTwistedhttps://twistedmatrix.com/trac/chrome/common/trac_banner.pnghttps://twistedmatrix.com/trac/ticket/5147
Trac 1.2fijalWed, 08 Jun 2011 11:53:11 GMTtype changedhttps://twistedmatrix.com/trac/ticket/5147#comment:1
https://twistedmatrix.com/trac/ticket/5147#comment:1
<ul>
<li><strong>type</strong>
changed from <em>enhancement</em> to <em>defect</em>
</li>
</ul>
TicketJean-Paul CalderoneThu, 09 Jun 2011 01:13:48 GMTbranch, branch_author sethttps://twistedmatrix.com/trac/ticket/5147#comment:2
https://twistedmatrix.com/trac/ticket/5147#comment:2
<ul>
<li><strong>branch</strong>
set to <em>branches/inotify-types-decl-5147</em>
</li>
<li><strong>branch_author</strong>
set to <em>exarkun</em>
</li>
</ul>
<p>
(In <a class="missing changeset" title="No changeset 32078 in the repository">[32078]</a>) Branching to 'inotify-types-decl-5147'
</p>
TicketJean-Paul CalderoneThu, 09 Jun 2011 01:37:38 GMTattachment sethttps://twistedmatrix.com/trac/ticket/5147
https://twistedmatrix.com/trac/ticket/5147
<ul>
<li><strong>attachment</strong>
set to <em>inotify-warnings.txt</em>
</li>
</ul>
TicketJean-Paul CalderoneThu, 09 Jun 2011 01:40:21 GMTkeywords sethttps://twistedmatrix.com/trac/ticket/5147#comment:3
https://twistedmatrix.com/trac/ticket/5147#comment:3
<ul>
<li><strong>keywords</strong>
<em>review</em> added
</li>
</ul>
<p>
Declarations fixed in the branch. I expanded an existing unit test to cover the type declarations for all three ctypes-invoked functions. I also fixed a bug in the implementation which apparently let the implementation only work by accident on CPython and prevented it from working on PyPy. This caused a test to fail on PyPy, which apparently has a stricter version of ctypes argument checking logic.
</p>
<p>
For ease of review, test runs from CPython and the latest PyPy nightly against trunk and the branch are attached. Reviewers can also find PyPy nightlies at &lt;<a class="ext-link" href="http://buildbot.pypy.org/nightly/trunk/"><span class="icon">​</span>http://buildbot.pypy.org/nightly/trunk/</a>&gt;.
</p>
<p>
Not bothering to force a build now because half the builders are offline. It would certainly be nice to see the results of building the branch before merging this, though.
</p>
TicketjesstessSat, 11 Jun 2011 18:50:53 GMTowner sethttps://twistedmatrix.com/trac/ticket/5147#comment:4
https://twistedmatrix.com/trac/ticket/5147#comment:4
<ul>
<li><strong>owner</strong>
set to <em>jesstess</em>
</li>
</ul>
TicketjesstessSat, 11 Jun 2011 20:10:21 GMTowner changed; cc set; keywords deletedhttps://twistedmatrix.com/trac/ticket/5147#comment:5
https://twistedmatrix.com/trac/ticket/5147#comment:5
<ul>
<li><strong>keywords</strong>
<em>review</em> removed
</li>
<li><strong>cc</strong>
<em>jesstess</em> added
</li>
<li><strong>owner</strong>
changed from <em>jesstess</em> to <em>Jean-Paul Calderone</em>
</li>
</ul>
<p>
Thanks for working on this, exarkun. Some feedback:
</p>
<ul><li>the docstring for <code>initializeModule</code> should reflect that it is now setting <code>argtypes</code> and <code>restype</code> for all 3 functions
</li><li>it looks like <code>inotify_add_watch</code> takes a <code>uint32_t</code> mask and not an <code>int</code> mask as its third argument:
<pre class="wiki"> libc.inotify_add_watch.argtypes = [
ctypes.c_int, ctypes.c_char_p, ctypes.c_int]
</pre></li><li>I don't see a failing PyPy test in the attached test run results, so I'm not sure what test now passes. PyPy and Twisted aren't playing nicely on my dev machine right now or I'd check myself, but if a failing test is now passing, great.
</li></ul><p>
Other than that, I think this is good to merge!
</p>
<p>
Build results <a class="ext-link" href="http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/inotify-types-decl-5147"><span class="icon">​</span>here</a>.
</p>
TicketJean-Paul CalderoneSat, 11 Jun 2011 20:59:04 GMThttps://twistedmatrix.com/trac/ticket/5147#comment:6
https://twistedmatrix.com/trac/ticket/5147#comment:6
<blockquote class="citation">
<p>
the docstring for initializeModule should reflect ...
</p>
</blockquote>
<p>
Fixed in <a class="missing changeset" title="No changeset 32094 in the repository">r32094</a>
</p>
<blockquote class="citation">
<p>
it looks like inotify_add_watch takes a uint32_t mask
</p>
</blockquote>
<p>
Oops! Thanks. Fixed in <a class="changeset" href="https://twistedmatrix.com/trac/changeset/32095" title="Apply 6401.1.patch from kkpattern.
Refs: #6401
git-svn-id: ...">r32095</a>.
</p>
<blockquote class="citation">
<p>
I don't see a failing PyPy test in the attached test run results
</p>
</blockquote>
<p>
Indeed, sorry for not being more clear. The initial purpose of the branch was just to clean up the warnings visible on PyPy. While doing that, I found that if I <em>only</em> added the type definitions, this caused a failure due to the bug in <code>INotify.ignore</code> which would trigger a test failure. So then I went ahead and fixed that bug to get the test passing again.
</p>
<p>
The details of the test failure, for the curious, were this. <code>ignore</code> would sometimes pass <code>None</code> as the watch id value to <code>inotify_rm_watch</code>. CPython 2.6 (at least) silently translated <code>None</code> to <code>0</code>, making the call possible but not valid, since no watch id value ever seems to be 0 (but who knows if that actually holds). It would then go on to try to pop None out of a dictionary which failed with a <code>KeyError</code>, which is what the test wanted in the first place.
</p>
<p>
Once the argument types for <code>inotify_rm_watch</code> were defined, ctypes stopped accepting <code>None</code> - since it had been told the parameter is a c_uint32, it wanted an integer that fit in that range. So the code path started failing with a TypeError.
</p>
<p>
The fix to the test was to explicitly raise the KeyError sooner in this case, before ever trying to call <code>inotify_rm_watch</code>.
</p>
TicketJean-Paul CalderoneSat, 11 Jun 2011 21:07:10 GMThttps://twistedmatrix.com/trac/ticket/5147#comment:7
https://twistedmatrix.com/trac/ticket/5147#comment:7
<p>
Oops. It didn't make sense to me that <code>inotify_add_watch</code> would return <code>int</code> representing a new watch id but <code>inotify_rm_watch</code> would take <code>uint32</code> representing the watch id to remove, so I dug around a little bit. The man page seems broken. The header file more sensibly handles the watch id as an <code>int</code> in both places. <em>Only</em> the mask is a <code>uint32</code>. I'll update the type definitions to match the header file instead of the man page.
</p>
<p>
</p>
TicketJean-Paul CalderoneSun, 12 Jun 2011 01:44:33 GMTstatus changed; resolution sethttps://twistedmatrix.com/trac/ticket/5147#comment:8
https://twistedmatrix.com/trac/ticket/5147#comment:8
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
</ul>
<p>
(In <a class="missing changeset" title="No changeset 32097 in the repository">[32097]</a>) Merge inotify-types-decl-5147
</p>
<p>
Author: exarkun
Reviewer: jesstess
Fixes: <a class="closed ticket" href="https://twistedmatrix.com/trac/ticket/5147" title="#5147: defect: _inotify should specify result type of all ctypes function calls (closed: fixed)">#5147</a>
</p>
<p>
Declare the argument and return types on the ctypes wrapper functions used in
<code>twisted.python._inotify</code>. Also fix a case where <code>inotify_rm_watch</code> was
incorrectly called with <code>None</code> when <code>INotify.ignore</code> was called with an unwatched
path.
</p>
Ticket