Messages in this thread | | | Date | Sat, 14 Aug 2004 21:23:43 +0100 | From | viro@parcelfa ... | Subject | Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update) |
| |
On Sat, Aug 14, 2004 at 12:55:01PM -0700, Chris Wright wrote: > Looks nice. I didn't realize you were working on this consolidation too. > I cooked up a similar patch. In this case the user loads its inode > specific write_ops on open, then just uses the generic helpers. I also > fully serialized all write/read transactions per inode. It's lightly > tested. If there's anything you like in there, feel free to use it.
This is *wrong*.
First of all, it ties you to ->i_ino values. Which is OK on a specific fs, but not in a generic helper functions.
What's more, there is no point in any extra structures here - you are getting a file-specific method anyway, so you make it ->write() (which is where behaviour differs) instead of ->open(). Which kills the need of callbacks.
As a general rule, it's better to provide several helpers and let the users of interface call them rather than trying to fit everything into callbacks, flags, etc.
Consider for instance a driver that wants one such request/reply file. With your scheme it will have to declare two functions - foo_write_op() and foo_open(), the latter being a boilerplate _and_ declare (for fsck knows what reason) a single-element array so that foo_open() could pass array - file->f_dentry->d_inode->i_ino, only to compensate for use of ->i_ino in your helper.
Lots of glue for no good reason _and_ a new function type to deal with. As opposed to one function (foo_write()) that is a normal instance of ->write() and is actually smaller than your foo_write_op() + foo_open(). No arrays, no magic, no boilerplate code... - 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/
| |