On Wed, Aug 28, 2019 at 02:23:50PM +1000, Dave Chinner wrote:
> + * If ATTR_ALLOC is set in @flags, allocate the buffer for the value after
> + * existence of the attribute has been determined. On success, return that
> + * buffer to the caller and leave them to free it. On failure, free any
> + * allocated buffer and ensure the buffer pointer returned to the caller is
> + * null.
Given that all three callers pass ATTR_ALLOC, do we even need a flag
and keep the old behavior around, at least at the xfs_attr_get level?
For the lower level we still have scrub, but that fills out the args
structure directly.
> +static int
> +xfs_attr_copy_value(
> + struct xfs_da_args *args,
> + unsigned char *value,
> + int valuelen)
> +{
> + /*
> + * No copy if all we have to do is get the length
> + */
> + if (args->flags & ATTR_KERNOVAL) {
> + args->valuelen = valuelen;
> + return 0;
> + }
> +
> + /*
> + * No copy if the length of the existing buffer is too small
> + */
> + if (args->valuelen < valuelen) {
> + args->valuelen = valuelen;
> + return -ERANGE;
> + }
> +
> + if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
> + args->value = kmem_alloc_large(valuelen, KM_SLEEP);
> + if (!args->value)
> + return -ENOMEM;
> + }
> + args->valuelen = valuelen;
Can't we just move the setting of ->valuelen up to common code shared
between all the branches? That means it would also be set on an
allocation error, but that should be harmless.
> + /* remote block xattr requires IO for copy-in */
> + if (args->rmtblkno)
> + return xfs_attr_rmtval_get(args);
> +
> + /*
> + * This is to prevent a GCC warning because the remote xattr case
> + * doesn't have a value to pass in. In that case, we never reach here,
> + * but GCC can't work that out and so throws a "passing NULL to
> + * memcpy" warning.
> + */
> + if (!value)
> + return -EINVAL;
> + memcpy(args->value, value, valuelen);
> + return 0;
> +}
Can you split creating this helper into a separate prep patch? While
not strictly required it would make reviewing what is consolidation
vs what is new code for the on-demand buffer allocation a little easier.
> + error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
> + ATTR_ALLOC|ATTR_ROOT);
Please keep space between the symbols and | on each side.
