On Tue, Apr 09, 2024 at 11:04:08PM -0700, Christoph Hellwig wrote:
> 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.
Yeah. At this point the ioctl is so much different from Allison's
original version that it doesn't make much sense to keep her as the
patch author or sob person.
> > + /* 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;
Eh, I'll keep it, just in case. The getparents_by_handle aligns nicely
with a single cacheline. :P
> > +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?
Hopefully our downstream users also have compilers that allow void
pointer arithmetic. ;)
> > + */
> > +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.
Yeah, on further thought I don't like the 0/1 return value convention
and will change that to require callers to screen for ATTR_PARENT.
> > +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.
Let's align it to u64; everything else is.
> > + unsigned short reclen = xfs_getparents_rec_sizeof(namelen);
>
> Please avoid the overly long line here.
Fixed.
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Thanks!
--D
