Messages in this thread | | | Date | Sat, 14 Aug 2004 13:41:37 -0700 | From | Chris Wright <> | Subject | Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update) |
| |
* viro@parcelfarce.linux.theplanet.co.uk (viro@parcelfarce.linux.theplanet.co.uk) wrote: > 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*.
Thanks for peeking at it.
> First of all, it ties you to ->i_ino values. Which is OK on a specific > fs, but not in a generic helper functions.
Good point.
> 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.
Yes, I took too simplistic a view on moving the common code over to generic.
> 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...
Agreed. Thanks for the feedback. -chris - 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/
| |