On Thu, Aug 29, 2019 at 12:55:59AM -0700, Christoph Hellwig wrote:
> 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
Only one caller passes ATTR_ALLOC - the ACL code. The other two
have their own buffers that are supplied....
> 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.
Can't overwrite args->valuelen until we've done the ERANGE check.
Sure, I could put it in a local variable, but that doesn't reduce
the amount of code, or make it obvious that we intentionally return
the attribute size when the supplied buffer it too small...
> > + /* 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.
ok.
> > + 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.
Fixed.
--
Dave Chinner
david@xxxxxxxxxxxxx
