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.