On 2/23/20 11:50 PM, Amir Goldstein wrote:
On Sun, Feb 23, 2020 at 6:51 PM Allison Collins
<allison.henderson@xxxxxxxxxx> wrote:
On 2/23/20 4:54 AM, Amir Goldstein wrote:
On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
<allison.henderson@xxxxxxxxxx> 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 | 37 +++++++-------
fs/xfs/libxfs/xfs_attr_leaf.c | 104 +++++++++++++++++++++-------------------
fs/xfs/libxfs/xfs_attr_remote.c | 2 +-
fs/xfs/libxfs/xfs_da_btree.c | 6 ++-
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, 130 insertions(+), 123 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 6717f47..9acdb23 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)
+ memcpy(&args->name, name, sizeof(struct xfs_name));
Maybe xfs_name_copy and xfs_name_equal are in order?
You are suggesting to add xfs_name_copy and xfs_name_equal helpers? I'm
not sure there's a use case yet for xfs_name_equal, at least not in this
set. And I think people indicated that they preferred the memcpy in
past reviews rather than handling each member of the xfs_name struct.
Unless I misunderstood the question, I'm not sure there is much left for
a xfs_name_copy helper to cover that the memcpy does not?
It's fine. The choice between xfs_name_copy and memcpy is
mostly personal taste. I did not read through past reviews.
+ /* Use name now stored in args */
+ name = &args.name;
+
It seem that the context of these comments be clear in the future.
You are asking to add more context to the comment? How about:
/*
* Use name now stored in args. Abandon the local name
* parameter as it will not be updated in the subroutines
*/
Does that help some?
Can't you just not use local name arg anymore?
Instead of re-assigning it and explaining why you do that.
Does that gain anything to code readability or anything else?
Well, with out the set, you cannot use for example name->type anymore,
you need to use args->name->type. In order to use the local name
variable again, it needs to be updated. I stumbled across this myself
when working with it and thought it would be better to fix it right away
rather than let others run across the same mistake. It seems like a
subtle and easy thing to overlook otherwise.
I do think it's a bit of a wart that people may not have thought about
when we talked about adding this patch. Though I don't know that it's a
big enough deal to drop it either. But I did feel some motivation to at
least clean it up and make a comment about it.
Allison
Thanks,
Amir.
