----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3676/#review8934 -----------------------------------------------------------
src/sim/fd_array.hh (line 65) <http://reviews.gem5.org/r/3676/#comment7680> 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 (line 74) <http://reviews.gem5.org/r/3676/#comment7678> 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 (line 140) <http://reviews.gem5.org/r/3676/#comment7679> 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 (line 262) <http://reviews.gem5.org/r/3676/#comment7681> seems like this could be inlined in the header src/sim/fd_array.cc (line 302) <http://reviews.gem5.org/r/3676/#comment7682> this is a good candidate for inlining too src/sim/process.cc (line 245) <http://reviews.gem5.org/r/3676/#comment7683> 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 (line 526) <http://reviews.gem5.org/r/3676/#comment7684> 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 (line 995) <http://reviews.gem5.org/r/3676/#comment7685> 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 (line 997) <http://reviews.gem5.org/r/3676/#comment7686> 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(), or B. 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. - Steve Reinhardt On Oct. 18, 2016, 1:01 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3676/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2016, 1:01 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11699:c22ac97e3372 > --------------------------- > 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. > > > Diffs > ----- > > src/kern/tru64/tru64.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/fd_array.hh PRE-CREATION > src/sim/fd_array.cc PRE-CREATION > src/sim/fd_entry.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/fd_entry.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > > Diff: http://reviews.gem5.org/r/3676/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
