[PATCH v3] Implement decoding of ustat syscall

[Dmitry V. Levin]

On Sat, Jan 07, 2017 at 06:22:44PM +0800, JingPiao Chen wrote:
[...]
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 57dd8b9..ceeccd8 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -414,6 +414,7 @@ check_PROGRAMS = \
>  	utime \
>  	utimensat \
>  	utimes \
> +	ustat \
>  	vfork-f \
>  	vhangup \
>  	vmsplice \

Please keep this list sorted.

> @@ -796,6 +797,7 @@ DECODER_TESTS = \
>  	utime.test \
>  	utimensat.test \
>  	utimes.test \
> +	ustat.test \
>  	vhangup.test \
>  	vmsplice.test \
>  	wait4.test \

Likewise.

[...]
> +#include "tests.h"
> +#include <asm/unistd.h>
> +
> +#ifdef __NR_ustat
> +
> +#include <stdio.h>

Please indent.

> +#include <stdlib.h>

Include <sys/sysmacros.h> instead.

> +#include <unistd.h>
> +#include <ustat.h>
> +
> +int
> +main(int argc, char *argv[])
> +{
> +	unsigned int ma = atoi(argv[1]);
> +	unsigned int mi = atoi(argv[2]);
> +	dev_t dev = makedev(ma, mi);

This is not needed, it's easier to obtain a valid device number without
any external help.

> +	kernel_ulong_t magic = (kernel_ulong_t) 0xfacefeedffffffff;
> +	struct ustat *ust = tail_alloc(sizeof(*ust));
> +
> +	long rc = syscall(__NR_ustat, magic, 0);
> +	printf("ustat(makedev(%u, %u), NULL) = %s\n", major((unsigned int) magic),
> +	       minor((unsigned int) magic), sprintrc(rc));

This looks awkward, try assigning "magic" to a variable of type unsigned int
instead, e.g.

	unsigned int dev = (unsigned int) magic;
	long rc = syscall(__NR_ustat, magic, 0);
	printf("ustat(makedev(%u, %u), NULL) = %s\n",
	       major(dev), minor(dev), sprintrc(rc));

> +	rc = syscall(__NR_ustat, magic, ust);
> +	printf("ustat(makedev(%u, %u), %p) = %s\n", major((unsigned int) magic),
> +	       minor((unsigned int) magic), ust, sprintrc(rc));

Likewise.

At this point you can obtain a valid device number, assign it to "dev",
check for overflow, and proceed with the check for decoding of potentially
successful ustat invocation.

> +	rc = syscall(__NR_ustat, dev, ust);
> +	printf("ustat(makedev(%u, %u), {f_tfree=%llu, f_tinode=%llu}) = %s\n",
> +	       ma, mi, zero_extend_signed_to_ull(ust->f_tfree),
> +	       zero_extend_signed_to_ull(ust->f_tinode), sprintrc(rc));
> +
> +	puts("+++ exited with 0 +++");
> +	return 0;
> +}
> +
> +#else
> +
> +SKIP_MAIN_UNDEFINED("__NR_ustat")
> +
> +#endif
> diff --git a/tests/ustat.test b/tests/ustat.test
> new file mode 100755
> index 0000000..e7f2867
> --- /dev/null
> +++ b/tests/ustat.test
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +# Check ustat syscall decoding.
> +
> +. "${srcdir=.}/init.sh"
> +
> +get_fs_dev_t()
> +{
> +	name=$(ls /proc/fs/ext4/ | awk '{print $1}')
> +	fs_info=$(grep "$name" /proc/diskstats)
> +	major=$(echo $fs_info | awk '{print $1}')
> +	minor=$(echo $fs_info | awk '{print $2}')
> +}
> +
> +check_prog awk
> +check_prog grep
> +get_fs_dev_t
> +run_prog ./ustat $major $minor > /dev/null
> +run_strace -a25 -eustat ./ustat $major $minor  > "$EXP"
> +match_diff "$LOG" "$EXP"
> +rm -f "$EXP" "$LOG"

No, this is too complicated and fragile.
For example, in my test environment
$ ls /proc/fs/ext4/
ls: cannot access /proc/fs/ext4/: No such file or directory

Please revert to the previous edition of ustat.test and obtain
a valid device number inside ustat.c.

> diff --git a/ustat.c b/ustat.c
> new file mode 100644
> index 0000000..988692c
> --- /dev/null
> +++ b/ustat.c
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2017 JingPiao Chen <chenjingpiao at gmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "defs.h"
> +#include DEF_MPERS_TYPE(struct_ustat)
> +#include <ustat.h>
> +typedef struct ustat struct_ustat;
> +#include MPERS_DEFS
> +
> +SYS_FUNC(ustat)
> +{
> +	struct_ustat ust;

Let's move this definition ...

> +	if (entering(tcp))
> +		print_dev_t((unsigned int) tcp->u_arg[0]);
> +	else {

... here.

> +		tprints(", ");
> +		if (!umove_or_printaddr(tcp, tcp->u_arg[1], &ust))
> +			tprintf("{f_tfree=%llu, f_tinode=%llu}",
> +				zero_extend_signed_to_ull(ust.f_tfree),
> +				zero_extend_signed_to_ull(ust.f_tinode));
> +	}
> +
> +	return 0;
> +}

-- 
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/20170107/cfdf3b2e/attachment.bin>