Fix descriptor bugs and support dup() on regular files for Python#1257
Fix descriptor bugs and support dup() on regular files for Python#1257stevenengler merged 4 commits intoshadow:devfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1257 +/- ##
==========================================
- Coverage 55.68% 55.64% -0.04%
==========================================
Files 142 142
Lines 20508 20562 +54
Branches 5031 5053 +22
==========================================
+ Hits 11419 11442 +23
- Misses 5984 5992 +8
- Partials 3105 3128 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
911900b to
2c2c405
Compare
robgjansen
left a comment
There was a problem hiding this comment.
Thanks for organizing the commits - that made it easy to review :)
Requesting changes, primarily to address the NULL pointer issue.
src/main/host/descriptor/file.c
Outdated
| descriptor_adjustStatus(&file->super, STATUS_DESCRIPTOR_ACTIVE, TRUE); | ||
|
|
||
| return _file_getFD(file); | ||
| return 0; |
There was a problem hiding this comment.
Add a comment here, or document in the header file what file_open and file_openat are expected to return. (It's useful since the return logic is slightly different from open and openat.)
There was a problem hiding this comment.
I changed the return value back, but made it an error to call file_openat() if you haven't registered the descriptor yet.
| LegacyDescriptor* desc = process_getRegisteredLegacyDescriptor(sys->process, fd); | ||
|
|
||
| LegacyDescriptorType dType = descriptor_getType(desc); |
There was a problem hiding this comment.
What if the plugin passes in a bad FD? I think desc will be NULL then, and descriptor_getType will fail the magic assertion.
Instead, do we want to return -EBADF if desc is NULL?
For example, _syscallhandler_readHelper does this:
/* Get the descriptor. */
LegacyDescriptor* desc = process_getRegisteredLegacyDescriptor(sys->process, fd);
if (!desc) {
return (SysCallReturn){.state = SYSCALL_DONE, .retval.as_i64 = -EBADF};
}There was a problem hiding this comment.
Thanks, I forgot to check this.
| warning("Cannot dup legacy descriptors"); | ||
| return (SysCallReturn){.state = SYSCALL_DONE, .retval.as_i64 = -EOPNOTSUPP}; | ||
| const SysCallArgs* args) { | ||
| gint fd = args->args[0].as_i64; |
There was a problem hiding this comment.
I generally like to add a debug message right after setting the fd in this line, to log the fd that we got on the syscall, before doing other stuff that may cause problems.
src/test/file/test_file.c
Outdated
| assert_nonneg_errno(fd = open(adf.name, O_RDONLY)); | ||
| assert_nonneg_errno(fd2 = dup(fd)); |
There was a problem hiding this comment.
Do we want to expand this test, to make sure that something we wrote to fd before the dup is actually readable from fd2 after the dup?
There was a problem hiding this comment.
I added more to this test.
Fixes a few descriptor bugs and implements
dup()for regular files, which is required for Python. (Python usesdup()on stdin, stdout, and stderr to check if they are valid descriptors. Seecreate_stdio()in CPython.)Shadow is still missing syscalls to use things like the built-in http server (I see futex errors, missing socket options, and IPv6 issues). Python also make heavy use of
select(), so things liketime.sleep()don't work in Shadow.