-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ctypes calling convention that allows safe access of errno #46131
Comments
This patch adds new calling conventions to ctypes foreign functions by If CDLL(..., errno=True) or WinDLL(..., errno=True) is used, the Similarly, if CDLL(..., GetLastError=True) or WinDLL(..., The LastError and errno attributes are readonly from Python code, |
I like this way to tell that a function modifies errno or GetLastError. But this thread-local attribute on the function seems bizarre to me.
|
I'd stronly prefer NOT to add errno to the function return value. The recommended way to check for errors that I had in mind is in func = CDLL(..., errno=True)
func.argtypes = [...]
func.restype = ...
def errcheck(result, func, args):
if result == -1: # function failed
raise EnvironmentError(func.errno)
func.errcheck = errcheck Of course, an alternative to a thread local storage Anyway, this patch should be extended so that it is also possible |
thread local storage sounds also a bit weird to me. Errcheck sounds way Cheers, |
Alternatively, we can try to make ctypes "feel like" C itself: ctypes.set_errno(0)
while True:
dirent = linux_c_lib.readdir(byref(dir))
if not dirent:
if ctypes.get_errno() == 0:
break
else:
raise IOError("oups!") We are doing this kind of thing in PyPy with the "rffi" foreign function |
That's a great idea.
Is the ctypes.set_errno(...) function really needed? Wouldn't it be sufficient |
I also think that there should be explicit set_errno/get_errno calls. If |
But would it hurt to set errno to zero before *any* function call? My experiments show that it is faster to clear errno always |
People might get confused that errno is modified "without reason". I don't understand the need for the TLS, either - can't you just read |
This does not work because Python can run arbitrary code, |
On the one hand, no it does not hurt when we're resetting errno before IMO it's fine to reset errno before any call, until someone can find Cheers, |
How can Python run arbitrary code between the return from a ctypes |
By freeing objects because their refcount has reached zero? |
I'm in favor of keeping set_errno(). Not only is it more like C, but I personally don't care if this means a few percents of performance I imagine that the same approach could work with GetLastError() on Using the native errno instead of a custom TLS value is bad because a |
No, that should not set errno. Free cannot fail, and will not modify #include <errno.h>
int main()
{
errno = 117;
free(malloc(100));
printf("%d\n", errno);
} malloc might set errno, but only if it fails (in which case you'll get |
[...]
So what's the semantics of set_errno then? Set the real errno? If so, |
AFAIU, set_errno/get_errno should provide a ctypes-private copy of the real errno. Probably the real errno from before this action should be restored at the end. Code that I have in mind: __thread int _ctypes_errno; /* ctypes-private global error number in thread local storage */ static int _call_function_pointer(...)
{
int old_errno;
.
.
old_errno = errno;
errno = _ctypes_errno;
ffi_call(....);
_ctypes_errno = errno;
errno = old_errno;
.
.
}
static PyObject *
_ctypes_set_errno(PyObject *self, Pyobject *args)
{
.
_ctypes_errno = parsed_argument;
.
}
static PyObject *
_ctypes_get_errno(PyObject *self, Pyobject *args)
{
return PyInt_FromLong(_ctypes_errno);
} |
If you clear errno, anyway, you can also drop the set_errno call, and |
My code sample did not intend to clear errno, it was intended to set errno
To make my point clear (in case is isn't already): In the code dirent = linux_c_lib.readdir(byref(dir))
if not dirent:
if ctypes.get_errno() == 0: Consider the garbage collector free some objects that release resources Similarly for calling set_errno() or not calling it; other stdlib function Anyway, how can we proceed? Do you have a suggestion? Should set_errno() set a flag that is examined and consumed before |
It's the same thing (to me): you change errno.
However, even the TLS copy of errno may change because of this,
I'll leave it up to you to decide - you'll take both blame and praise, |
Yes, it's annoying, but at least the Python programmer has a way to fix (Another note: the C-level errno and the TLS copy should also be In PyPy, when creating and using our own internal FFI, we started by (A related issue that we may or may not care about: it's more than |
Ok, here is the plan (basically Armin's proposal): ctypes maintains a gloabl, but thread-local, variable that contains an error number; ctypes.set_errno(value) copies 'value' into ctypes_errno, and returns the old value. Foreign functions created with CDLL(..., use_errno=True), when called, copy The same scheme is implemented on Windows for GetLastError() and SetLastError(), Armin Rigo schrieb:
Not sure what this is for, please proofread the patch when I attach one.
Since the new behaviour will only be invoked when use_errno or use_LastError |
Here is a patch implementing the plan. This text could serve as a start for the /* Foreign functions created with CDLL(..., use_errno=True), when called, swap The values are also swapped immeditately before and after ctypes callback Two new ctypes functions are provided to access the 'ctypes_errno' value
The same scheme is implemented on Windows for GetLastError() and On Windows, TlsSetValue and TlsGetValue calls are used to provide thread |
Thomas Heller schrieb:
ctypes-errno-3.patch, in case of doubt. |
A different patch but implementing the same functionality was now |
What I meant is what should occur when a pure Python function is used |
I figured that out in the meantime and implemented it in this way. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: