Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[viro]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/