[PATCH v5] Implement decoding of statx syscall

[Dmitry V. Levin]

On Wed, Mar 15, 2017 at 02:57:02PM +0300, Victor Krapivensky wrote:
> * Makefile.am (strace_SOURCES): Add statx.c and stat.h.

There is a shorter form for this:

* statx.c: New file.
* statx.h: Likewise.
* Makefile.am (strace_SOURCES): Add them.

> * linux/i386/syscallent.h: Add statx entry.
> * linux/x32/syscallent.h: Likewise.
> * linux/x86_64/syscallent.h: Likewise.

The current practice is either add a number, e.g.
* linux/i386/syscallent.h [383]: Add statx entry.
or add a name, e.g.
* linux/i386/syscallent.h (statx): New entry.

[...]
> +		printxvals(tcp->u_arg[2] & AT_STATX_SYNC_TYPE, "AT_STATX_???",
> +		           at_statx_sync_types, NULL);

I think printxval would be easier to understand than printxvals.

[...]
> +#include "tests.h"
> +#include <asm/unistd.h>
> +#include <linux/stat.h>
> +#include "xlat.h"
> +#include "xlat/statx_masks.h"
> +#include "xlat/statx_attrs.h"
> +#include "xlat/at_statx_sync_types.h"
> +
> +#ifdef __NR_statx

The current practice is to place this ifdef right after <asm/unistd.h>.

[...]
Let's add "static" to all these variables.

> +
> +# define TEST_SYSCALL_INVOKE(sample, pst) \
> +	syscall(__NR_statx, AT_FDCWD, sample, TEST_SYSCALL_STATX_FLAGS, \
> +	        TEST_SYSCALL_STATX_MASK, pst)
> +# define PRINT_SYSCALL_HEADER(sample) \
> +	do { \
> +		int saved_errno = errno; \
> +		printf("%s(AT_FDCWD, \"%s\", %s, %s, ", \
> +		       TEST_SYSCALL_STR, sample, TEST_SYSCALL_STATX_FLAGS_STR, \
> +		       TEST_SYSCALL_STATX_MASK_STR)
> +# define PRINT_SYSCALL_FOOTER(rc) \
> +		errno = saved_errno; \
> +		printf(") = %s\n", sprintrc(rc)); \
> +	} while (0)
> +
> +# include "xstatx.c"
> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("__NR_statx")
> +
> +#endif
> +

This blank line at the end has caused this "git am" response:

Applying: Implement decoding of statx syscall
.git/rebase-apply/patch:385: new blank line at EOF.
+
fatal: 1 line adds whitespace errors.
Patch failed at 0001 Implement decoding of statx syscall

Please don't. :)

[...]
> @@ -246,19 +305,26 @@ main(void)
>  			return 77;
>  		}
>  	}
> -	(void) unlink(sample);

If you move this unlink invocation, please add it on error paths.

> +# if IS_STATX
> +#  define ST_SIZE_FIELD stx_size
> +# else
> +#  define ST_SIZE_FIELD st_size
> +# endif
>  	if (!rc && zero_extend_signed_to_ull(SAMPLE_SIZE) !=
> -	    zero_extend_signed_to_ull(st[0].st_size)) {
> +	    zero_extend_signed_to_ull(st[0].ST_SIZE_FIELD)) {
>  		fprintf(stderr, "Size mismatch: "
>  				"requested size(%llu) != st_size(%llu)\n",
>  			zero_extend_signed_to_ull(SAMPLE_SIZE),
> -			zero_extend_signed_to_ull(st[0].st_size));
> +			zero_extend_signed_to_ull(st[0].ST_SIZE_FIELD));
>  		fprintf(stderr, "The most likely reason for this is incorrect"
>  				" definition of %s.\n"
>  				"Here is some diagnostics that might help:\n",
>  			STRUCT_STAT_STR);

There is no diagnostics in case of IS_STATX.
Someone seems to be too lazy to split the message. :)

> -#define LOG_STAT_OFFSETOF_SIZEOF(object, member)			\
> +# if !IS_STATX
> +
> +#  define LOG_STAT_OFFSETOF_SIZEOF(object, member)			\
>  		fprintf(stderr, "offsetof(%s, %s) = %zu"		\
>  				", sizeof(%s) = %zu\n",			\
>  				STRUCT_STAT_STR, #member,		\
> @@ -273,10 +339,12 @@ main(void)
>  		LOG_STAT_OFFSETOF_SIZEOF(st[0], st_gid);
>  		LOG_STAT_OFFSETOF_SIZEOF(st[0], st_rdev);
>  		LOG_STAT_OFFSETOF_SIZEOF(st[0], st_size);
> -# if !OLD_STAT
> +#  if !OLD_STAT
>  		LOG_STAT_OFFSETOF_SIZEOF(st[0], st_blksize);
>  		LOG_STAT_OFFSETOF_SIZEOF(st[0], st_blocks);
> -# endif /* !OLD_STAT */
> +#  endif /* !OLD_STAT */
> +
> +# endif /* !IS_STATX */
>  
>  		return 1;

Please add that unlink(sample) call here.

>  	}
> @@ -288,6 +356,74 @@ main(void)
>  		print_stat(st);
>  	PRINT_SYSCALL_FOOTER(rc);
>  
> +# 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.)

This is not the way we do multiline comments, please convert.

The last but not least, please add a line to NEWS file.

On the whole, this is very close to what I'd like to see as a statx parser.
I've removed it from the list of available microprojects.


-- 
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/20170316/1fdd70b9/attachment.bin>