On Wed, Nov 06, 2019 at 06:27:49PM -0700, Allison Collins 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 appearence 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>
---
This mostly looks good to me. Just some small nits..
fs/xfs/libxfs/xfs_attr.c | 154 +++++++++++++++++++++++++++---------------
fs/xfs/libxfs/xfs_attr.h | 1 +
fs/xfs/libxfs/xfs_attr_leaf.c | 107 ++++++++++++++++++-----------
fs/xfs/libxfs/xfs_attr_leaf.h | 2 +
4 files changed, 171 insertions(+), 93 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 5cb83a8..c8a3273 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
...
@@ -310,6 +313,34 @@ xfs_attr_set_args(
}
/*
+ * Return EEXIST if attr is found, or ENOATTR if not
+ */
+int
+xfs_has_attr(
+ struct xfs_da_args *args)
+{
+ struct xfs_inode *dp = args->dp;
+ struct xfs_buf *bp;
+ int error;
+
+ if (!xfs_inode_hasattr(dp)) {
+ error = -ENOATTR;
+ } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+ ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
+ error = xfs_attr_shortform_hasname(args, NULL, NULL);
+ } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+ error = xfs_attr_leaf_hasname(args, &bp);
+ if (error != -ENOATTR && error != -EEXIST)
+ goto out;
Hmm.. is this basically an indirect check for whether bp is set? If so,
I think doing bp = NULL above and:
if (bp)
xfs_trans_brelse(args->trans, bp);
... is more straightforward.
+ xfs_trans_brelse(args->trans, bp);
+ } else {
+ error = xfs_attr_node_hasname(args, NULL);
+ }
+out:
+ return error;
+}
+
+/*
* Remove the attribute specified in @args.
*/
int
...
@@ -832,6 +869,38 @@ xfs_attr_leaf_get(xfs_da_args_t *args)
return error;
}
+/*
+ * Return EEXIST if attr is found, or ENOATTR if not
+ * statep: If not null is set to point at the found state. Caller will
+ * be responsible for freeing the state in this case.
+ */
+STATIC int
+xfs_attr_node_hasname(
+ struct xfs_da_args *args,
+ struct xfs_da_state **statep)
+{
+ struct xfs_da_state *state;
+ int retval, error;
+
+ state = xfs_da_state_alloc();
+ state->args = args;
+ state->mp = args->dp->i_mount;
+
+ /*
+ * Search to see if name exists, and get back a pointer to it.
+ */
+ error = xfs_da3_node_lookup_int(state, &retval);
+ if (error == 0)
+ error = retval;
+
+ if (statep != NULL)
+ *statep = state;
+ else
+ xfs_da_state_free(state);
+
+ return error;
+}
The state allocation handling is a little wonky here in the error
scenario. I think precedent is that if we're returning an unexpected
error, we should probably just free state directly rather than rely on
the caller to do so. If the function returns "success" (meaning -EEXIST
or -ENOATTR), then the caller owns the state memory. It might also make
sense to NULL init the pointer either at the top of this helper or the
caller.
+
/*========================================================================
* External routines when attribute list size > geo->blksize
*========================================================================*/
...
@@ -1324,20 +1376,14 @@ xfs_attr_node_get(xfs_da_args_t *args)
trace_xfs_attr_node_get(args);
- state = xfs_da_state_alloc();
- state->args = args;
- state->mp = args->dp->i_mount;
-
/*
* Search to see if name exists, and get back a pointer to it.
*/
- error = xfs_da3_node_lookup_int(state, &retval);
- if (error) {
+ error = xfs_attr_node_hasname(args, &state);
+ if (error != -EEXIST) {
retval = error;
Can we kill retval in this function now? The only use is to assign error
to it.
goto out_release;
}
- if (retval != -EEXIST)
- goto out_release;
/*
* Get the value, local or "remote"
...
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 93c3496..d06cfd6 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -655,18 +655,67 @@ xfs_attr_shortform_create(xfs_da_args_t *args)
}
/*
+ * Return -EEXIST if attr is found, or -ENOATTR if not
+ * args: args containing attribute name and namelen
+ * sfep: If not null, pointer will be set to the last attr entry found on
+ -EEXIST. On -ENOATTR pointer is left at the last entry in the list
+ * basep: If not null, pointer is set to the byte offset of the entry in the
+ * list on -EEXIST. On -ENOATTR, pointer is left at the byte offset of
+ * the last entry in the list
+ */
+int
+xfs_attr_shortform_hasname(
+ struct xfs_da_args *args,
+ struct xfs_attr_sf_entry **sfep,
+ int *basep)
+{
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ int base = sizeof(struct xfs_attr_sf_hdr);
+ int size = 0;
+ int end;
+ int i;
+
+ base = sizeof(struct xfs_attr_sf_hdr);
Double init.
+ sf = (struct xfs_attr_shortform *)args->dp->i_afp->if_u1.if_data;
+ sfe = &sf->list[0];
+ end = sf->hdr.count;
+ for (i = 0; i < end; sfe = XFS_ATTR_SF_NEXTENTRY(sfe),
+ base += size, i++) {
+ size = XFS_ATTR_SF_ENTSIZE(sfe);
+ if (sfe->namelen != args->name.len)
+ continue;
+ if (memcmp(sfe->nameval, args->name.name, args->name.len) != 0)
+ continue;
+ if (!xfs_attr_namesp_match(args->name.type, sfe->flags))
+ continue;
+ break;
+ }
+
+ if (sfep != NULL)
+ *sfep = sfe;
+
+ if (basep != NULL)
+ *basep = base;
+
+ if (i == end)
+ return -ENOATTR;
+ return -EEXIST;
+}
+
+/*
* Add a name/value pair to the shortform attribute list.
* Overflow from the inode has already been checked for.
*/
void
xfs_attr_shortform_add(xfs_da_args_t *args, int forkoff)
{
- xfs_attr_shortform_t *sf;
- xfs_attr_sf_entry_t *sfe;
- int i, offset, size;
- xfs_mount_t *mp;
- xfs_inode_t *dp;
- struct xfs_ifork *ifp;
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ int offset, size, error;
+ struct xfs_mount *mp;
+ struct xfs_inode *dp;
+ struct xfs_ifork *ifp;
Might as well fix up the typedef in the function signature (here and
below) as well.
Brian
trace_xfs_attr_sf_add(args);
@@ -677,18 +726,8 @@ xfs_attr_shortform_add(xfs_da_args_t *args, int forkoff)
ifp = dp->i_afp;
ASSERT(ifp->if_flags & XFS_IFINLINE);
sf = (xfs_attr_shortform_t *)ifp->if_u1.if_data;
- sfe = &sf->list[0];
- for (i = 0; i < sf->hdr.count; sfe = XFS_ATTR_SF_NEXTENTRY(sfe), i++) {
-#ifdef DEBUG
- if (sfe->namelen != args->name.len)
- continue;
- if (memcmp(args->name.name, sfe->nameval, args->name.len) != 0)
- continue;
- if (!xfs_attr_namesp_match(args->name.type, sfe->flags))
- continue;
- ASSERT(0);
-#endif
- }
+ error = xfs_attr_shortform_hasname(args, &sfe, NULL);
+ ASSERT(error != -EEXIST);
offset = (char *)sfe - (char *)sf;
size = XFS_ATTR_SF_ENTSIZE_BYNAME(args->name.len, args->valuelen);
@@ -733,33 +772,23 @@ xfs_attr_fork_remove(
int
xfs_attr_shortform_remove(xfs_da_args_t *args)
{
- xfs_attr_shortform_t *sf;
- xfs_attr_sf_entry_t *sfe;
- int base, size=0, end, totsize, i;
- xfs_mount_t *mp;
- xfs_inode_t *dp;
+ struct xfs_attr_shortform *sf;
+ struct xfs_attr_sf_entry *sfe;
+ int base, size = 0, end, totsize;
+ struct xfs_mount *mp;
+ struct xfs_inode *dp;
+ int error;
trace_xfs_attr_sf_remove(args);
dp = args->dp;
mp = dp->i_mount;
- base = sizeof(xfs_attr_sf_hdr_t);
sf = (xfs_attr_shortform_t *)dp->i_afp->if_u1.if_data;
- sfe = &sf->list[0];
- end = sf->hdr.count;
- for (i = 0; i < end; sfe = XFS_ATTR_SF_NEXTENTRY(sfe),
- base += size, i++) {
- size = XFS_ATTR_SF_ENTSIZE(sfe);
- if (sfe->namelen != args->name.len)
- continue;
- if (memcmp(sfe->nameval, args->name.name, args->name.len) != 0)
- continue;
- if (!xfs_attr_namesp_match(args->name.type, sfe->flags))
- continue;
- break;
- }
- if (i == end)
- return -ENOATTR;
+
+ error = xfs_attr_shortform_hasname(args, &sfe, &base);
+ if (error != -EEXIST)
+ return error;
+ size = XFS_ATTR_SF_ENTSIZE(sfe);
/*
* Fix up the attribute fork data, covering the hole
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
index 017480e..e108b37 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.h
+++ b/fs/xfs/libxfs/xfs_attr_leaf.h
@@ -42,6 +42,8 @@ int xfs_attr_shortform_getvalue(struct xfs_da_args *args);
int xfs_attr_shortform_to_leaf(struct xfs_da_args *args,
struct xfs_buf **leaf_bp);
int xfs_attr_shortform_remove(struct xfs_da_args *args);
+int xfs_attr_shortform_hasname(struct xfs_da_args *args,
+ struct xfs_attr_sf_entry **sfep, int *basep);
int xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode *dp);
int xfs_attr_shortform_bytesfit(struct xfs_inode *dp, int bytes);
xfs_failaddr_t xfs_attr_shortform_verify(struct xfs_inode *ip);
--
2.7.4
