On Tue, Feb 18, 2025 at 10:04:56AM -0800, Darrick J. Wong wrote:
> On Tue, Feb 18, 2025 at 07:54:09AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 18, 2025 at 12:36:33AM -0800, Christoph Hellwig wrote:
> > > On Thu, Feb 06, 2025 at 03:03:32PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > Aheada of deprecating V4 support in the kernel, let's give people a way
> > > > to extract their files from a filesystem without needing to mount.
> > >
> > > So I've wanted a userspace file access for a while, but if we deprecate
> > > the v4 support in the kernel that will propagte to libxfs quickly,
> > > and this code won't help you with v4 file systems either. So I don't
> > > think the rationale here seems very good.
> >
> > We aren't removing V4 support from the kernel until September 2030 and
> > xfsprogs effectively builds with CONFIG_XFS_SUPPORT_V4=y. That should
> > be enough time, right?
> >
> > > > extern void bmapinflate_init(void);
> > > > +extern void rdump_init(void);
> > >
> > > No need for the extern.
> >
> > Ok.
> >
> > > > + /* XXX cannot copy fsxattrs */
> > >
> > > Should this be fixed first? Or document in a full sentence comment
> > > explaining why it can't should not be?
> >
> > /* XXX cannot copy fsxattrs until setfsxattrat() syscall merge */
> >
> > > > + [1] = {
> > > > + .tv_sec = inode_get_mtime_sec(VFS_I(ip)),
> > > > + .tv_nsec = inode_get_mtime_nsec(VFS_I(ip)),
> > > > + },
> > > > + };
> > > > + int ret;
> > > > +
> > > > + /* XXX cannot copy ctime or btime */
> > >
> > > Same for this and others.
> >
> > Is there a way to set ctime or btime? I don't know of any.
>
> Now that I'm past all the morning meetings and have had time to research
> things a little more deeply/wake up more: no, there's no way to set the
> inode change or birth times.
>
> > /* Cannot set ctime or btime */
>
> So I'll go with ^^ this comment.
>
> > > > + /* Format xattr name */
> > > > + if (attr_flags & XFS_ATTR_ROOT)
> > > > + nsp = XATTR_TRUSTED_PREFIX;
> > > > + else if (attr_flags & XFS_ATTR_SECURE)
> > > > + nsp = XATTR_SECURITY_PREFIX;
> > > > + else
> > > > + nsp = XATTR_USER_PREFIX;
> > >
> > > Add a self-cotained helper for this? I'm pretty sure we do this
> > > translation in a few places.
>
> Actually, xfsprogs doesn't:
>
> $ git grep XATTR_SECURITY_PREFIX
> db/rdump.c:293: nsp = XATTR_SECURITY_PREFIX;
> mkfs/proto.c:393: } else if (!strncmp(attrname, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> mkfs/proto.c:394: args.name = (unsigned char *)attrname + XATTR_SECURITY_PREFIX_LEN;
>
> The kernel gets a little closer in xfs_xattr.c:
>
> $ git grep XATTR_SECURITY_PREFIX include/ fs/*.c fs/xfs/
> fs/xattr.c:128: if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN) ||
> fs/xattr.c:228: int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
> fs/xattr.c:229: XATTR_SECURITY_PREFIX_LEN);
> fs/xattr.c:249: const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> fs/xattr.c:442: if (!strncmp(name, XATTR_SECURITY_PREFIX,
> fs/xattr.c:443: XATTR_SECURITY_PREFIX_LEN)) {
> fs/xattr.c:444: const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> fs/xfs/xfs_xattr.c:207: .prefix = XATTR_SECURITY_PREFIX,
> fs/xfs/xfs_xattr.c:304: prefix = XATTR_SECURITY_PREFIX;
> fs/xfs/xfs_xattr.c:305: prefix_len = XATTR_SECURITY_PREFIX_LEN;
>
> But xfs_xattr_put_listent has some custom logic in it:
>
> if (flags & XFS_ATTR_ROOT) {
> #ifdef CONFIG_XFS_POSIX_ACL
> if (namelen == SGI_ACL_FILE_SIZE &&
> strncmp(name, SGI_ACL_FILE,
> SGI_ACL_FILE_SIZE) == 0) {
> __xfs_xattr_put_listent(
> context, XATTR_SYSTEM_PREFIX,
> XATTR_SYSTEM_PREFIX_LEN,
> XATTR_POSIX_ACL_ACCESS,
> strlen(XATTR_POSIX_ACL_ACCESS));
> } else if (namelen == SGI_ACL_DEFAULT_SIZE &&
> strncmp(name, SGI_ACL_DEFAULT,
> SGI_ACL_DEFAULT_SIZE) == 0) {
> __xfs_xattr_put_listent(
> context, XATTR_SYSTEM_PREFIX,
> XATTR_SYSTEM_PREFIX_LEN,
> XATTR_POSIX_ACL_DEFAULT,
> strlen(XATTR_POSIX_ACL_DEFAULT));
> }
> #endif
>
> /*
> * Only show root namespace entries if we are actually allowed to
> * see them.
> */
> if (!capable(CAP_SYS_ADMIN))
> return;
>
> prefix = XATTR_TRUSTED_PREFIX;
> prefix_len = XATTR_TRUSTED_PREFIX_LEN;
>
> But I do see that rdump should translate SGI_ACL_FILE to
> XATTR_POSIX_ACL_ACCESS so I'll go do that.
...actually, no. The SGI_ACL_{FILE,DEFAULT} xattrs can be copied
verbatim into another XFS filesystem and they continue to work
correctly. Translating them into the generic
"system.posix_acl_{access,default}" xattrs requires the introduction of
libacl as a hard dependency because the exact format of the blob passed
around by setxattr/getxattr is private to libacl.
I think it's a bridge too far to have xfs_db depend on libacl for the
sake of non-XFS filesystems so I'll add a warning if we think we're
copying the ACL xattr to something that isn't an XFS filesystem.
--D
> --D
>
> > Ok. I think at least scrub phase5 does this.
> >
> > > > + if (XFS_IS_REALTIME_INODE(ip))
> > > > + btp = ip->i_mount->m_rtdev_targp;
> > > > + else
> > > > + btp = ip->i_mount->m_ddev_targp;
> > >
> > > Should be move xfs_inode_buftarg from kernel code to common code?
> >
> > Hmm. The xfs_inode -> xfs_buftarg translation could be moved to
> > libxfs/xfs_inode_util.c, yes. Though that can't happen until 6.15
> > because we're well past the merge window. For now I think it's the only
> > place in xfsprogs where we do that.
> >
> > --D
> >
>
