syscall_emul: [patch 10/22] refactor fdentry and add fdarray class

Information
Submitter:	Brandon Potter
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

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.

Testing Done

Issue Summary

Description	From	Last Updated	Status
This seems weird. I'm pretty sure the '=' operator has access to all the private members and that this is ...	Michael LeBeane	Oct. 18, 2016, 12:57 p.m.	Resolved
Do you need to cast here?	Michael LeBeane	Oct. 18, 2016, 11:56 a.m.	Resolved
same here	Michael LeBeane	Oct. 18, 2016, 11:56 a.m.	Resolved
same here	Michael LeBeane	Oct. 18, 2016, 11:56 a.m.	Resolved
Can you rename this to something more descriptive, like HostMappedFDEntry?	Michael LeBeane	Oct. 18, 2016, 12:27 p.m.	Resolved
Why remove? Does this not work anymore?	Michael LeBeane	Oct. 31, 2016, 8:19 a.m.	Dropped
These downcasts are messy, see above for suggestion on getting rid of them.	Michael LeBeane	Nov. 16, 2016, 8:45 a.m.	Resolved
I suggest calling this setFileOffsets() since it ends up calling setFileOffset() on each FDEntry. Or maybe updateFileOffsets(), if you think ...	Steve Reinhardt	Nov. 16, 2016, 8:58 a.m.	Resolved
Why doesn't a process that shares the same file descriptors just share the whole FDArray object? This internal shared pointer ...	Steve Reinhardt	Nov. 16, 2016, 9:43 a.m.	Dropped
Can't these be static variables and not per-instance? I don't see where they're modified after they're constructed.	Steve Reinhardt	Nov. 16, 2016, 9:12 a.m.	Dropped
if you're leaving this code in for it to be fixed later, it needs a comment to that effect, and ...	Steve Reinhardt	Nov. 16, 2016, 9:26 a.m.	Resolved
I don't think this is true... the whole ENOTTY thing is for stdlib to figure out if stdout is buffered ...	Steve Reinhardt	Nov. 16, 2016, 9:55 a.m.	Resolved
This seems unnecessarilly verbose. Unless you actually need fda somewhere else in the function, just do process->fds[tgt_fd] I'd say the ...	Steve Reinhardt	Nov. 16, 2016, 8:46 a.m.	Resolved
I agree with Michael, these dynamic casts are ugly. I think you should either: A. Go all the way and ...	Steve Reinhardt	Nov. 16, 2016, 8:52 a.m.	Resolved
Please add in a warning.	Michael LeBeane	Jan. 17, 2017, 9:02 a.m.	Resolved
Please add in a warning.	Michael LeBeane	Jan. 17, 2017, 9:02 a.m.	Resolved
I see you added getSimFD() to the base class, but it still looks like you have some unnecessary dynamic casts ...	Michael LeBeane	Jan. 17, 2017, 9:09 a.m.	Dropped

src/sim/fd_array.hh (Diff revision 1)

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

Show all issues

src/sim/fd_array.cc (Diff revision 1)

Do you need to cast here?

Show all issues

Brandon Potter Oct. 18, 2016, 12:09 p.m. (Oct. 18, 2016, 12:09 p.m.)
```
Nice, thanks.
```

src/sim/fd_array.cc (Diff revision 1)

same here

Show all issues

src/sim/fd_array.cc (Diff revision 1)

same here

Show all issues

src/sim/fd_entry.hh (Diff revision 1)

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

Brandon Potter Oct. 18, 2016, 12:09 p.m. (Oct. 18, 2016, 12:09 p.m.)
```
Good recommendations.
```

src/sim/fd_entry.hh (Diff revision 1)

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

Show all issues

Brandon Potter Oct. 18, 2016, 12:09 p.m. (Oct. 18, 2016, 12:09 p.m.)

The only issue is that pipes are "host mapped" too. How about FileBackedFDEntry?

Michael LeBeane Oct. 18, 2016, 12:20 p.m. (Oct. 18, 2016, 12:20 p.m.)
```
Sure, that's even better.
```

Brandon Potter Oct. 18, 2016, 1:04 p.m. (Oct. 18, 2016, 1:04 p.m.)

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.

src/sim/fd_entry.hh (Diff revision 1)

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.

