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
pgprut3pVys9k.pgp
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
