Re: [PATCH v4 1/3] file_attr: introduce program to set/get fsxattr

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

On Mon, Oct 06, 2025 at 11:37:53AM +0200, Andrey Albershteyn wrote:
> On 2025-10-05 18:36:56, Zorro Lang wrote:
> > On Fri, Oct 03, 2025 at 11:32:44AM +0200, Andrey Albershteyn wrote:
> > > This programs uses newly introduced file_getattr and file_setattr
> > > syscalls. This program is partially a test of invalid options. This will
> > > be used further in the test.
> > > 
> > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > ---
> > 
> > [snap]
> > 
> > > +	if (!path1 && optind < argc)
> > > +		path1 = argv[optind++];
> > > +	if (!path2 && optind < argc)
> > > +		path2 = argv[optind++];
> > > +
> > > +	if (at_fdcwd) {
> > > +		fd = AT_FDCWD;
> > > +		path = path1;
> > > +	} else if (!path2) {
> > > +		error = stat(path1, &status);
> > > +		if (error) {
> > > +			fprintf(stderr,
> > > +"Can not get file status of %s: %s\n", path1, strerror(errno));
> > > +			return error;
> > > +		}
> > > +
> > > +		if (SPECIAL_FILE(status.st_mode)) {
> > > +			fprintf(stderr,
> > > +"Can not open special file %s without parent dir: %s\n", path1, strerror(errno));
> > > +			return errno;
> > > +		}
> > > +
> > > +		fd = open(path1, O_RDONLY);
> > > +		if (fd == -1) {
> > > +			fprintf(stderr, "Can not open %s: %s\n", path1,
> > > +					strerror(errno));
> > > +			return errno;
> > > +		}
> > > +	} else {
> > > +		fd = open(path1, O_RDONLY);
> > > +		if (fd == -1) {
> > > +			fprintf(stderr, "Can not open %s: %s\n", path1,
> > > +					strerror(errno));
> > > +			return errno;
> > > +		}
> > > +		path = path2;
> > > +	}
> > > +
> > > +	if (!path)
> > > +		at_flags |= AT_EMPTY_PATH;
> > > +
> > > +	error = file_getattr(fd, path, &fsx, fa_size,
> > > +			at_flags);
> > > +	if (error) {
> > > +		fprintf(stderr, "Can not get fsxattr on %s: %s\n", path,
> > > +				strerror(errno));
> > > +		return error;
> > > +	}
> > 
> > We should have a _require_* helper to _notrun your generic and xfs test cases,
> > when system doesn't support the file_getattr/setattr feature. Or we always hit
> > something test errors like below on old system:
> > 
> >   +Can not get fsxattr on ./fifo: Operation not supported
> > 
> > Maybe check if the errno is "Operation not supported", or any better idea?
> 
> There's build system check for file_getattr/setattr syscalls, so if
> they aren't in the kernel file_attr will not compile.
> 
> Then there's _require_test_program "file_attr" in the tests, so
> these will not run if kernel doesn't have these syscalls.
> 
> However, for XFS for example, there's [1] and [2] which are
> necessary for these tests to pass. 
> 
> So, there a few v6.17 kernels which would still run these tests but
> fail for XFS (and still fails as these commits are in for-next now).
> 
> For other filesystems generic/ will also fail on newer kernels as it
> requires similar modifications as in XFS to support changing file
> attributes on special files.
> 
> I suppose it make sense for this test to fail for other fs which
> don't implement changing file attributes on special files.
> Otherwise, this test could be split into generic/ (file_get/setattr
> on regular files) and xfs/ (file_get/setattr on special files).
> 
> What do you think?

generic/772 (and xfs/648) probably each ought to be split into two
pieces -- one for testing file_[gs]etattr on regular/directory files;
and a second one for the special files.  All four of them ought to
_notrun if the kernel doesn't support the intended target.

Right now I have injected into both:

mkfifo $projectdir/fifo

$here/src/file_attr --get $projectdir ./fifo &>/dev/null || \
	_notrun "file_getattr not supported on $FSTYP"

to make the failures go away on 6.17.

--D

> [1]: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/commit/?h=for-next&id=8a221004fe5288b66503699a329a6b623be13f91
> [2]: https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/commit/?h=for-next&id=0239bd9fa445a21def88f7e76fe6e0414b2a4da0
> 
> > 
> > 
> > Thanks,
> > Zorro
> > 
> > > +	if (action) {
> > > +		fsx.fa_xflags |= (fa_xflags | unknwon_fa_flag);
> > > +
> > > +		error = file_setattr(fd, path, &fsx, fa_size,
> > > +				at_flags);
> > > +		if (error) {
> > > +			fprintf(stderr, "Can not set fsxattr on %s: %s\n", path,
> > > +					strerror(errno));
> > > +			return error;
> > > +		}
> > > +	} else {
> > > +		if (path2)
> > > +			print_xflags(fsx.fa_xflags, 0, 1, path, 0, 1);
> > > +		else
> > > +			print_xflags(fsx.fa_xflags, 0, 1, path1, 0, 1);
> > > +	}
> > > +
> > > +	return error;
> > > +
> > > +usage:
> > > +	printf("Usage: %s [options]\n", argv[0]);
> > > +	printf("Options:\n");
> > > +	printf("\t--get, -g\t\tget filesystem inode attributes\n");
> > > +	printf("\t--set, -s\t\tset filesystem inode attributes\n");
> > > +	printf("\t--at-cwd, -a\t\topen file at current working directory\n");
> > > +	printf("\t--no-follow, -n\t\tdon't follow symlinks\n");
> > > +	printf("\t--set-nodump, -d\t\tset FS_XFLAG_NODUMP on an inode\n");
> > > +	printf("\t--invalid-at, -i\t\tUse invalid AT_* flag\n");
> > > +	printf("\t--too-big-arg, -b\t\tSet fsxattr size bigger than PAGE_SIZE\n");
> > > +	printf("\t--too-small-arg, -m\t\tSet fsxattr size to 19 bytes\n");
> > > +	printf("\t--new-fsx-flag, -x\t\tUse unknown fa_flags flag\n");
> > > +
> > > +	return 1;
> > > +}
> > > 
> > > -- 
> > > 2.50.1
> > > 
> > 
> 
> -- 
> - Andrey
> 
>