Fix dup/dup2 + Fix fcntl + New fdtables API #117
Conversation
|
The changes look good and nice comment updates. What I think would really be helpful here is a longerform comment explaining why we call to the kernel for dup here even though were virtually handling file descriptors. There definitely was a decision/reason for that made at some point but I forget all the details so was struggling to reason through this, seems like an issue that deserves some detail for people looking at this in the future. |
|
The way I implemented fd tracking didn't take fcntl's behavior into account. I'm fine with what you've done. One question is what real world code uses fcntl like this and would break if we did dup2 / grabbed an available fd (but not the lowest) instead? We're likely to be much, much, much slower than native here. |
Theoretically, the new API shouldn't introduce additional performance overhead compared to dup2 or grabbing an available fd. The only difference is just offset by the different starting point in the FD search. The rest of the logic remains essentially the same. Motivated by this, I started doing some benchmarks (#62) to better understand the performance in lind-wasm to help optimize implementation |
|
This looks good to me now but we should be sure as to why this is failing the CI (I know its a WIP for now but still is useful). When it's failing early like this it usually means RawPOSIX isn't building correctly so lets confirm that. |
I can run unit-test on server. I'm going to check about CI. |
@rennergade, I talked with @m-hemmings about the failure. This PR actually passed all checks, but it took over an hour, so it timed out and reported an error. This should stop happening once we merge #154. |
|
Ok great I appreciate you looking into this.
|
There was a problem hiding this comment.
Approved. Should to be good to merge with approval from @JustinCappos
This PR addresses issues related to
dup/dup2andfcntl. Related to bug reported in #112Implementation details
dup/dup2:
Since RawPOSIX introduces the concept of virtual file descriptors through fdtables, duplicating a file descriptor using dup/dup2 in RawPOSIX does not require calling the underlying kernel syscall. Instead, it only needs to ensure that the two virtual fds point to the same underlying kernel fd.
One consideration is that the O_CLOEXEC flag is not automatically inherited when duplicating a file descriptor. Therefore, we need to explicitly set it to false for the duplicated fd.
fcntl:
Due to the design of fdtables, we need to handle F_DUPFD, F_DUPFD_CLOEXEC, F_GETFD, and F_SETFD separately. The operations F_DUPFD and F_DUPFD_CLOEXEC cannot directly use RawPOSIX dup/dup2 calls because fcntl allocates the lowest available fd starting from the given argument.
To accommodate this behavior, I added a new fdtables API specifically for handling these cases.
fdtables:
As mentioned above, I introduced a new API in all four algorithms in fdtables to support fcntl operations. I considered to change arguments list of original API, but this will cause modification spread to almost entire RawPOSXI codebase, since get_unused_virtual_fd is used in almost every fs syscalls. We could mark this as a TODO if we want future optimization
Test
Passed unit test:
Added new test file: