Re: [PATCH 1/1] rpdfs: Initial extended attribute support

[Zach Brown <zab@zabbo.net>]

On Tue, Mar 03, 2026 at 09:20:52AM -0600, Chris Kirby wrote:
> Initial extended attribute support

Just a few cleanups..

> +#define RPDFS_XATTR_MAX_NAME_LEN	RPDFS_NAME_MAX

I suppose this should go in format-block.h by the other xattr constants.
(And like the dirent _NAME_MAX equivalent).

> +	if (copy_value) {
> +		char *dest;

I have a really old habit to not declare local variables in blocks from
old gcc bugs that used to blow out the stack.  (I think due to
larger alignments.. it's been so long!)  Anyway, if possible, declare
everything up top.

> +		dest = (char *)kx + offsetof(struct key_xattr,
> +				xattr.name[name_len]);
> +		memcpy(dest, value, size);

But in this case do we still need this single-use indirection after the
flex array fixes?  If we do, so be it, but if this is left over from
trying to avoid the array overflow warnings before the flex union fix
then it'd be nice to clean this (and the other instances) up.

> +static int add_xattr_item_cb(struct rpdfs_fs_info *rfi,
> +			     struct rpdfs_btree_item_args *a,
> +                             struct rpdfs_btree_item_args *b,
> +			     struct rpdfs_btree_item_args *ins,
> +			     void *arg)
> +{
> +	struct key_xattr *kx = arg;
> +
> +	if (a && xattr_key_hash(&kx->key) == xattr_key_hash(&a->key))
> +		return -EEXIST;

We all knew this, but to be explicit: these patterns don't make sense
for the xattrs that have a unique lsq in the key.  These come from the
dirents that only allow a single collision bit so that they can be done
in one call to a handful of items in one block.  When we have a full
64bits of collision the potentially matching entries can span lots of
items and blocks in the btree.

But the btree is going away, and this makes it an easy to follow
derivation from the dirents, so sure, let's go with this for now.  It
won't matter before we replace the btree and all of this changes.

> +	do {
> +		have_old = false;
> +
> +		ret = rpdfs_inode_txn_prepare(rfi, &txn, inode, RBAF_WRITE);
> +		if (ret == 0) {
> +			ret = rpdfs_btree_txn_prepare_lookup(rfi, &txn,
> +							     &RPDFS_I(inode)->xattrs,
> +							     &old_kx->key,
> +							     lookup_xattr_cb,
> +							     old_kx);
> +
> +			/* lookup returns the xattr size if it finds the name */
> +			if (ret >= 0) {
> +				have_old = true;
> +				ret = 0;
> +			}
> +
> +			/*
> +			 * It's an error to specify XATTR_REPLACE if the name
> +			 * doesn't already exist.
> +			 */
> +			if (!have_old) {
> +				if (flags & XATTR_REPLACE)
> +					ret = -ENODATA;
> +				else
> +					ret = 0;
> +			}
> +
> +			/*
> +			 * It's an error to specify XATTR_CREATE if the name
> +			 * already exists.
> +			 */
> +			if (ret == 0 && have_old && (flags & XATTR_CREATE))
> +				ret = -EEXIST;
> +
> +			if (ret == 0 && have_old) {
> +				ret = prepare_delete_xattr(rfi, &txn, inode,
> +							   old_kx);
> +			}
> +		}
> +
> +		if (ret == 0 && value != NULL) {
> +			xattr_key_set_uniq(new_kx, ri->xattr_creates);
> +
> +			ret = prepare_add_xattr(rfi, &txn, inode, new_kx);
> +		}
> +	} while (rpdfs_txn_retry(rfi, &txn, &ret));

Oof, that's a lot to be in the retry block.  If it's more than a handful
of lines of obvious cascading error checking, can you pop it up in a
helper function?

> +	creates = __le64_to_cpu(ri->xattr_creates) + 1;
> +
> +	if (have_old)
> +		apply_delete_xattr(rfi, &txn, inode, old_kx);
> +
> +	if (value) {
> +		apply_add_xattr(rfi, &txn, inode, new_kx);
> +		ri->xattr_creates = cpu_to_le64(creates);

Drop the "creates" that might not have been used and increment directly
with le64_add_cpu(&ri->xattr_creates, 1);

- z