> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote: > > src/sim/fd_array.cc, line 75 > > <http://reviews.gem5.org/r/3676/diff/1/?file=61680#file61680line75> > > > > Do you need to cast here?
Nice, thanks. > On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote: > > src/sim/fd_entry.hh, line 79 > > <http://reviews.gem5.org/r/3676/diff/1/?file=61681#file61681line79> > > > > Maybe mark as TODO? Should also put a warn when trying to > > (un)serialize if you don't think it will work correctly. Good recommendations. > On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote: > > src/sim/fd_entry.cc, line 33 > > <http://reviews.gem5.org/r/3676/diff/1/?file=61682#file61682line33> > > > > Should probably just add your name here, not delete the others. I was the one who wrote the original implementation of fd_entry so the other names probably shouldn't have been there in the first place. Steve reviewed fd_entry and was aware of it, but I doubt that Nate would want to be associated with that particular piece of code. Anyways, I wrote all of the code and it should have been only my name originally. On top of that, I practically rewrote everything for this iteration. > On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote: > > src/sim/fd_entry.hh, line 140 > > <http://reviews.gem5.org/r/3676/diff/1/?file=61681#file61681line140> > > > > Can you rename this to something more descriptive, like > > HostMappedFDEntry? The only issue is that pipes are "host mapped" too. How about FileBackedFDEntry? > On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote: > > src/sim/fd_entry.hh, line 168 > > <http://reviews.gem5.org/r/3676/diff/1/?file=61681#file61681line168> > > > > 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. Yes, the downcasts in the syscall code are the one thing that I really dislike about the patch. However, this is the solution that I wanted to avoid. (I'm hopefulthat someone has an alternative, because I haven't thought of one yet.) I'd like to be able to identify the objects by individual classes by downcasting instead of an alternative (like an enum with type information), but I don't want to downcast too often because it's annoying in the general case (file-backed fd). As you point out, the DeviceFDEntry doesn't have a sim_fd field which is why the accessor method and that field aren't there. That being said, I might add it to the base class and leave it uninitialized for the DeviceFDEntry, but that kind of defeats the purpose for giving these individual types. (The original code doesn't make a distinction between the types; it has to be inferred by the context. Many of the fields were left uninitialized so it was bug prone.) Anyways, this is pertinent. Thanks for highlighting it. (I might end up going this route.) - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3676/#review8881 ----------------------------------------------------------- On Oct. 17, 2016, 3:41 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3676/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2016, 3:41 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11698:502f94aa9638 > --------------------------- > 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/sim/syscall_emul.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.hh 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/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/tru64/tru64.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > > Diff: http://reviews.gem5.org/r/3676/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
