Re: [PATCH v4 03/17] xfs: Embed struct xfs_name in xfs_da_args

[Allison Collins <allison.henderson@oracle.com>]

On 11/7/19 6:25 PM, Darrick J. Wong wrote:
> On Wed, Nov 06, 2019 at 06:27:47PM -0700, Allison Collins wrote:
>> This patch embeds an xfs_name in xfs_da_args, replacing the name,
>> namelen, and flags members.  This helps to clean up the xfs_da_args
>> structure and make it more uniform with the new xfs_name parameter
>> being passed around.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c        |  36 +++++++-------
>>   fs/xfs/libxfs/xfs_attr_leaf.c   | 106 +++++++++++++++++++++-------------------
>>   fs/xfs/libxfs/xfs_attr_remote.c |   2 +-
>>   fs/xfs/libxfs/xfs_da_btree.c    |   5 +-
>>   fs/xfs/libxfs/xfs_da_btree.h    |   4 +-
>>   fs/xfs/libxfs/xfs_dir2.c        |  18 +++----
>>   fs/xfs/libxfs/xfs_dir2_block.c  |   6 +--
>>   fs/xfs/libxfs/xfs_dir2_leaf.c   |   6 +--
>>   fs/xfs/libxfs/xfs_dir2_node.c   |   8 +--
>>   fs/xfs/libxfs/xfs_dir2_sf.c     |  30 ++++++------
>>   fs/xfs/scrub/attr.c             |  12 ++---
>>   fs/xfs/xfs_trace.h              |  20 ++++----
>>   12 files changed, 128 insertions(+), 125 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 5a9624a..b77b985 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -72,13 +72,12 @@ xfs_attr_args_init(
>>   	args->geo = dp->i_mount->m_attr_geo;
>>   	args->whichfork = XFS_ATTR_FORK;
>>   	args->dp = dp;
>> -	args->flags = flags;
>> -	args->name = name->name;
>> -	args->namelen = name->len;
>> -	if (args->namelen >= MAXNAMELEN)
>> +	name->type = flags;
> 
> Is there a purpose for modifying the caller's @name, instead of setting
> args.name.type = flags after the memcpy?
Not really other than to just make them consistent so that the mem copy 
would cover all three members.  I think initially they were set 
individually, but people preferred the memcpy, and then later we decided 
to break the flags variable out of the name struct.  I can explicitly 
set args.name.type if folks prefer.

A subtle pitfall I noticed about using the name struct like this now: 
callers need to take care to abandon the original name struct or update 
the original pointer to this struct.  It's kind of easy to continue 
using the original struct like it's a convenience pointer, but it not 
the same memory so if args gets passed into some subroutine that 
modifies it, now you're out of sync.  If it doesnt get passed around or 
modified, it doesnt matter, but we've created a sort of usage rule about 
how to handle the name param which may not be entirely obvious to other 
developers.

> 
>> +	memcpy(&args->name, name, sizeof(struct xfs_name));
>> +	if (args->name.len >= MAXNAMELEN)
>>   		return -EFAULT;		/* match IRIX behaviour */
>>   
>> -	args->hashval = xfs_da_hashname(args->name, args->namelen);
>> +	args->hashval = xfs_da_hashname(args->name.name, args->name.len);
>>   	return 0;
>>   }
>>   
>> @@ -236,7 +235,7 @@ xfs_attr_try_sf_addname(
>>   	 * Commit the shortform mods, and we're done.
>>   	 * NOTE: this is also the error path (EEXIST, etc).
>>   	 */
>> -	if (!error && (args->flags & ATTR_KERNOTIME) == 0)
>> +	if (!error && (args->name.type & ATTR_KERNOTIME) == 0)
>>   		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>>   
>>   	if (mp->m_flags & XFS_MOUNT_WSYNC)
>> @@ -357,6 +356,9 @@ xfs_attr_set(
>>   	if (error)
>>   		return error;
>>   
>> +	/* Use name now stored in args */
>> +	name = &args.name;
> 
> You could probably set this as part of the variable declaration, e.g.
> 
> struct xfs_name		*name = &args.name;
No, because name is a parameter here, so you would loose the contents at 
the start of the function if we did this.  And then args init would 
memcopy and empty struct onto itself.  And then confusion would 
ensue.... :-(

> 
>> +
>>   	args.value = value;
>>   	args.valuelen = valuelen;
>>   	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
>> @@ -372,7 +374,7 @@ xfs_attr_set(
>>   	 */
>>   	if (XFS_IFORK_Q(dp) == 0) {
>>   		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
>> -			XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen, valuelen);
>> +			XFS_ATTR_SF_ENTSIZE_BYNAME(args.name.len, valuelen);
> 
> If you're going to keep the convenience variable @name, then please use
> it throughout the function.
I think this is the only place it's used for this function at this time. 
  I use it later in patch 15 though.  I think I stumbled across the name 
sync bug then, and corrected it with the parameter pointer assignment 
above just because it seemed like the right place to fix it.

Do you want the function to uniformly use an explicit reference, or the 
param?  Seems like a bit of a wart unfortunately :-(

> 
>>   
>>   		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
>>   		if (error)
> 
> <snip>
> 
>> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
>> index 4fd1223..129ec09 100644
>> --- a/fs/xfs/libxfs/xfs_da_btree.c
>> +++ b/fs/xfs/libxfs/xfs_da_btree.c
>> @@ -2040,8 +2040,9 @@ xfs_da_compname(
>>   	const unsigned char *name,
>>   	int		len)
>>   {
>> -	return (args->namelen == len && memcmp(args->name, name, len) == 0) ?
>> -					XFS_CMP_EXACT : XFS_CMP_DIFFERENT;
> 
> Wow that's gross. :)
> 
> 	if (args->name.len == len && !memcmp(args->name.name, name, len))
> 		return XFS_CMP_EXACT;
> 
> 	return XFS_CMP_DIFFERENT;
> 
> Hmm, is that better?
Sure, will fix.

Allison

> 
> The rest looks reasonable...
> 
> --D
>