> 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);
>
> 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.
>
> Nilay Vaish wrote:
> 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().
We're not assuming no caller passes -1. We're assuming that if you are passing
-1 you are doing so intentionally, and therefore any callers that do not wish
to pass -1 should check their fd before calling, which is a fine assumption.
Our CLDriver passes -1.
CLDriver::open() looks like this, and it handles setting the driver pointer in
the FDE:
int
ClDriver::open(LiveProcess *p, ThreadContext *tc, int mode, int flags)
{
int fd = p->allocFD(-1, filename, 0, 0, false);
FDEntry *fde = p->getFDEntry(fd);
fde->driver = this;
return fd;
}
We could move allocFD() out of the driver's open() call and into syscall emul's
open() call and have the driver return the desired fd it wants passed to
allocFD(), but this would require completely redefining the
EmulatedDriver::open() call's semantics. This is unnecessary code reorg. From
the comment in emulated_driver.hh
/**
* Abstract method, invoked when the user program calls open() on
* the device driver. The parameters are the same as those passed
* to openFunc() (q.v.).
* @return A newly allocated target fd, or -1 on error.
*/
Unless you have an objection other than it's simply your preference to move
allocFD() out of the emulated driver, we will ship this as-is. We don't need to
duplicate what the kernel internals would actually do, this is SE mode, we just
want to emulate functionality.
- Tony
-----------------------------------------------------------
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
