Add fcntl module #41
Add fcntl module #41
Conversation
There was a problem hiding this comment.
While I see that you did set your email address in git since your previous submission, the author name on this commit is still "krkacc" which is not acceptable. Please set an appropriate author name and rewrite the commit.
Additionally, I would like to inquire about your use of Kuroko and your need for an fcntl module with F_GETFL/F_SETFL.
src/modules/module_socket.c
Outdated
| @@ -9,6 +9,7 @@ | |||
| #ifdef _WIN32 | |||
| #include <winsock2.h> | |||
| #include <ws2tcpip.h> | |||
| #undef AF_UNIX | |||
There was a problem hiding this comment.
Please do not include unrelated changes in pull requests.
There was a problem hiding this comment.
Additionally, Windows has supported AF_UNIX for some time now, though I'm not sure where they're defining the relevant structs...
There was a problem hiding this comment.
They might be in afunix.h.
https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
src/modules/module_fcntl.c
Outdated
| KRK_DOC(module, "@brief Provides access to file control functions."); | ||
|
|
||
| KRK_DOC(BIND_FUNC(module,fcntl), | ||
| "@brief Modify file fd with command cmd\n" |
There was a problem hiding this comment.
fcntl does a great many things and this is not a particularly good description of it.
src/modules/module_fcntl.c
Outdated
| "@brief Modify file fd with command cmd\n" | ||
| "@arguments fd,cmd,arg=0\n\n" | ||
| "@p cmd should be an integer value defined by the @c F options. " | ||
| "@p arg should be an integer value defined by the @ref mod_os @c O options."); |
There was a problem hiding this comment.
Many fcntl commands take a pointer to a struct of some form, and even those that take integers are mostly not the O_ file descriptor options.
| FCNTL_CONST(F_GETFL); | ||
| FCNTL_CONST(F_SETFL); |
There was a problem hiding this comment.
Why just these two? Why not all the ones defined in POSIX?
src/modules/module_fcntl.c
Outdated
| #include <kuroko/value.h> | ||
| #include <kuroko/util.h> | ||
|
|
||
| #ifdef _WIN32 |
There was a problem hiding this comment.
This should really be handled by excluding this module from the build targets on Windows in the Makefile.
src/modules/module_fcntl.c
Outdated
| return krk_runtimeError(KRK_EXC(typeError), | ||
| "expected integer or object with fileno(), not '%T'", fileno); | ||
| } | ||
| int result = fcntl(fd, cmd, arg); |
There was a problem hiding this comment.
While not likely to be relevant for the two exposed commands, fcntl can be interrupted for many commands, and going by what CPython does it would be worthwhile to catch EINTR and retry.
|
I have updated the author name and have rewritten the commit to address the feedback.
I don't use Kuroko for much currently. I want to use fcntl for setting blocking/non-blocking mode on sockets. |
|
This is looking much better. I don't want to add a new module at this stage in the 1.4 release schedule, and I don't have a separate development / release branching strategy at the moment, so I'll wait to merge until 1.4 is finished. I will try to fix the Windows build of the socket module in 1.4, though. |
|
Sounds good. |
The socket module wouldn't compile when I tested cross compiling because it couldn't find
sys/un.h, so I disabledAF_UNIXsockets on Windows.