> On Nov. 14, 2015, 8:50 a.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);
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. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3188/#review7583 ----------------------------------------------------------- On Oct. 30, 2015, 2: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, 2: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
