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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> [djwong: adjust to new ondisk format, split ioctls]
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
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@xxxxxx>
