Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig <hch@lst.de>]

On Fri, May 30, 2008 at 04:37:42PM +1000, Timothy Shimmin wrote:
> > Yes.  I have an initial patch that goes directly to xfs_attr_list_int
> > from xfs_xattr.c and kills most of the ATTR_KERN flags.  It's a quite
> > nice cleanup already.  Next step will be to convert dmapi to use it's
> > own callback aswell.  This will be an even bigger cleanup as
> > put_listent gets the xattr value aswell and we can kill the additional
> > xfs_attr_get calls, making this code simpler and more efficient.
> > 
> Sorry, what xfs_attr_get call are you referring to?

xfs_dm_getall_dmattr currently does an xfs_attr_list and then performs
and xfs_attr_get for each attribute it cares about.  But rewriting this
to use put_listent we not only get rid of the temporary buffer but
also the need to calls xfs_attr_get because put_listent already gets
the attribute value passed.  It also fixes the race mentioned in the
comment and with a properly written put_listent helper can copy the
attributes directly to userspace without any kernel buffer at all.

That would also fix the direct userspace pointer dereference currently
lingering in that code :)

> > I've started looking at this and after some investigation I think
> > we should just pass the xfs_inode directly to all the functions and then
> > a void parameter, yes.  We'll need to find a solution for the
> > seen_enough paramter, but I think this could be handled similar to
> > filldir.  There's also some functions directly touching the attr cursor
> > which seems solveable, too.
> > 
> I'll await the patch :)
> The seen_enough param was added for search type callbacks so the callback
> could terminate the list walk early.
> Oh okay, I also used it to stop when we fill the buffer.

Yes. May idea is to break out of the loop as soon as put_listent returns
a non-zero value just like filldir.  We can then decided to play with
positivie / begative values to allow for a sucessfull early exit, or
just like filldir have the actual errno inside the private context
structure.  I suspect the first variant will be cleaner.