On Sun, Feb 23, 2020 at 7:28 PM Allison Collins <allison.henderson@xxxxxxxxxx> wrote: > > > > On 2/23/20 5:20 AM, Amir Goldstein wrote: > > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins > > <allison.henderson@xxxxxxxxxx> wrote: > >> > >> From: Allison Henderson <allison.henderson@xxxxxxxxxx> > >> > >> This patch adds a new functions to check for the existence of an attribute. > >> Subroutines are also added to handle the cases of leaf blocks, nodes or shortform. > >> Common code that appears in existing attr add and remove functions have been > >> factored out to help reduce the appearance of duplicated code. We will need these > >> routines later for delayed attributes since delayed operations cannot return error > >> codes. > >> > >> Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx> > >> --- > >> fs/xfs/libxfs/xfs_attr.c | 171 ++++++++++++++++++++++++++++-------------- > >> fs/xfs/libxfs/xfs_attr.h | 1 + > >> fs/xfs/libxfs/xfs_attr_leaf.c | 111 +++++++++++++++++---------- > >> fs/xfs/libxfs/xfs_attr_leaf.h | 3 + > >> 4 files changed, 188 insertions(+), 98 deletions(-) > >> > >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > >> index 9acdb23..2255060 100644 > >> --- a/fs/xfs/libxfs/xfs_attr.c > >> +++ b/fs/xfs/libxfs/xfs_attr.c > >> @@ -46,6 +46,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args); > >> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args); > >> STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args); > >> STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args); > >> +STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp); > >> > >> /* > >> * Internal routines when attribute list is more than one block. > >> @@ -53,6 +54,8 @@ STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args); > >> STATIC int xfs_attr_node_get(xfs_da_args_t *args); > >> STATIC int xfs_attr_node_addname(xfs_da_args_t *args); > >> STATIC int xfs_attr_node_removename(xfs_da_args_t *args); > >> +STATIC int xfs_attr_node_hasname(xfs_da_args_t *args, > >> + struct xfs_da_state **state); > >> STATIC int xfs_attr_fillstate(xfs_da_state_t *state); > >> STATIC int xfs_attr_refillstate(xfs_da_state_t *state); > >> > >> @@ -310,6 +313,37 @@ xfs_attr_set_args( > >> } > >> > >> /* > >> + * Return EEXIST if attr is found, or ENOATTR if not > > > > This is a very silly return value for a function named has_attr in my taste. > > I realize you inherited this interface from xfs_attr3_leaf_lookup_int(), but > > IMO this change looks like a very good opportunity to change that internal > > API: > > > > xfs_has_attr? > > > > 0: NO > > 1: YES (or stay with the syscall standard of -ENOATTR) > > <0: error > Darrick had mentioned something like that in the last revision, but I > think people wanted to keep everything a straight hoist at this phase. > At least as much as possible. Maybe I can add another patch that does > this at the end when we're doing clean ups. In my opinion, this change is hard enough to follow as is. Making the interface cleaner at this point is not going to make following the change any harder if anything I thing a clean interface that makes sense, will make things easier at this point. But whether you do it now or later, the interface was very bad IMO before and when attached to a helper named has_attr makes it much worse. Thanks, Amir.
