From 14e70ec876d422f5786462dd84318b4c371789af Mon Sep 17 00:00:00 2001 From: Sean Reifschneider Date: Thu, 18 Mar 2021 22:55:37 -0600 Subject: [PATCH 1/2] Assorted Python changes based on a code review. Many changes were based on conventions in the Python socketmodule. Changed many of the docstrings to match the Python socket library conventions and enhancing them. I can either remove the prototype part or add it to other docstrings in the library, depending on feedback I get. Changed setblocking to use the flag argument instead of always just setting NONBLOCK. Added enable/disable threading around more lwip calls. Implementing optional backlog on listen(). Removing a few seemingly unneeded Py_INCREF(Py_None) calls. Moved getblocking function based on alpha sorting of names. --- src/bindings/python/PythonSockets.cpp | 51 ++++++++++++---- src/bindings/python/libzt.py | 2 +- src/bindings/python/sockets.py | 84 ++++++++++++++++++++++----- 3 files changed, 109 insertions(+), 28 deletions(-) diff --git a/src/bindings/python/PythonSockets.cpp b/src/bindings/python/PythonSockets.cpp index febc050..be82915 100644 --- a/src/bindings/python/PythonSockets.cpp +++ b/src/bindings/python/PythonSockets.cpp @@ -26,22 +26,46 @@ #ifdef ZTS_ENABLE_PYTHON -int zts_py_setblocking(int fd, int flag) +int zts_py_setblocking(int fd, int block) { - int flags = ZTS_ERR_OK; - if ((flags = zts_fcntl(fd, F_GETFL, 0)) < 0) { - return ZTS_ERR_SOCKET; + int new_flags, flags, err = 0; + + Py_BEGIN_ALLOW_THREADS + flags = zts_fcntl(fd, F_GETFL, 0); + + if (flags < 0) { + err = ZTS_ERR_SOCKET; + goto done; } - return zts_fcntl(fd, F_SETFL, flags | ZTS_O_NONBLOCK); + + if (block) { + new_flags |= ZTS_O_NONBLOCK; + } else { + new_flags &= ~ZTS_O_NONBLOCK; + } + + if (new_flags != flags) { + err = zts_fcntl(fd, F_SETFL, flags); + } + +done: + Py_END_ALLOW_THREADS + + return err; } int zts_py_getblocking(int fd) { - int flags = ZTS_ERR_OK; - if ((flags = zts_fcntl(fd, F_GETFL, 0)) < 0) { + int flags; + + Py_BEGIN_ALLOW_THREADS + flags = zts_fcntl(fd, F_GETFL, 0); + Py_END_ALLOW_THREADS + + if (flags < 0) { return ZTS_ERR_SOCKET; } - return flags & ZTS_O_NONBLOCK; + return flags & ZTS_O_NONBLOCK; } static int zts_py_tuple_to_sockaddr(int family, @@ -98,6 +122,9 @@ PyObject * zts_py_accept(int fd) int zts_py_listen(int fd, int backlog) { + if (backlog < 0) { + backlog = 128; + } return zts_listen(fd, backlog); } @@ -114,7 +141,6 @@ int zts_py_bind(int fd, int family, int type, PyObject *addr_obj) Py_BEGIN_ALLOW_THREADS err = zts_bind(fd, (struct zts_sockaddr *)&addrbuf, addrlen); Py_END_ALLOW_THREADS - Py_INCREF(Py_None); return err; } @@ -131,7 +157,6 @@ int zts_py_connect(int fd, int family, int type, PyObject *addr_obj) Py_BEGIN_ALLOW_THREADS err = zts_connect(fd, (struct zts_sockaddr *)&addrbuf, addrlen); Py_END_ALLOW_THREADS - Py_INCREF(Py_None); return err; } @@ -207,7 +232,11 @@ int zts_py_send(int fd, PyObject *buf, int flags) int zts_py_close(int fd) { - return zts_close(fd); + int err; + Py_BEGIN_ALLOW_THREADS + err = zts_close(fd); + Py_END_ALLOW_THREADS + return err; } #endif // ZTS_ENABLE_PYTHON diff --git a/src/bindings/python/libzt.py b/src/bindings/python/libzt.py index bfc77de..678f266 100644 --- a/src/bindings/python/libzt.py +++ b/src/bindings/python/libzt.py @@ -1,5 +1,5 @@ # This file was automatically generated by SWIG (http://www.swig.org). -# Version 4.0.2 +# Version 4.0.1 # # Do not make changes to this file unless you know what you are doing--modify # the SWIG interface file instead. diff --git a/src/bindings/python/sockets.py b/src/bindings/python/sockets.py index 547f84e..7adc31a 100644 --- a/src/bindings/python/sockets.py +++ b/src/bindings/python/sockets.py @@ -236,7 +236,11 @@ class socket: raise NotImplementedError("if_indextoname(): libzt does not name interfaces.") def accept(self): - """Accept connection on the socket""" + """accept() -> (socket, address_info) + + Wait for incoming connection and return a tuple of socket object and + client address info. This address info may be a tuple of host address + and port.""" new_conn_fd, addr, port = libzt.zts_py_accept(self._fd) if new_conn_fd < 0: handle_error(new_conn_fd) @@ -244,25 +248,33 @@ class socket: return socket(self._family, self._type, self._proto, new_conn_fd), addr def bind(self, local_address): - """Bind the socket to a local interface address""" + """bind(address) + + Bind the socket to a local interface address""" err = libzt.zts_py_bind(self._fd, self._family, self._type, local_address) if err < 0: handle_error(err) def close(self): - """Close the socket""" + """close() + + Close the socket""" err = libzt.zts_py_close(self._fd) if err < 0: handle_error(err) def connect(self, remote_address): - """Connect the socket to a remote address""" + """connect(address) + + Connect the socket to a remote address""" err = libzt.zts_py_connect(self._fd, self._family, self._type, remote_address) if err < 0: handle_error(err) def connect_ex(self, remote_address): - """Connect to remote host but return low-level result code, and errno on failure + """connect_ex(address) -> errno + + Connect to remote host but return low-level result code, and errno on failure This uses a non-thread-safe implementation of errno """ err = libzt.zts_py_connect(self._fd, self._family, self._type, remote_address) @@ -287,6 +299,12 @@ class socket: """libzt does not support this (yet)""" raise NotImplementedError("libzt does not support this (yet?)") + def getblocking(self): + """getblocking() + + Return True if the socket is in blocking mode, False if it is non-blocking""" + return libzt.zts_py_getblocking(self._fd) + def getpeername(self): """libzt does not support this (yet)""" raise NotImplementedError("libzt does not support this (yet?)") @@ -299,10 +317,6 @@ class socket: """libzt does not support this (yet)""" raise NotImplementedError("libzt does not support this (yet?)") - def getblocking(self): - """Get whether this socket is in blocking or non-blocking mode""" - return libzt.zts_py_getblocking(self._fd) - def gettimeout(self): """libzt does not support this (yet)""" raise NotImplementedError("libzt does not support this (yet?)") @@ -311,8 +325,19 @@ class socket: """libzt does not support this (yet)""" raise NotImplementedError("libzt does not support this (yet?)") - def listen(self, backlog): - """Put the socket in a listening state (with an optional backlog argument)""" + def listen(self, backlog=None): + """listen([backlog]) + + Put the socket in a listening state. Backlog specifies the number of + outstanding connections the OS will queue without being accepted. If + less than 0, it is set to 0. If not specified, a reasonable default + will be used.""" + + if backlog is not None and backlog < 0: + backlog = 0 + if backlog is None: + backlog = -1 # Lower-level code picks default + err = libzt.zts_py_listen(self._fd, backlog) if err < 0: handle_error(err) @@ -322,7 +347,16 @@ class socket: raise NotImplementedError("libzt does not support this (yet?)") def recv(self, n_bytes, flags=0): - """Read data from the socket""" + """recv(buffersize[, flags]) -> data + + Read up to buffersize bytes from remote. Wait until at least one byte + is read, or remote is closed. If all data is read and remote is closed, + returns empty string. Flags may be: + + - ZTS_MSG_PEEK - Peeks at an incoming message. + - ZTS_MSG_DONTWAIT - Nonblocking I/O for this operation only. + - ZTS_MSG_MORE - Wait for more than one message. + """ err, data = libzt.zts_py_recv(self._fd, n_bytes, flags) if err < 0: handle_error(err) @@ -350,7 +384,16 @@ class socket: raise NotImplementedError("libzt does not support this (yet?)") def send(self, data, flags=0): - """Write data to the socket""" + """send(data[, flags]) -> count + + Write data to the socket. Returns the number of bytes sent, which + may be less than the full data size if the network is busy. + Optional flags may be: + + - ZTS_MSG_PEEK - Peeks at an incoming message. + - ZTS_MSG_DONTWAIT - Nonblocking I/O for this operation only. + - ZTS_MSG_MORE - Sender will send more. + """ err = libzt.zts_py_send(self._fd, data, flags) if err < 0: handle_error(err) @@ -389,8 +432,10 @@ class socket: raise NotImplementedError("libzt does not support this (yet?)") def setblocking(self, flag): - """Set whether this socket is in blocking or non-blocking mode""" - libzt.zts_py_setblocking(self._fd, flag) + """setblocking(flag) + + Sets the socket to blocking mode if flag=True, non-blocking if flag=False.""" + libzt.zts_py_setblocking(self._fd, block) def settimeout(self, value): """libzt does not support this (yet)""" @@ -404,5 +449,12 @@ class socket: raise NotImplementedError("libzt does not support this (yet?)") def shutdown(self, how): - """Shut down one or more aspects (rx/tx) of the socket""" + """shutdown(how) + + Shut down one or more aspects (rx/tx) of the socket depending on how: + + - ZTS_SHUT_RD - Shut down reading side of socket. + - ZTS_SHUT_WR - Shut down writing side of socket. + - ZTS_SHUT_RDWR - Both ends of the socket. + """ libzt.zts_shutdown(self._fd, how) From 2ca8e01864f8504cd5db4f3f4a87223704065dba Mon Sep 17 00:00:00 2001 From: Sean Reifschneider Date: Fri, 19 Mar 2021 10:44:29 -0600 Subject: [PATCH 2/2] Fixing some bugs in socket blocking. There were some bugs in my blocking code that are fixed in this. I had the setblocking() flag reversed, and wasn't passing the flag along to the underlying code properly. --- src/bindings/python/PythonSockets.cpp | 12 ++++++------ src/bindings/python/sockets.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bindings/python/PythonSockets.cpp b/src/bindings/python/PythonSockets.cpp index be82915..cc8e926 100644 --- a/src/bindings/python/PythonSockets.cpp +++ b/src/bindings/python/PythonSockets.cpp @@ -28,24 +28,24 @@ int zts_py_setblocking(int fd, int block) { - int new_flags, flags, err = 0; + int new_flags, cur_flags, err = 0; Py_BEGIN_ALLOW_THREADS - flags = zts_fcntl(fd, F_GETFL, 0); + cur_flags = zts_fcntl(fd, F_GETFL, 0); - if (flags < 0) { + if (cur_flags < 0) { err = ZTS_ERR_SOCKET; goto done; } - if (block) { + if (!block) { new_flags |= ZTS_O_NONBLOCK; } else { new_flags &= ~ZTS_O_NONBLOCK; } - if (new_flags != flags) { - err = zts_fcntl(fd, F_SETFL, flags); + if (new_flags != cur_flags) { + err = zts_fcntl(fd, F_SETFL, new_flags); } done: diff --git a/src/bindings/python/sockets.py b/src/bindings/python/sockets.py index 7adc31a..0f5b879 100644 --- a/src/bindings/python/sockets.py +++ b/src/bindings/python/sockets.py @@ -435,7 +435,7 @@ class socket: """setblocking(flag) Sets the socket to blocking mode if flag=True, non-blocking if flag=False.""" - libzt.zts_py_setblocking(self._fd, block) + libzt.zts_py_setblocking(self._fd, flag) def settimeout(self, value): """libzt does not support this (yet)"""