Brandon Potter Oct. 18, 2016, 12:09 p.m. (Oct. 18, 2016, 12:09 p.m.)

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

src/sim/fd_entry.cc (Diff revision 1)

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

Brandon Potter Oct. 18, 2016, 12:09 p.m. (Oct. 18, 2016, 12:09 p.m.)

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.

src/sim/process.cc (Diff revision 1)

Why remove? Does this not work anymore?

Show all issues

Brandon Potter Oct. 18, 2016, 12:21 p.m. (Oct. 18, 2016, 12:21 p.m.)

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.

Michael LeBeane Oct. 18, 2016, 12:27 p.m. (Oct. 18, 2016, 12:27 p.m.)

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.

src/sim/syscall_emul.hh (Diff revision 1)

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

Show all issues

Description:

~		Changeset 11698:502f94aa9638
	~	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.

Diff:

Revision 2 (+972 -432)

Show changes

	src/kern/tru64/tru64.hh
	src/sim/SConscript
	src/sim/fd_array.hh
	src/sim/fd_array.cc
	src/sim/fd_entry.hh
	src/sim/fd_entry.cc
	src/sim/process.hh
	src/sim/process.cc
	2 more

Description:

~		Changeset 11699:8239e863b624
	~	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.

Diff:

Revision 3 (+962 -432)

Show changes

	src/kern/tru64/tru64.hh
	src/sim/SConscript
	src/sim/fd_array.hh
	src/sim/fd_array.cc
	src/sim/fd_entry.hh
	src/sim/fd_entry.cc
	src/sim/process.hh
	src/sim/process.cc
	2 more

src/sim/fd_array.hh (Diff revision 3)

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.

Show all issues

Brandon Potter Nov. 16, 2016, 8:59 a.m. (Nov. 16, 2016, 8:59 a.m.)
```
I went with update.
```

src/sim/fd_array.hh (Diff revision 3)

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.

Show all issues

Brandon Potter Nov. 16, 2016, 9:57 a.m. (Nov. 16, 2016, 9:57 a.m.)

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.

src/sim/fd_array.hh (Diff revision 3)

Can't these be static variables and not per-instance? I don't see where they're modified after they're constructed.

Show all issues

Brandon Potter Nov. 16, 2016, 9:15 a.m. (Nov. 16, 2016, 9:15 a.m.)

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

src/sim/fd_array.cc (Diff revision 3)

seems like this could be inlined in the header

src/sim/fd_array.cc (Diff revision 3)

this is a good candidate for inlining too

src/sim/process.cc (Diff revision 3)

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.

Show all issues

src/sim/syscall_emul.hh (Diff revision 3)

I don't think this is true... the whole ENOTTY thing is for stdlib to figure out if stdout is buffered or not.

Show all issues

Brandon Potter Nov. 16, 2016, 9:57 a.m. (Nov. 16, 2016, 9:57 a.m.)

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.

src/sim/syscall_emul.hh (Diff revision 3)

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

Show all issues

src/sim/syscall_emul.hh (Diff revision 3)

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.

Show all issues

Brandon Potter Nov. 16, 2016, 8:59 a.m. (Nov. 16, 2016, 8:59 a.m.)
```
I went with B. and added sim_fd to the base class.
```

Description:

~		Changeset 11699:c22ac97e3372
	~	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.

Diff:

Revision 4 (+950 -507)

Show changes

	src/arch/sparc/process.hh
	src/gpu-compute/cl_driver.cc
	src/kern/tru64/tru64.hh
	src/sim/SConscript
	src/sim/fd_array.hh
	src/sim/fd_array.cc
	src/sim/fd_entry.hh
	src/sim/fd_entry.cc
	4 more

src/sim/process.cc (Diff revision 4)

Please add in a warning.

Show all issues

src/sim/process.cc (Diff revision 4)

Please add in a warning.

Show all issues

src/sim/syscall_emul.hh (Diff revision 4)

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

Show all issues

Ship It!

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11698:502f94aa9638

Changeset 11699:8239e863b624

Diff:

Description:

Changeset 11699:8239e863b624

Changeset 11699:c22ac97e3372

Diff:

Description:

Changeset 11699:c22ac97e3372

Changeset 11717:7f3bf41cce5c

Diff:

Status: Closed (submitted)