Christoph Hellwig wrote:
> On Tue, May 27, 2008 at 08:50:48PM +1000, Timothy Shimmin wrote:
>>> - the callback is supplied by the xfs_attr_list caller, not set based
>>> on options
>> Oh, okay. For example, instead of setting the flags to ATTR_KERNOVAL
>> such as in xfs_vn_listxattr when size is 0, one could just set the callback
>> to xfs_attr_kern_list_sizes and pass it in etc...
>
> 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?
>>> - there will be an opaque object supplied to xfs_attr_list that is to
>>> be used by put_listent so that we don't have to pass down
>>> implementation-specific arguments directly.
>>>
>> Ok.
>> So instead of overloading fields in xfs_attr_list_context_t,
>> you'll pass down a void* argument or some such for callback specific data.
>
> 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.
>>> I'd also like to move the attrlist_cursor_kern_t into this callback
>>> opaque context because it doesn't make sense for the normal xattr API,
>>> but I'll have to see if that's actually feasible.
>> BTW, the cursor stuff is a bit flawed. Like the dir1 code (I believe),
>> if from userspace you use the cursor and modifications happen to the EAs
>> (add or removal) between calls,
>> we can end up repeating elements in the list or miss some.
>> We don't preserve the position and we can compact the data etc.
>
> Yes, I think this whole cursor is a rather bad idea. But given that
> it's used by xfsdump we can't easily get rid of it.
Not if we are to remain compatible with old xfsdump programs (assuming we
changed
the latest one).
Yep.
--Tim
|