| Message ID | 171270969674.3631889.16669894985199358307.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/32] xfs: rearrange xfs_attr_match parameters | expand |
On Tue, Apr 09, 2024 at 05:55:20PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Add a new XFS_DA_OP_PARENT flag to signal that the caller wants to look The flag doesn't actually exist, the match is done on the XFS_ATTR_PARENT namespaces. > > @@ -2444,14 +2477,17 @@ xfs_attr3_leaf_lookup_int( > name_loc = xfs_attr3_leaf_name_local(leaf, probe); > if (!xfs_attr_match(args, entry->flags, > name_loc->nameval, > - name_loc->namelen)) > + name_loc->namelen, > + &name_loc->nameval[name_loc->namelen], > + be16_to_cpu(name_loc->valuelen))) If we'd switch from the odd pre-existing three-tab indent to the normal two-tab indent we'd avoid the overly long line here. > continue; > args->index = probe; > return -EEXIST; > } else { > name_rmt = xfs_attr3_leaf_name_remote(leaf, probe); > if (!xfs_attr_match(args, entry->flags, name_rmt->name, > - name_rmt->namelen)) > + name_rmt->namelen, NULL, > + be32_to_cpu(name_rmt->valuelen))) ... and here. The remote side might also benefit from a local variable to store the endian swapped version of the valuelen instead of calculating it twice. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Tue, Apr 09, 2024 at 10:16:58PM -0700, Christoph Hellwig wrote: > On Tue, Apr 09, 2024 at 05:55:20PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Add a new XFS_DA_OP_PARENT flag to signal that the caller wants to look > > The flag doesn't actually exist, the match is done on the > XFS_ATTR_PARENT namespaces. How about: "xfs: allow xattr matching on name and value for local/sf pptr attrs "If a file is hardlinked with the same name but from multiple parents, the parent pointers will all have the same dirent name (== attr name) but with different parent_ino/parent_gen values. To disambiguate, we need to be able to match on both the attr name and the attr value. This is in contrast to regular xattrs, which are matched only on name. "Therefore, plumb in the ability to match shortform and local attrs on name and value in the XFS_ATTR_PARENT namespace. Parent pointer attr values are never large enough to be stored in a remote attr, so we need can reject these cases as corruption." > > > > @@ -2444,14 +2477,17 @@ xfs_attr3_leaf_lookup_int( > > name_loc = xfs_attr3_leaf_name_local(leaf, probe); > > if (!xfs_attr_match(args, entry->flags, > > name_loc->nameval, > > - name_loc->namelen)) > > + name_loc->namelen, > > + &name_loc->nameval[name_loc->namelen], > > + be16_to_cpu(name_loc->valuelen))) > > If we'd switch from the odd pre-existing three-tab indent to the normal > two-tab indent we'd avoid the overly long line here. > > > continue; > > args->index = probe; > > return -EEXIST; > > } else { > > name_rmt = xfs_attr3_leaf_name_remote(leaf, probe); > > if (!xfs_attr_match(args, entry->flags, name_rmt->name, > > - name_rmt->namelen)) > > + name_rmt->namelen, NULL, > > + be32_to_cpu(name_rmt->valuelen))) > > ... and here. Believe it or not that's what vim autoformat does by default. But yes, I'll reduce it to two indents to reduce the indentation and overflow. > The remote side might also benefit from a local variable to store the > endian swapped version of the valuelen instead of calculating it twice. Ok. > Otherwise looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks! --D
On Wed, Apr 10, 2024 at 02:13:10PM -0700, Darrick J. Wong wrote: > How about: > > "xfs: allow xattr matching on name and value for local/sf pptr attrs How about: "match on the attr value as well for parent pointers" for the subject? The commit message body looks good to me.
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 490608bbed7ad..7d74ade47d8f1 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -513,12 +513,33 @@ static inline unsigned int xfs_attr_match_mask(const struct xfs_da_args *args) return XFS_ATTR_NSP_ONDISK_MASK | XFS_ATTR_INCOMPLETE; } +static inline bool +xfs_attr_parent_match( + const struct xfs_da_args *args, + const void *value, + unsigned int valuelen) +{ + ASSERT(args->value != NULL); + + /* Parent pointers do not use remote values */ + if (!value) + return false; + + /* The only value we support is a parent rec. */ + if (valuelen != sizeof(struct xfs_parent_rec)) + return false; + + return memcmp(args->value, value, valuelen) == 0; +} + static bool xfs_attr_match( struct xfs_da_args *args, unsigned int attr_flags, const unsigned char *name, - unsigned int namelen) + unsigned int namelen, + const void *value, + unsigned int valuelen) { unsigned int mask = xfs_attr_match_mask(args); @@ -529,6 +550,9 @@ xfs_attr_match( if (memcmp(args->name, name, namelen) != 0) return false; + if (attr_flags & XFS_ATTR_PARENT) + return xfs_attr_parent_match(args, value, valuelen); + return true; } @@ -538,6 +562,13 @@ xfs_attr_copy_value( unsigned char *value, int valuelen) { + /* + * Parent pointer lookups require the caller to specify the name and + * value, so don't copy anything. + */ + if (args->attr_filter & XFS_ATTR_PARENT) + return 0; + /* * No copy if all we have to do is get the length */ @@ -747,7 +778,9 @@ xfs_attr_sf_findname( sfe < xfs_attr_sf_endptr(sf); sfe = xfs_attr_sf_nextentry(sfe)) { if (xfs_attr_match(args, sfe->flags, sfe->nameval, - sfe->namelen)) + sfe->namelen, + &sfe->nameval[sfe->namelen], + sfe->valuelen)) return sfe; } @@ -2444,14 +2477,17 @@ xfs_attr3_leaf_lookup_int( name_loc = xfs_attr3_leaf_name_local(leaf, probe); if (!xfs_attr_match(args, entry->flags, name_loc->nameval, - name_loc->namelen)) + name_loc->namelen, + &name_loc->nameval[name_loc->namelen], + be16_to_cpu(name_loc->valuelen))) continue; args->index = probe; return -EEXIST; } else { name_rmt = xfs_attr3_leaf_name_remote(leaf, probe); if (!xfs_attr_match(args, entry->flags, name_rmt->name, - name_rmt->namelen)) + name_rmt->namelen, NULL, + be32_to_cpu(name_rmt->valuelen))) continue; args->index = probe; args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);