> On Nov. 14, 2015, 4:50 p.m., Tony Gutierrez wrote: > > I will be shipping this today unless somebody voices an objection. > > Nilay Vaish wrote: > Do you think it would be better if the function took the driver variable > as an input param? > May be we can strengthen the if() condition to: abort_if(fd == -1 && > driver == nullptr); > > Steve Reinhardt wrote: > I don't think that would be useful---as the commit message says, every > caller already checks sim_fd before calling this function, so the current > check is useless, and the way that we're signaling that we really do want to > allocate a file descriptor with no host equivalent is by passing in sim_fd == > -1. Adding another parameter just to say "yes I really mean it" seems > unnecessarily redundant to me. > > Note that sim_fd isn't even used in this function other than to copy its > value into the FDEntry table, so there's actually no harm in the value being > -1 at this point. As I said in the commit message, I think the original > check was well intentioned but misguided and useless, and all that's happened > is that it's gone from being useless to being in the way of a reasonable use > case.
Is it fine to assume that since no caller is currently passing on a -1 as fd, no one will do so in future? As I understand, you are calling allocFD from the driver you have implemented. So the current code is: 1. application calls open on device d 2. syscall emul open() calls open on device d. 3. device d calls allocFD. I am not even sure where you set the driver pointer in the FDEntry object. I would rather have: 1. application calls open 2. syscall emul open() opens the device and receives an fd in return. 3. syscall emul open() calls allocFD and sets the driver pointer in the FDEntry object. I would be surprised if the Linux kernel makes device drivers call allocFD(). - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3188/#review7583 ----------------------------------------------------------- On Oct. 30, 2015, 9:52 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3188/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2015, 9:52 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11194:c55d82df60b4 > --------------------------- > syscall_emul: don't check host fd when allocating target fd > > There's a well-meaning check in Process::allocFD() to return an invalid > target fd (-1) if the incoming host fd is -1. However, this means that > emulated drivers, which want to allocate a target fd that doesn't > correspond to a host fd, can't use -1 to indicate an intentionally > invalid host fd. > > It turns out the allocFD() check is redundant, as callers always test > the host fd for validity before calling. Also, callers never test the > return value of allocFD() for validity, so even if the test failed, > it would likely have the undesirable result of returning -1 to the > target app as a file descriptor without setting errno. > > Thus the check is pointless and is now getting in the way, so it seems > we should just get rid of it. > > > Diffs > ----- > > src/sim/process.cc 4daf60db14d794e2344a6c86a93bdd8273bc5bb6 > > Diff: http://reviews.gem5.org/r/3188/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
