Re: [PATCH 3/4] xfs_io: make ls/chattr work with special files

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Aug 11, 2025 at 07:57:20PM +0200, Andrey Albershteyn wrote:
> On 2025-08-11 08:12:17, Darrick J. Wong wrote:
> > On Fri, Aug 08, 2025 at 09:30:18PM +0200, Andrey Albershteyn wrote:
> > > From: Andrey Albershteyn <aalbersh@redhat.com>
> > > 
> > > With new file_getattr/file_setattr syscalls we can now list/change file
> > > attributes on special files instead for ignoring them.
> > > 
> > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > ---
> > >  io/attr.c | 130 ++++++++++++++++++++++++++++++++++++--------------------------
> > >  1 file changed, 75 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/io/attr.c b/io/attr.c
> > > index fd82a2e73801..1cce602074f4 100644
> > > --- a/io/attr.c
> > > +++ b/io/attr.c
> > > @@ -8,6 +8,7 @@
> > >  #include "input.h"
> > >  #include "init.h"
> > >  #include "io.h"
> > > +#include "libfrog/file_attr.h"
> > >  
> > >  static cmdinfo_t chattr_cmd;
> > >  static cmdinfo_t lsattr_cmd;
> > > @@ -156,36 +157,35 @@ lsattr_callback(
> > >  	int			status,
> > >  	struct FTW		*data)
> > >  {
> > > -	struct fsxattr		fsx;
> > > -	int			fd;
> > > +	struct file_attr	fa;
> > > +	int			error;
> > >  
> > >  	if (recurse_dir && !S_ISDIR(stat->st_mode))
> > >  		return 0;
> > >  
> > > -	if ((fd = open(path, O_RDONLY)) == -1) {
> > > -		fprintf(stderr, _("%s: cannot open %s: %s\n"),
> > > -			progname, path, strerror(errno));
> > > -		exitcode = 1;
> > > -	} else if ((xfsctl(path, fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > > +	error = file_getattr(AT_FDCWD, path, stat, &fa, AT_SYMLINK_NOFOLLOW);
> > > +	if (error) {
> > >  		fprintf(stderr, _("%s: cannot get flags on %s: %s\n"),
> > >  			progname, path, strerror(errno));
> > >  		exitcode = 1;
> > > -	} else
> > > -		printxattr(fsx.fsx_xflags, 0, 1, path, 0, 1);
> > > +		return 0;
> > > +	}
> > > +
> > > +	printxattr(fa.fa_xflags, 0, 1, path, 0, 1);
> > 
> > Maybe it's time to rename this printxflags or something that's less
> > easily confused with extended attributes...
> 
> print_xflags()?

Sounds good to me.

> > 
> > >  
> > > -	if (fd != -1)
> > > -		close(fd);
> > >  	return 0;
> > >  }
> > >  
> > >  static int
> > >  lsattr_f(
> > > -	int		argc,
> > > -	char		**argv)
> > > +	int			argc,
> > > +	char			**argv)
> > >  {
> > > -	struct fsxattr	fsx;
> > > -	char		*name = file->name;
> > > -	int		c, aflag = 0, vflag = 0;
> > > +	struct file_attr	fa;
> > > +	char			*name = file->name;
> > > +	int			c, aflag = 0, vflag = 0;
> > > +	struct stat		st;
> > > +	int			error;
> > >  
> > >  	recurse_all = recurse_dir = 0;
> > >  	while ((c = getopt(argc, argv, "DRav")) != EOF) {
> > > @@ -211,17 +211,27 @@ lsattr_f(
> > >  	if (recurse_all || recurse_dir) {
> > >  		nftw(name, lsattr_callback,
> > >  			100, FTW_PHYS | FTW_MOUNT | FTW_DEPTH);
> > > -	} else if ((xfsctl(name, file->fd, FS_IOC_FSGETXATTR, &fsx)) < 0) {
> > > +		return 0;
> > > +	}
> > > +
> > > +	error = stat(name, &st);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	error = file_getattr(AT_FDCWD, name, &st, &fa, AT_SYMLINK_NOFOLLOW);
> > > +	if (error) {
> > >  		fprintf(stderr, _("%s: cannot get flags on %s: %s\n"),
> > >  			progname, name, strerror(errno));
> > >  		exitcode = 1;
> > > -	} else {
> > > -		printxattr(fsx.fsx_xflags, vflag, !aflag, name, vflag, !aflag);
> > > -		if (aflag) {
> > > -			fputs("/", stdout);
> > > -			printxattr(-1, 0, 1, name, 0, 1);
> > > -		}
> > > +		return 0;
> > >  	}
> > > +
> > > +	printxattr(fa.fa_xflags, vflag, !aflag, name, vflag, !aflag);
> > > +	if (aflag) {
> > > +		fputs("/", stdout);
> > > +		printxattr(-1, 0, 1, name, 0, 1);
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > @@ -232,44 +242,43 @@ chattr_callback(
> > >  	int			status,
> > >  	struct FTW		*data)
> > >  {
> > > -	struct fsxattr		attr;
> > > -	int			fd;
> > > +	struct file_attr	attr;
> > > +	int			error;
> > >  
> > >  	if (recurse_dir && !S_ISDIR(stat->st_mode))
> > >  		return 0;
> > >  
> > > -	if ((fd = open(path, O_RDONLY)) == -1) {
> > > -		fprintf(stderr, _("%s: cannot open %s: %s\n"),
> > > -			progname, path, strerror(errno));
> > > -		exitcode = 1;
> > > -	} else if (xfsctl(path, fd, FS_IOC_FSGETXATTR, &attr) < 0) {
> > > +	error = file_getattr(AT_FDCWD, path, stat, &attr, AT_SYMLINK_NOFOLLOW);
> > > +	if (error) {
> > >  		fprintf(stderr, _("%s: cannot get flags on %s: %s\n"),
> > >  			progname, path, strerror(errno));
> > >  		exitcode = 1;
> > > -	} else {
> > > -		attr.fsx_xflags |= orflags;
> > > -		attr.fsx_xflags &= ~andflags;
> > > -		if (xfsctl(path, fd, FS_IOC_FSSETXATTR, &attr) < 0) {
> > > -			fprintf(stderr, _("%s: cannot set flags on %s: %s\n"),
> > > -				progname, path, strerror(errno));
> > > -			exitcode = 1;
> > > -		}
> > > +		return 0;
> > > +	}
> > > +
> > > +	attr.fa_xflags |= orflags;
> > > +	attr.fa_xflags &= ~andflags;
> > > +	error = file_setattr(AT_FDCWD, path, stat, &attr, AT_SYMLINK_NOFOLLOW);
> > > +	if (error) {
> > > +		fprintf(stderr, _("%s: cannot set flags on %s: %s\n"),
> > > +			progname, path, strerror(errno));
> > > +		exitcode = 1;
> > >  	}
> > >  
> > > -	if (fd != -1)
> > > -		close(fd);
> > >  	return 0;
> > >  }
> > >  
> > >  static int
> > >  chattr_f(
> > > -	int		argc,
> > > -	char		**argv)
> > > +	int			argc,
> > > +	char			**argv)
> > >  {
> > > -	struct fsxattr	attr;
> > > -	struct xflags	*p;
> > > -	unsigned int	i = 0;
> > > -	char		*c, *name = file->name;
> > > +	struct file_attr	attr;
> > > +	struct xflags		*p;
> > > +	unsigned int		i = 0;
> > > +	char			*c, *name = file->name;
> > > +	struct stat		st;
> > > +	int			error;
> > >  
> > >  	orflags = andflags = 0;
> > >  	recurse_all = recurse_dir = 0;
> > > @@ -326,19 +335,30 @@ chattr_f(
> > >  	if (recurse_all || recurse_dir) {
> > >  		nftw(name, chattr_callback,
> > >  			100, FTW_PHYS | FTW_MOUNT | FTW_DEPTH);
> > > -	} else if (xfsctl(name, file->fd, FS_IOC_FSGETXATTR, &attr) < 0) {
> > > +		return 0;
> > > +	}
> > > +
> > > +	error = stat(name, &st);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	error = file_getattr(AT_FDCWD, name, &st, &attr, AT_SYMLINK_NOFOLLOW);
> > > +	if (error) {
> > >  		fprintf(stderr, _("%s: cannot get flags on %s: %s\n"),
> > >  			progname, name, strerror(errno));
> > >  		exitcode = 1;
> > > -	} else {
> > > -		attr.fsx_xflags |= orflags;
> > > -		attr.fsx_xflags &= ~andflags;
> > > -		if (xfsctl(name, file->fd, FS_IOC_FSSETXATTR, &attr) < 0) {
> > > -			fprintf(stderr, _("%s: cannot set flags on %s: %s\n"),
> > > -				progname, name, strerror(errno));
> > > -			exitcode = 1;
> > > -		}
> > > +		return 0;
> > >  	}
> > > +
> > > +	attr.fa_xflags |= orflags;
> > > +	attr.fa_xflags &= ~andflags;
> > > +	error = file_setattr(AT_FDCWD, name, &st, &attr, AT_SYMLINK_NOFOLLOW);
> > 
> > For my own curiosity, if you wanted to do the get/set sequence on a file
> > that's already open, is that just:
> > 
> > 	file_getattr(fd, "", &attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> > 	...
> > 	file_setattr(fd, "", &attr, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
> 
> yeah, that should work

Cool, thanks for confirming my understanding. :)

--D

> > 
> > --D
> > 
> > > +	if (error) {
> > > +		fprintf(stderr, _("%s: cannot set flags on %s: %s\n"),
> > > +			progname, name, strerror(errno));
> > > +		exitcode = 1;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > 
> > > -- 
> > > 2.49.0
> > > 
> > > 
> > 
> 
> -- 
> - Andrey
> 
>