Opened 3 years ago

Closed 3 years ago

#5147 defect closed fixed (fixed)

_inotify should specify result type of all ctypes function calls

Reported by: fijal Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess Branch: branches/inotify-types-decl-5147
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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

Attachments (1)

inotify-warnings.txt (66.3 KB) - added by exarkun 3 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by fijal

  • Type changed from enhancement to defect

comment:2 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/inotify-types-decl-5147

(In [32078]) Branching to 'inotify-types-decl-5147'

Changed 3 years ago by exarkun

comment:3 Changed 3 years ago by exarkun

  • Keywords review added

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.

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 <http://buildbot.pypy.org/nightly/trunk/>.

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.

comment:4 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:5 Changed 3 years ago by jesstess

  • Cc jesstess added
  • Keywords review removed
  • Owner changed from jesstess to exarkun

Thanks for working on this, exarkun. Some feedback:

  • the docstring for initializeModule should reflect that it is now setting argtypes and restype for all 3 functions
  • it looks like inotify_add_watch takes a uint32_t mask and not an int mask as its third argument:
         libc.inotify_add_watch.argtypes = [
             ctypes.c_int, ctypes.c_char_p, ctypes.c_int]
    
  • 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.

Other than that, I think this is good to merge!

Build results here.

comment:6 Changed 3 years ago by exarkun

the docstring for initializeModule should reflect ...

Fixed in r32094

it looks like inotify_add_watch takes a uint32_t mask

Oops! Thanks. Fixed in r32095.

I don't see a failing PyPy test in the attached test run results

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 only added the type definitions, this caused a failure due to the bug in INotify.ignore which would trigger a test failure. So then I went ahead and fixed that bug to get the test passing again.

The details of the test failure, for the curious, were this. ignore would sometimes pass None as the watch id value to inotify_rm_watch. CPython 2.6 (at least) silently translated None to 0, 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 KeyError, which is what the test wanted in the first place.

Once the argument types for inotify_rm_watch were defined, ctypes stopped accepting None - 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.

The fix to the test was to explicitly raise the KeyError sooner in this case, before ever trying to call inotify_rm_watch.

comment:7 Changed 3 years ago by exarkun

Oops. It didn't make sense to me that inotify_add_watch would return int representing a new watch id but inotify_rm_watch would take uint32 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 int in both places. Only the mask is a uint32. I'll update the type definitions to match the header file instead of the man page.


comment:8 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [32097]) Merge inotify-types-decl-5147

Author: exarkun
Reviewer: jesstess
Fixes: #5147

Declare the argument and return types on the ctypes wrapper functions used in
twisted.python._inotify. Also fix a case where inotify_rm_watch was
incorrectly called with None when INotify.ignore was called with an unwatched
path.

Note: See TracTickets for help on using tickets.