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? Thanks, Amir.
