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@xxxxxxxxxx>
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
---
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
