On Tue, Feb 25, 2020 at 03:09:52PM -0800, Christoph Hellwig wrote:
> Instead of converting from one style of arguments to another in
> xfs_attr_set, pass the structure from higher up in the call chain.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr.c | 69 ++++++++++++++++++----------------------
> fs/xfs/libxfs/xfs_attr.h | 3 +-
> fs/xfs/xfs_acl.c | 33 ++++++++++---------
> fs/xfs/xfs_ioctl.c | 22 ++++++++-----
> fs/xfs/xfs_iops.c | 13 +++++---
> fs/xfs/xfs_xattr.c | 21 ++++++++----
> 6 files changed, 87 insertions(+), 74 deletions(-)
Looks fine.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
One question, not directly related to the patch:
> __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> {
> - struct xfs_inode *ip = XFS_I(inode);
> - unsigned char *ea_name;
> - struct xfs_acl *xfs_acl = NULL;
> - int len = 0;
> - int error;
> + struct xfs_inode *ip = XFS_I(inode);
> + struct xfs_da_args args = {
> + .dp = ip,
> + .flags = ATTR_ROOT,
> + };
> + int error;
>
> switch (type) {
> case ACL_TYPE_ACCESS:
> - ea_name = SGI_ACL_FILE;
> + args.name = SGI_ACL_FILE;
> break;
> case ACL_TYPE_DEFAULT:
> if (!S_ISDIR(inode->i_mode))
> return acl ? -EACCES : 0;
> - ea_name = SGI_ACL_DEFAULT;
> + args.name = SGI_ACL_DEFAULT;
> break;
> default:
> return -EINVAL;
> }
> + args.namelen = strlen(args.name);
>
> if (acl) {
> - len = XFS_ACL_MAX_SIZE(ip->i_mount);
> - xfs_acl = kmem_zalloc_large(len, 0);
> - if (!xfs_acl)
> + args.valuelen = XFS_ACL_MAX_SIZE(ip->i_mount);
> + args.value = kmem_zalloc_large(args.valuelen, 0);
> + if (!args.value)
> return -ENOMEM;
>
> - xfs_acl_to_disk(xfs_acl, acl);
> + xfs_acl_to_disk(args.value, acl);
>
> /* subtract away the unused acl entries */
> - len -= sizeof(struct xfs_acl_entry) *
> + args.valuelen -= sizeof(struct xfs_acl_entry) *
> (XFS_ACL_MAX_ENTRIES(ip->i_mount) - acl->a_count);
why do we allocate a maximally sized buffer for the attribute data
(64kB for v5 filesystems) when we already know the size of the
data we are about to format into it? Why isn't this just:
if (acl) {
args.valuelen = sizeof(struct xfs_acl_entry) * acl->a_count;
args.value = kmem_zalloc_large(args.valuelen, 0);
if (!args.value)
return -ENOMEM;
xfs_acl_to_disk(args.value, acl);
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
