On Tue, Apr 09, 2024 at 10:26:07PM -0700, Christoph Hellwig wrote:
> On Tue, Apr 09, 2024 at 05:56:22PM -0700, Darrick J. Wong wrote:
> > From: Allison Henderson <allison.henderson@xxxxxxxxxx>
> >
> > 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
I combined the three patches into this:
xfs: create attr log item opcodes and formats for parent pointers
Make the necessary alterations to the extended attribute log intent item
ondisk format so that we can log parent pointer operations. This
requires the creation of new opcodes specific to parent pointers, and a
new four-argument replace operation to handle renames. At this point
this part of the patchset has changed so much from what Allison original
wrote that I no longer think her SoB applies.
Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
and about time, I was getting real irritated at having to iterate back
and forth across those. ;)
> > + 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?
static inline bool
xfs_attri_validate_namelen(unsigned int namelen)
{
return namelen > 0 && namelen <= XATTR_NAME_MAX;
}
Done.
> > + 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?
Done.
> > + if (xfs_attr_log_item_op(old_attrp) == XFS_ATTRI_OP_FLAGS_PPTR_REPLACE) {
>
> Please avoid the overly long line here.
I've turned that into a switch()
> >
> > + /* 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?
Ok, I'll add that as a prep patch.
--D
