syscall_emul: [patch 10/22] refactor fdentry and add fdarray class
Review Request #3676 - Created Oct. 17, 2016 and submitted
Information | |
---|---|
Brandon Potter | |
gem5 | |
default | |
Reviewers | |
Default | |
Changeset 11717:7f3bf41cce5c --------------------------- syscall_emul: [patch 10/22] refactor fdentry and add fdarray class Several large changes happen in this patch. The FDEntry class is rewritten so that file descriptors now correspond to types: 'Regular' which is normal file-backed file with the file open on the host machine, 'Pipe' which is a pipe that has been opened on the host machine, and 'Device' which does not have an open file on the host yet acts as a pseudo device with which to issue ioctls. Other types which might be added in the future are directory entries and sockets (off the top of my head). The FDArray class was create to hold most of the file descriptor handling that was stuffed into the Process class. It uses shared pointers and the std::array type to hold the FDEntries mentioned above. The implementation could use a review; I feel that there's some room for improvement, but it seems like a decent first step. The changes to these two classes needed to be propagated out to the rest of the code so there were quite a few changes for that. Also, comments were added where I thought they were needed to help others and extend our DOxygen coverage.
-
src/sim/fd_array.hh (Diff revision 1) -
This seems weird. I'm pretty sure the '=' operator has access to all the private members and that this is not needed.
-
src/sim/fd_array.cc (Diff revision 1) -
Do you need to cast here?
-
src/sim/fd_array.cc (Diff revision 1) -
same here
-
src/sim/fd_array.cc (Diff revision 1) -
same here
-
src/sim/fd_entry.hh (Diff revision 1) -
Maybe mark as TODO? Should also put a warn when trying to (un)serialize if you don't think it will work correctly.
-
src/sim/fd_entry.hh (Diff revision 1) -
Can you rename this to something more descriptive, like HostMappedFDEntry?
-
src/sim/fd_entry.hh (Diff revision 1) -
If you move simFD to the base class you can avoid a lot of your downcasts later. I don't think it's a big deal that DeviceFDEntry doesn't use it.
-
src/sim/fd_entry.cc (Diff revision 1) -
Should probably just add your name here, not delete the others.
-
src/sim/process.cc (Diff revision 1) -
Why remove? Does this not work anymore?
-
src/sim/syscall_emul.hh (Diff revision 1) -
These downcasts are messy, see above for suggestion on getting rid of them.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+972 -432) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+962 -432) |
-
src/sim/fd_array.hh (Diff revision 3) -
I suggest calling this setFileOffsets() since it ends up calling setFileOffset() on each FDEntry. Or maybe updateFileOffsets(), if you think 'set' is ambiguous. To me, 'find' doesn't imply a state update, so it's not as clear what this is doing.
-
src/sim/fd_array.hh (Diff revision 3) -
Why doesn't a process that shares the same file descriptors just share the whole FDArray object? This internal shared pointer thing is kinda awkward... which is OK if it's needed, but I don't understand why.
-
src/sim/fd_array.hh (Diff revision 3) -
Can't these be static variables and not per-instance? I don't see where they're modified after they're constructed.
-
src/sim/fd_array.cc (Diff revision 3) -
seems like this could be inlined in the header
-
src/sim/fd_array.cc (Diff revision 3) -
this is a good candidate for inlining too
-
src/sim/process.cc (Diff revision 3) -
if you're leaving this code in for it to be fixed later, it needs a comment to that effect, and would be cleaner if you put the whole loop inside "#if 0" rather than just commenting out the body.
-
src/sim/syscall_emul.hh (Diff revision 3) -
I don't think this is true... the whole ENOTTY thing is for stdlib to figure out if stdout is buffered or not.
-
src/sim/syscall_emul.hh (Diff revision 3) -
This seems unnecessarilly verbose. Unless you actually need fda somewhere else in the function, just do
process->fds[tgt_fd]
I'd say the same for fdp too; why define it if it's only going to be used once, unless (perhaps) there's a line length issue with the dynamic cast, but I hope that goes away as well -
src/sim/syscall_emul.hh (Diff revision 3) -
I agree with Michael, these dynamic casts are ugly. I think you should either:
A. Go all the way and define virtual functions on FDEntry to implement all the fd-related syscalls, so you can just replace the whole body of this function with
process->fds[tgt_fd]->fchmod(bufPtr)
(or something like that), and put what used to be the body of this function in FileFDEntry::fchmod(), orB. Back off, put sim_fd back in FDentry, and just have the common syscalls like this one be oblivious to the FDEntry subclass. Presumably the host will do the right thing for pipes/sockets vs. files, and if you force sim_fd to -1 for emulated devices, you'll automatically get EBADF for those without putting in any additional checks.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+950 -507) |
-
src/sim/process.cc (Diff revision 4) -
Please add in a warning.
-
src/sim/process.cc (Diff revision 4) -
Please add in a warning.
-
src/sim/syscall_emul.hh (Diff revision 4) -
I see you added getSimFD() to the base class, but it still looks like you have some unnecessary dynamic casts laying around.
Ship It!
Ship It!