On Mon, Feb 24, 2020 at 9:37 AM Allison Collins <allison.henderson@xxxxxxxxxx> wrote: > > > > 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. > Understood. I have no smart suggestion, so withdrawing my comment. Thanks, Amir.
