Messages in this thread |  | | Date | Tue, 22 May 2001 15:37:54 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH] struct char_device |
| |
On Tue, 22 May 2001 Andries.Brouwer@cwi.nl wrote: > > The operations are different, but all bdev/cdev code is identical. > > So the choice is between two uglies: > (i) have some not entirely trivial amount of code twice in the kernel > (ii) have a union at the point where the struct operations > is assigned. > > I preferred the union.
I would much prefer a union of pointers over a pointer to a union.
So I'd much rather have the inode have a
union { struct block_device *block; struct char_device *char; } dev;
and then have people do
cdev = inode->dev.char;
to get the right information, than to have
union block_char_union { struct block_device block; struct char_device char; };
.. with struct inode containing .. union block_char_union *dev;
Why? Because if you have a "struct inode", you also have enough information to decide _which_ of the two types of pointers you have, so you can do the proper dis-ambiguation of the union and properly select either 'inode->dev.char' or 'inode->dev.block' depending on other information in the inode.
In contrast, if you have a pointer to a union, you don't have information of which sub-type it is, and you'd have to carry that along some other way (for example, by having common fields at the beginning). Which I think is broken.
So my suggestion for eventual interfaces:
- have functions like
struct block_dev *bdget(struct inode *); struct char_dev *cdget(struct inode *);
which populate the "inode->dev" union pointer, which in turn is _only_ a cache of the lookup. Right now we do this purely based on "dev_t", and I think that is bogus. We should never pass a "dev_t" around without an inode, I think.
And we should not depend on the "inode->dev.xxxx" pointer being valid all the time, as there is absolutely zero point in initializing the pointer every time the inode is read just because somebody does a "ls -l /dev". Thus the "cache" part above.
- NO reason to try to make "struct block_dev" and "struct char_dev" look similar. They will have some commonality for lookup purposes (that issue is similar, as Andries points out), and maybe that commonality can be separated out into a sub-structure or something. But apart from that, they have absolutely nothing to do with each other, and I'd rather not have them have even a _superficial_ connection.
Block devices will have the "request queue" pointer, and the size and partitioning information. Character devices currently would not have much more than the operations pointer and name, but who knows..
But the most important thing is to be able to do this in steps. One of the reasons Andries has had patches for a long time is that it was never very gradual. Al's patch is gradual, and I like that.
Linus
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |