Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Tony Gutierrez]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review9280
---

Ship it!


Ship It!

- Tony Gutierrez


On Nov. 16, 2016, 9:59 a.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3676/
> ---
> 
> (Updated Nov. 16, 2016, 9:59 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11717:7f3bf41cce5c
> ---
> 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 c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_array.hh PRE-CREATION 
>   src/sim/fd_array.cc PRE-CREATION 
>   src/sim/SConscript c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/gpu-compute/cl_driver.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/kern/tru64/tru64.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/sparc/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> 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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review9278
---

Ship it!


Ship It!

- Michael LeBeane


On Nov. 16, 2016, 5:59 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3676/
> ---
> 
> (Updated Nov. 16, 2016, 5:59 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11717:7f3bf41cce5c
> ---
> 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 c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_array.hh PRE-CREATION 
>   src/sim/fd_array.cc PRE-CREATION 
>   src/sim/SConscript c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/gpu-compute/cl_driver.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/kern/tru64/tru64.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/sparc/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> 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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review9137
---



src/sim/process.cc (line 228)


Please add in a warning.



src/sim/process.cc (line 249)


Please add in a warning.



src/sim/syscall_emul.hh (line 989)


I see you added getSimFD() to the base class, but it still looks like you 
have some unnecessary dynamic casts laying around.


- Michael LeBeane


On Nov. 16, 2016, 5:59 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3676/
> ---
> 
> (Updated Nov. 16, 2016, 5:59 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11717:7f3bf41cce5c
> ---
> 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 c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_entry.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/sim/fd_array.hh PRE-CREATION 
>   src/sim/fd_array.cc PRE-CREATION 
>   src/sim/SConscript c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/gpu-compute/cl_driver.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/kern/tru64/tru64.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
>   src/arch/sparc/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
> 
> 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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/
---

(Updated Nov. 16, 2016, 5:59 p.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 11717:7f3bf41cce5c
---
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 (updated)
-

  src/sim/syscall_emul.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/fd_entry.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/fd_entry.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/sim/fd_array.hh PRE-CREATION 
  src/sim/fd_array.cc PRE-CREATION 
  src/sim/SConscript c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/gpu-compute/cl_driver.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/kern/tru64/tru64.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 
  src/arch/sparc/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 

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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

> On Oct. 19, 2016, 7:29 p.m., Steve Reinhardt wrote:
> > src/sim/fd_array.hh, line 74
> > 
> >
> > 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.

Steve, I'd like to drop this unless you're absolutely against me doing so.

The implementation may look a little weird, but the copy constructor largely 
contains the std::shared_ptr. In my opinion, it's better that the shared_ptr 
isn't visible outside the class. The object assignment looks like it's a normal 
object copy constructor being called rather than making it look like a pointer 
assignment.


> On Oct. 19, 2016, 7:29 p.m., Steve Reinhardt wrote:
> > src/sim/syscall_emul.hh, line 526
> > 
> >
> > I don't think this is true... the whole ENOTTY thing is for stdlib to 
> > figure out if stdout is buffered or not.

OK, wasn't aware of that. I removed the comment and put the tty check at the 
top of the function with the device check after.


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review8934
---


On Oct. 18, 2016, 8: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, 8: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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

> On Oct. 19, 2016, 7:29 p.m., Steve Reinhardt wrote:
> > src/sim/fd_array.hh, line 140
> > 
> >
> > Can't these be static variables and not per-instance? I don't see where 
> > they're modified after they're constructed.

I don't think that it's possible to initialize static variables with 
aggregates. I tried following through with this recommendation but I get 
"non-constant in-class initialization invalid for static member". I think that 
the aggregate initialization must be done at runtime, but for statics the 
resolution has to be done at compile time (just guessing).


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review8934
---


On Oct. 18, 2016, 8: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, 8: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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Steve Reinhardt]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review8934
---



src/sim/fd_array.hh (line 65)


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)


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)


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)


seems like this could be inlined in the header



src/sim/fd_array.cc (line 302)


this is a good candidate for inlining too



src/sim/process.cc (line 245)


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)


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)


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)


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 

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote:
> > src/sim/fd_entry.hh, line 140
> > 
> >
> > Can you rename this to something more descriptive, like 
> > HostMappedFDEntry?
> 
> Brandon Potter wrote:
> The only issue is that pipes are "host mapped" too. How about 
> FileBackedFDEntry?
> 
> Michael LeBeane wrote:
> Sure, that's even better.

