Re: [PATCH 11/32] xfs: log parent pointer xattr replace operations

Christoph Hellwig
2024-04-10T05:26:07Z

On Tue, Apr 09, 2024 at 05:56:22PM -0700, Darrick J. Wong wrote:
> From: Allison Henderson <allison.henderson@oracle.com>
>
> The parent pointer code needs to do a deferred parent pointer replace
> operation with the xattr log intent code. Declare a new logged xattr
> opcode and push it through the log.
>
> (Formerly titled "xfs: Add new name to attri/d" and described as
> follows:

I don't think this history is very important. The being said,
I suspect this and the previous two patches should be combined into
a single one adding the on-disk formats for parent pointers, and the
commit log could use a complete rewrite saying that it a

> + return false;
> + if (attrp->alfi_old_name_len == 0 ||
> + attrp->alfi_old_name_len > XATTR_NAME_MAX)
> + return false;
> + if (attrp->alfi_new_name_len == 0 ||
> + attrp->alfi_new_name_len > XATTR_NAME_MAX)
> + return false;

Given that we have four copies of this (arguably simple) check,
should we grow a helper for it?

> + if (attrp->alfi_value_len == 0 ||
> + attrp->alfi_value_len > XATTR_SIZE_MAX)
> + return false;

All parent pointer attrs must be sized for exactly the parent_rec,
so we should probably check for that explicitly?

> + if (xfs_attr_log_item_op(old_attrp) == XFS_ATTRI_OP_FLAGS_PPTR_REPLACE) {

Please avoid the overly long line here.

>
> + /* Validate the new attr name */
> + if (new_name_len > 0) {
> + if (item->ri_buf[i].i_len != xlog_calc_iovec_len(new_name_len)) {

.. and here.

And while we're at it, maybe factor the checking for valid xattr
name and value log iovecs into little helper instead of duplicating
them a few times?