Re: [PATCH 27/32] xfs: Add parent pointer ioctls

[Christoph Hellwig <hch@infradead.org>]

Maybe replace the subject with 'add parent pointer listing ioctls' ?

On Tue, Apr 09, 2024 at 06:00:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This patch adds a pair of new file ioctls to retrieve the parent pointer
> of a given inode.  They both return the same results, but one operates
> on the file descriptor passed to ioctl() whereas the other allows the
> caller to specify a file handle for which the caller wants results.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> [djwong: adjust to new ondisk format, split ioctls]
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Note that the first signoff should always be from the patch author.
as recorded in the From line.

> +	/* Size of the gp_buffer in bytes */
> +	__u32				gp_bufsize;
> +
> +	/* Must be set to zero */
> +	__u64				__pad;

We don't really need this as padding.  If you want to keep it for
extensibility (although I can't really think of anything to use it
for in the future) it should probably be renamed to gp_reserved;

> +static inline struct xfs_getparents_rec *
> +xfs_getparents_next_rec(struct xfs_getparents *gp,
> +			struct xfs_getparents_rec *gpr)
> +{
> +	char *next = ((char *)gpr + gpr->gpr_reclen);
> +	char *end = (char *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
> +
> +	if (next >= end)
> +		return NULL;
> +
> +	return (struct xfs_getparents_rec *)next;

We rely on void pointer arithmetics everywhere in the kernel and
xfsprogs, so maybe use that here and avoid the need for the cast
at the end?

> + */
> +int
> +xfs_parent_from_xattr(
> +	struct xfs_mount	*mp,
> +	unsigned int		attr_flags,
> +	const unsigned char	*name,
> +	unsigned int		namelen,
> +	const void		*value,
> +	unsigned int		valuelen,
> +	xfs_ino_t		*parent_ino,
> +	uint32_t		*parent_gen)
> +{
> +	const struct xfs_parent_rec	*rec = value;
> +
> +	if (!(attr_flags & XFS_ATTR_PARENT))
> +		return 0;

I wonder if this check should move to the callers.  That makes the
calling conventions a lot simpler, and I think it probably makes
the code a bit easier to follow as well.  But I'm not entirely sure
either and open for arguments.

> +static inline unsigned int
> +xfs_getparents_rec_sizeof(
> +	unsigned int		namelen)
> +{
> +	return round_up(sizeof(struct xfs_getparents_rec) + namelen + 1,
> +			sizeof(uint32_t));
> +}

As we marked the xfs_getparents_rec as __packed we shouldn't really
need the alignment here.  Or if we align, it should be to 8 bytes,
in which case we don't need to pack it.

> +	unsigned short			reclen = xfs_getparents_rec_sizeof(namelen);

Please avoid the overly long line here.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>