Linus Torvalds <torva...@linux-foundation.org> writes: > So I've looked at the devpts patches some more, and I'm still not happy > with them. > > And one thing I really detest about them is that the 16-patch series > didn't really make me warm and fuzzy in general. Some of the patches were > fine, but on the whole it all still looked rather hacky. > > So I started looking at the code with the intent of trying to clean things > up _gradually_, knowing roughly where we want to end up, but also trying > to make single patches that look sane on their own, and can be judged on > their own without any other patches or even any semantic arguments. > > And this appended patch is I think the first step. > > What this does is get rid of the horrible notion of having that > > struct inode *ptmx_inode > > be the interface between the pty code and devpts. By de-emphasizing the > ptmx inode, a lot of things actually get cleaner, and we will have a much > saner way forward. > > The patch itself is actually fairly straightforward, and apart from some > locking cleanups it's pretty mechanical: > > - the interfaces that devpts exposes all take "struct pts_fs_info *" > instead of "struct inode *ptmx_inode" now. > > NOTE! The "struct pts_fs_info" thing is a completely opaque structure > as far as the pty driver is concerned: it's still declared entirely > internally to devpts. So the pty code can't actually access it in any > way, just pass it as a "cookie" to the devpts code. > > - the "look up the pts fs info" is now a single clear operation, that > also does the reference count increment on the pts superblock. > > So "devpts_add/del_ref()" is gone, and replaced by a "lookup and get > ref" operation (devpts_get_ref(inode)), along with a "put ref" op > (devpts_put_ref()). > > - the pty master "tty->driver_data" field now contains the pts_fs_info, > not the ptmx inode. > > - because we don't care about the ptmx inode any more as some kind of > base index, the ref counting can now drop the inode games - it just > gets the ref on the superblock. > > - the pts_fs_info now has a back-pointer to the super_block. That's so > that we can easily look up the information we actually need. Although > quite often, the pts fs info was actually all we wanted, and not having > to look it up based on some magical inode makes things more > straightforward. > > Now, I haven't actually *tested* the patch very much: it compiles, it > boots, and my (fairly limited) environment still works, but that's by no > means exhaustive. So I may have screwed something up, but most of this was > really fairly straightforward. > > But more importantly, I think it all makes sense independently of anything > else. In particular, now that "devpts_get_ref(inode)" operation should > really be the *only* place we need to look up what devpts instance we're > associated with, and we do it exactly once, at ptmx_open() time. > > So I think this is a good step forward, while avoiding anything that could > be at all controversial. > > Comments about the patch?
Mostly I think it is six of one half dozen of the other, but given driver_data is used so few times and that those times are very clear whether we are accessing a master or a slave worth doing just for clarity. I have work inspired by this rolled into my code. I will post shortly after a little more testing. Eric
