Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin <tes@sgi.com>]

Christoph Hellwig wrote:
> On Fri, May 23, 2008 at 03:22:14PM +1000, Timothy Shimmin wrote:
>> Hi Christoph,
>>
>> Looks reasonable to me.
>>
>> In list_one_attr(), which looks based on attr_generic_listadd(),
>> it does a final:
>>> +	p += len;
>> which seems useless.
> 
> Yeah, feel free to remove it when you commit the patch.  Alternatively
> I'll send an incremental patch once commited.
> 
Yep, I'll remove it before checkin.

>> An aside, I noticed in passing (which was in existing code),
>> how we call vn_revalidate
>> in xfs_xattr_system_set(). I presume this is because we
>> call xfs_acl_setmode() in xfs_acl_vset() when we want to sync
>> the mode bits to the ACL.
>> If this is the case, then I think it would have been clearer
>> to put vn_revalidate() in the vicinity of xfs_acl_setmode().
>> Or is there some other reason?
> 
> No real reason, I was just keeping what was there before.  But getting
> rid of vn_revalidate complete is on my todo list.  The timestamp updates
> in there are already not nessecary anymore, and the i_mode/i_uid/i_gid
> and i_flags updates can be done much better in the caller that change
> the values in the dinode.
> 
Okay, good to know.


>> So you are removing xfs_vn_setxattr, xfs_vn_getxattr, and
>> xfs_vn_removexattr and calling generic_xattr and retaining the
>> code in xattr_handler's.
>>
>> You leave xfs_vn_listxattr alone. Just like ext3 does its own. Ok.
> 
> Yes, the generic listxattr doesn't buy us anything as we just traverse
> the attr btree and list all of them in their natural order.  No point
> in traversing it N times for N different attribute handlers.
> 
Good point (I didn't really look).


>>> +static int
>>> +xfs_xattr_system_get(struct inode *inode, const char *name,
>>> +		void *buffer, size_t size)
>>> +{
>>> +	int acl;
>>> +
>>> +	acl = xfs_decode_acl(name);
>>> +	if (acl < 0)
>>> +		return acl;
>>> +
>>> +	return xfs_acl_vget(inode, buffer, size, acl);
>>> +}
>>> +
>> Fine.
>> Seems a little funny as we are calling it a system EA but
>> then directly calling the acl code.
>> i.e. acknowledging, I guess, that we only have Posix ACLs
>> as system EAs.
>> Almost could call it xfs_xattr_acl_get().
>> It saves on one liner wrappers I guess.
> 
> Probably worth adding a comment that we only have acls for now.
> The reason I did this is to avoid doing multiple memcpys on the xattr
> name for decoding it.  It will probably need some revisiting if/when
> we support other system xattrs.
Okay, I'll add a comment.


> 
> Yeah, se the comment above.  vn_revalidate will go away pretty soon at
> which point this is sorted out.
> 
>>> +struct xattr_handler *xfs_xattr_handlers[] = {
>>> +	&xfs_xattr_user_handler,
>>> +	&xfs_xattr_trusted_handler,
>>> +	&xfs_xattr_security_handler,
>>> +	&xfs_xattr_system_handler,
>>> +	NULL
>>> +};
>>> +
>> So List code is done separately. Hmmmm...
>>
>> Oh, okay, ATTR_KERNAMELS (for attr_vn_listxattr) uses
>> concatenated string arrays
>> whereas for not ATTR_KERNAMELS, such as is used in
>> xfs_attrlist_by_handle is uses list data in the form:
>> <u32: valuelen> <char: name-array> <pad-to-4-byte-boundary>
> 
> Yes, this is quite unfortunate.  My plan is to refactor the low-level
> attr code so that is passes down a formatter ala filldir for directory
> reading.  This should clean up the attr listing code a lot and also
> gets rid of the levtover struct attrnames bits.  But that a different
> patch which still needs to be written.
> 
I guess this is done to some extent by the put_listent() callback.
Though, the context structure is probably a bit overused in different
ways.
The callback was also used for searching (for parent-ptr code) as well
as for list formatting.
I presume we are preserving the valuelen list format to keep the API
for xfs_attrlist_by_handle used by xfsdump and probably for dmf.

Thanks,
--Tim