[PATCH v7] Implement decoding of statx syscall

[Dmitry V. Levin]

On Fri, Mar 17, 2017 at 10:58:35PM +0300, Victor Krapivensky wrote:
> * linux/i386/syscallent.h [383]: Add statx entry.
> * linux/x32/syscallent.h [332]: Likewise.
> * linux/x86_64/syscallent.h [332]: Likewise.
> * pathtrace.c (pathtrace_match): Handle SEN_statx.
> * statx.c: New file.
> * statx.h: Likewise.
> * Makefile.am (strace_SOURCES): Add them.
> * tests/.gitignore: Add statx.
> * tests/Makefile.am (check_PROGRAMS): Likewise.
> (DECODER_TESTS): Add statx.test.
> * tests/statx.c: New file.
> * tests/statx.test: Likewise.
> * tests/xstatx.c: Modify to support statx.
> * xlat/at_statx_sync_types.in: New file.
> * xlat/statx_attrs.in: Likewise.
> * xlat/statx_masks.in: Likewise.
> * NEWS: Mention this change.

Just a few nitpicks:

[...]
> --- /dev/null
> +++ b/xlat/at_statx_sync_types.in
> @@ -0,0 +1,5 @@
> +AT_STATX_SYNC_AS_STAT	0x0000
> +AT_STATX_FORCE_SYNC	0x2000
> +AT_STATX_DONT_SYNC	0x4000
> +
> +AT_STATX_SYNC_TYPE	0x6000

I suggest placing AT_STATX_SYNC_TYPE before AT_STATX_FORCE_SYNC
so we could use printflags.

[...]
> +SYS_FUNC(statx)
> +{
> +	if (entering(tcp)) {
> +		print_dirfd(tcp, tcp->u_arg[0]);
> +		printpath(tcp, tcp->u_arg[1]);
> +		tprints(", ");
> +		if (printflags(at_flags, tcp->u_arg[2] & ~AT_STATX_SYNC_TYPE,
> +			       NULL))
> +			tprints("|");
> +		printxval(at_statx_sync_types,
> +			  tcp->u_arg[2] & AT_STATX_SYNC_TYPE, "AT_STATX_???");

Looking at this parser and its test:

> +	SET_FLAGS_INVOKE(AT_SYMLINK_FOLLOW | 0xffff0000U,
> +		"AT_SYMLINK_FOLLOW|0xffff0000|AT_STATX_SYNC_AS_STAT");

I think at_statx_sync_types should go first so the output would be
a more traditional AT_STATX_SYNC_AS_STAT|AT_SYMLINK_FOLLOW|0xffff0000,
e.g.

		unsigned int flags = tcp->u_arg[2];
		printflags(at_statx_sync_types, flags & AT_STATX_SYNC_TYPE, NULL);
		flags &= ~AT_STATX_SYNC_TYPE;
		if (flags) {
			tprints("|");
			printflags(at_flags, flags, NULL);
		}

[...]
> +# if IS_STATX
> +
> +	/*
> +	 * Check that our idea of struct_statx is no smaller than the kernel's.
> +	 * (If it is, statx is expected to fail with EFAULT.)
> +	 */
> +	STRUCT_STAT *st_cut2 = tail_alloc(sizeof(struct_statx));
> +	if ((rc = TEST_SYSCALL_INVOKE(sample, st_cut2))) {
> +		perror(TEST_SYSCALL_STR);
> +		(void) unlink(sample);
> +		return 1;
> +	}
> +	PRINT_SYSCALL_HEADER(sample);
> +	print_stat(st_cut2);
> +	PRINT_SYSCALL_FOOTER(rc);

I was looking at this hunk and thinking why can't we have this check
for other stat related syscalls as well.  We used to employ stat
definitions provided by libc, but recently we switched to asm/stat.h
for all these tests, so I changed the code in commit v4.16-59-g12781b7
to use a tail_alloc'ed object.

So now "st" is a tail_alloc'ed object, this extra check has completed
its mission and could be removed.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170318/a5195e64/attachment.bin>