I went with "FileFDEntry". It's not too long and still conveys the same point. 
It's annoying that it's file-file-descriptor, but fd is a misnomer. It points 
to so much stuff that isn't really a file.


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review8881
---


On Oct. 18, 2016, 8: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, 8: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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/
---

(Updated Oct. 18, 2016, 8:01 p.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

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 (updated)
-

  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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/
---

(Updated Oct. 18, 2016, 7:57 p.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 11699:8239e863b624
---
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 (updated)
-

  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
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Michael LeBeane]

> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote:
> > src/sim/process.cc, line 447
> > 
> >
> > Why remove?  Does this not work anymore?
> 
> Brandon Potter wrote:
> The subscript accessor returns the fdentry directly so it has to discard 
> the const qualifier for the (un)serialize methods.
> 
> Also, the checkpoints are broken so I'd like to revisit getting them 
> working a single patch or a subset of patches. I don't think it's 
> constructive to try to attack the checkpoint problem without having a clear 
> goal on how to handle the outliers and things that make it general in 
> difficult. For now, I'm ignoring the feature.
> 
> If you spend enough time looking at this patch and some of the subsequent 
> ones (look at the fcntl patch and the flags field for an example), it becomes 
> obvious that several of the fields in these classes are solely meant to hold 
> the metadata needed for checkpoints. This seems contradictory given that I'm 
> ignoring the feature. My goal right now is to make it apparent what types 
> we're dealing with in the system call code, but also to make it possible to 
> do checkpoints properly in the future by giving the metadata a structure to 
> reside in.

Ok sure.  I don't mind that checkpoints are a work in progress, but I would 
suggest adding some warnings since the code is only partially implemented.


- Michael


---
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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote:
> > src/sim/process.cc, line 447
> > 
> >
> > Why remove?  Does this not work anymore?

The subscript accessor returns the fdentry directly so it has to discard the 
const qualifier for the (un)serialize methods.

Also, the checkpoints are broken so I'd like to revisit getting them working a 
single patch or a subset of patches. I don't think it's constructive to try to 
attack the checkpoint problem without having a clear goal on how to handle the 
outliers and things that make it general in difficult. For now, I'm ignoring 
the feature.

If you spend enough time looking at this patch and some of the subsequent ones 
(look at the fcntl patch and the flags field for an example), it becomes 
obvious that several of the fields in these classes are solely meant to hold 
the metadata needed for checkpoints. This seems contradictory given that I'm 
ignoring the feature. My goal right now is to make it apparent what types we're 
dealing with in the system call code, but also to make it possible to do 
checkpoints properly in the future by giving the metadata a structure to reside 
in.


- 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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Michael LeBeane]

> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote:
> > src/sim/fd_entry.hh, line 140
> > 
> >
> > Can you rename this to something more descriptive, like 
> > HostMappedFDEntry?
> 
> Brandon Potter wrote:
> The only issue is that pipes are "host mapped" too. How about 
> FileBackedFDEntry?

Sure, that's even better.


- Michael


---
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

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

> On Oct. 17, 2016, 10:17 p.m., Michael LeBeane wrote:
> > src/sim/fd_array.cc, line 75
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 

Re: [gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Michael LeBeane]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/#review8881
---



src/sim/fd_array.hh (line 109)


This seems weird.  I'm pretty sure the '=' operator has access to all the 
private members and that this is not needed.



src/sim/fd_array.cc (line 75)


Do you need to cast here?



src/sim/fd_array.cc (line 88)


same here



src/sim/fd_array.cc (line 99)


same here



src/sim/fd_entry.hh (line 67)


Maybe mark as TODO?  Should also put a warn when trying to (un)serialize if 
you don't think it will work correctly.



src/sim/fd_entry.hh (line 128)


Can you rename this to something more descriptive, like HostMappedFDEntry?



src/sim/fd_entry.hh (line 156)


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.



src/sim/fd_entry.cc (line 33)


Should probably just add your name here, not delete the others.



src/sim/process.cc (line 245)


Why remove?  Does this not work anymore?



src/sim/syscall_emul.hh (line 631)


These downcasts are messy, see above for suggestion on getting rid of them.


- Michael LeBeane


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

[gem5-dev] Review Request 3676: syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

[Brandon Potter]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3676/
---

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